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

Improve policy validation #113

Merged
merged 7 commits into from
May 1, 2020
Merged

Improve policy validation #113

merged 7 commits into from
May 1, 2020

Conversation

lgfa29
Copy link
Contributor

@lgfa29 lgfa29 commented Apr 30, 2020

This PR improves policy validation in a few ways:

  1. validate the right target component: the initial implementation only validated ScalingPolicy.Target which is a map[string]string and doesn't actually need validation.
  2. encapsulate the logic to validate an HCL block in a function: this allows us to re-use the validation logic in different places, such as strategy and target.
  3. return error messages related to the jobspec when possible: previously the error messages would be related to the specific internal structure of the policy, which is not significant to end users. New error messages use the same block1->block2 notation from the Nomad docs.
  4. only test validateScalingPolicy: this is the main function that calls all the others. Testing individual sub-functions failed to capture some cases.
  5. expand test cases: test cases now have a full policy as input, as opposed to selectively remove fields from a known valid input. This makes it more clear to understand what is being tested.
  6. show validation error message in tests: it's useful to see how validation messages will be displayed to users. You can use the -show-validation-error flag when running tests to seem them:
go test -v -run Test_validate ./policy/nomad/ -show-validation-error

@lgfa29 lgfa29 requested review from cgbaker and jrasell April 30, 2020 16:51
@lgfa29 lgfa29 self-assigned this Apr 30, 2020
@lgfa29 lgfa29 added the policy label Apr 30, 2020
Copy link
Contributor

@cgbaker cgbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good stuff 👍

policy/nomad/validate.go Show resolved Hide resolved
@lgfa29 lgfa29 merged commit 186b24c into master May 1, 2020
@lgfa29 lgfa29 deleted the improve-policy-validation branch May 1, 2020 14:34
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

Successfully merging this pull request may close these issues.

2 participants