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

Audit validation tags for configuration properties #1491

Open
bluekeyes opened this issue Dec 12, 2019 · 3 comments
Open

Audit validation tags for configuration properties #1491

bluekeyes opened this issue Dec 12, 2019 · 3 comments

Comments

@bluekeyes
Copy link

Is your feature request related to a problem? Please describe.

For some properties (I hit it on DownloadMode, but may apply to others), the zero value is invalid and the property is effectively required, but is not marked as required for validation.

Describe the solution you'd like

Audit the configuration type definitions to make sure the validation tags are correct for properties. I think the required tag is the most relevant, since a missing required property gives a clear error at startup, while the zero value for the type may give a confusing error later on.

Describe alternatives you've considered

Introduce another method to validate configuration. This seems non-ideal, since validation tags are already used for some properties.

Additional context

This issue is split out of #1336, which has some additional information.

@arschles
Copy link
Member

@bluekeyes We should certainly get better at validating configurations. Since there are a lot of options, it's easy to forget something and make a mistake, so I think we should look holistically at the configuration. For example, if DownloadMode is set to "redirect", then DownloadURL needs to be set. We should check that and report it and disallow Athens from starting.

What do you think?

arschles added a commit to arschles/athens that referenced this issue Dec 12, 2019
When it's missing, we write a short message saying so, and when it's invalid, we do approximately the same thing. Athens has a lot of configuration options, so it's naturally easy to get them wrong or forget something. Nicer error messages should help.

If this code is acceptable, I'd like to apply it to a few other config values (in a follow-up PR) that are easy to get wrong.

ref gomods#1491, since this checks existence and validates values for download mode, and also reports errors in a more human-friendly way

cc/ @bluekeyes
@bluekeyes
Copy link
Author

Yeah, it makes sense to me that the application would need a way to validate relationships between properties and that that would provide a better user experience than just saying if an option is required or not.

From a pure code perspective, I personally find it a bit confusing when there are multiple validation methods at play in a project, so long term, I think it could be valuable to decide on when to use the validator.v9 library (if at all) vs. when to use any new method introduced here. But I'm also not very familiar with the Athens code base or development style.

@arschles
Copy link
Member

@bluekeyes I don't have any experience with validator.v9, but it looks nice. The idea I have in mind is to centralize all validation of configuration in one place in the code, regardless of where the configuration values came from - file, environment variables, or anywhere else we define later on.

Ideally, the validations would come with good error messages too, when Athens is misconfigured. I'll check out validator.v9 to see if I can get it working in #1492 too - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants