Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ZEP-0015: Helm Value Style Variable Interfaces #16
base: main
Are you sure you want to change the base?
ZEP-0015: Helm Value Style Variable Interfaces #16
Changes from 2 commits
3ee8cfd
99cc648
2e60973
8c39077
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not a fan of having four different formats. I doubt many are using .ini or .json formats. IMO it would be best to deprecate at least these two.
I think the argument can be made to keep only one format around. However, yaml and toml both have their use cases. toml is nice since Zarf tends to use hierarchies a lot (e.g. [zarf.package.create]), yaml is more intuitive when defining variables for Helm values files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're considering changing those, it would be nice to explicitly call out the possibility of deprecating and removing formats other than yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue the existing behavior of Helm overrides is not ideal and should just be changed. It helps that the feature is in "alpha" (though the Zarf team really needs to better track the state of our features). This comment gives a good example of the unexpected behavior when everything is converted to a string zarf-dev/zarf#2783 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am more worried about the risk of a breaking change on the
--set
flag, but I would still lean towards introducing this as a breaking change and avoiding the overhead of command line flags. Do you know of any situations in Helm charts where a user is likely to want their integer (or other type) represented as a string?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot imagine this functionality being added without extensive fuzzy tests, ensuring that we're not able to break zarf injecting various random strings, including to potential code injections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this in the pre-req section since it would be a new type of testing (I believe) and also added some links to how Helm handles this internally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: missing history, something as simple as: