Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supply a required property for each inputField #3430

Closed
theosanderson opened this issue Dec 11, 2024 · 4 comments
Closed

Supply a required property for each inputField #3430

theosanderson opened this issue Dec 11, 2024 · 4 comments
Labels
website Tasks related to the web application

Comments

@theosanderson
Copy link
Member

theosanderson commented Dec 11, 2024

inputFields are the fields used to define what input names the preprocessing pipeline is expecting. While these often correspond to metadata field names technically they can and will sometimes be distinct.

Some inputFields are required, and the schema should capture this to allow the website to display things accordingly.

(Cornelius refactored inputFields to be "DRY" and largely derived from the main field spec, for these we may be able to pass on the required property from the main field - but we should think about if that is actually the same property. My own instinct is to be less DRY and just to set out the inputFields explicitly separately, but this may not be required.)

@fhennig
Copy link
Contributor

fhennig commented Dec 12, 2024

My intuition when reasoning about these fields would be that any overlap with the metadata fields is incidental and that they are semantically distinct. I can see how it might be nice to have some tooling or syntactical support to allow for brevity when configuring them though.

@fhennig fhennig added the website Tasks related to the web application label Dec 12, 2024
@anna-parker
Copy link
Contributor

We now pass on the desired field to the website when generating the inputFields from the values.yaml: https://github.com/loculus-project/loculus/blob/main/kubernetes/loculus/templates/_inputFieldsFromValues.tpl#L5. I think this solves the issue at least for now, we allow additional input fields to be passed using extraInputFields in the values.yaml.

So I think this resolves the issue for now - @theosanderson is it ok to close this or would you still like to refactor the values.yaml to split the inputFields and the metadata more?

@theosanderson
Copy link
Member Author

Thanks, I agree the issue as titled is resolved. I can make another if I decide to push the refactoring thing! (I'll let you resolve in case you want to link to your PR or whatever)

@anna-parker
Copy link
Contributor

resolved with #3477

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
website Tasks related to the web application
Projects
None yet
Development

No branches or pull requests

3 participants