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

Feat/schema validation #5350

Merged
merged 30 commits into from
Apr 26, 2019
Merged

Conversation

ian-howell
Copy link
Contributor

@ian-howell ian-howell commented Feb 22, 2019

What this PR does / why we need it:
When complete, this PR will close 5081.

Special notes for your reviewer:
Very much a work in progress. Any advice is greatly appreciated.

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@helm-bot helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 22, 2019
@bacongobbler
Copy link
Member

bacongobbler commented Feb 22, 2019

From the conversation in #2431 and in the proposal, it was agreed that utilizing JSON schema for validation was the right way forward. Would you mind looking into that?

If you're looking for inspiration, the CNAB project utilizes JSON schema validation for the application bundle schema. Here's an example of how that schema looks.

@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 25, 2019
@ian-howell
Copy link
Contributor Author

Glad to see that this already exists and doesn't need to be built from scratch. I was getting pretty deep in the weeds with some of those changes...

The last commit should now meet the basic requirements of the spec. There's still some features to be added though.

@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 26, 2019
@helm-bot helm-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 26, 2019
@bacongobbler
Copy link
Member

There's still some features to be added though.

Okay. Let me know when those features have been added and I'll take another look. Thanks!

@ian-howell
Copy link
Contributor Author

The proposal mentions being able to generate a JSON schema from a given values file. However, Values are currently stored as map[string]interface{}, which means that the only information we can draw from a given property is its type. The types that we can determine are limited to what the json library's Unmarshal uses, which means no integers. Furthermore, if a property is null, there's no way to determine its type, and it must be omitted.

This last commit adds that functionality, but given the current limitations, the schemas it generates are very bare. In practice, a user could generate a schema, then modify it with more specific information. Is this the intention for this feature?

@helm-bot helm-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 27, 2019
@ian-howell
Copy link
Contributor Author

@bacongobbler This is just about finished, but I'm not sure where to put schema generation. Do we want to add a flag to install?

@adamreese adamreese added the v3.x Issues and Pull Requests related to the major version v3 label Feb 28, 2019
@bacongobbler
Copy link
Member

Hey @ian-howell, thanks for the ping! I'll have a look at this tomorrow and see what we need to do to tie this one off. Thanks for taking the time to implement this!

Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

I'm incredibly happy with the current status of this PR. Really well done! Just one small nit about the error output, but asides that this is shaping up to be a great submission. Awesome work!

Are you planning on adding documentation so others know how to use this new functionality, or would you like some assistance getting started with that?

pkg/chartutil/values.go Outdated Show resolved Hide resolved
@helm-bot helm-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 7, 2019
Signed-off-by: Ian Howell <ian.howell0@gmail.com>
Signed-off-by: Ian Howell <ian.howell0@gmail.com>
Signed-off-by: Ian Howell <ian.howell0@gmail.com>
Signed-off-by: Ian Howell <ian.howell0@gmail.com>
Signed-off-by: Ian Howell <ian.howell0@gmail.com>
The final coalesced values for a given chart will be validated against
that chart's schema, as well as any dependent subchart's schema

Signed-off-by: Ian Howell <ian.howell0@gmail.com>
Signed-off-by: Ian Howell <ian.howell0@gmail.com>
Signed-off-by: Ian Howell <ian.howell0@gmail.com>
The TestReadSchema unit test was simply testing the ReadValues function,
which is already being validated in the TestReadValues unit test

Signed-off-by: Ian Howell <ian.howell0@gmail.com>
Signed-off-by: Ian Howell <ian.howell0@gmail.com>
@ian-howell
Copy link
Contributor Author

@adamreese Alright, this should be good to go. Let me know if there's anymore changes I need to make.

Copy link
Member

@adamreese adamreese left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for making all the requested changes and esp the unit tests!!!

I might need to move a few things around with some of the load/render stages but this is awesome!

@adamreese adamreese merged commit ffff0e8 into helm:dev-v3 Apr 26, 2019
Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

LGTM as well. Thank you so much for all your hard work @ian-howell! This looks great. Can't wait to play around with it some more :)

@ghost
Copy link

ghost commented Oct 23, 2019

I think we can leave that decision up to the chart author. [...] we could support files named values.schema.(yaml|json).

I think this would be great. If values.schema.json exists, use that. Otherwise if values.schema.yaml exists use that. Feed both through a yaml parser and there wouldn't be any issues, JSON is valid YAML after all.

Personally, I have already written all of my JSON schema files in yaml a long time ago since that's what the specification in the community roadmap had said to do and have been using that in my CI/CD workflows. I'm sure I'm not alone.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. v3.x Issues and Pull Requests related to the major version v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants