ZEP-0015: Helm Value Style Variable Interfaces#16
Conversation
Racer159
commented
Feb 3, 2025
- One-line PR description: Adds a proposal for Zarf Variables to be handled more like Helm Values.
- Issue link: Helm Values Style Variable Interfaces #15
- Other comments:
Signed-off-by: Wayne Starr <me@racer159.com>
a871ff9 to
3ee8cfd
Compare
AustinAbro321
left a comment
There was a problem hiding this comment.
Thanks for writing this up, added a few comments now. The main question is our appetite for breaking changes. I suspect they will be okay here.
|
|
||
| ### Risks and Mitigations | ||
|
|
||
| This will require some additional processing of the `zarf-config` files to allow them to be processed properly because of Viper's long-running [case-insensitive keys issue](https://github.com/spf13/viper/issues/1014). This could be performed similar to UDS CLI's implementation of this feature and / or by deprecating some of the existing less-used formats for configuration. |
There was a problem hiding this comment.
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.
If we're considering changing those, it would be nice to explicitly call out the possibility of deprecating and removing formats other than yaml.
|
|
||
| This will require some additional processing of the `zarf-config` files to allow them to be processed properly because of Viper's long-running [case-insensitive keys issue](https://github.com/spf13/viper/issues/1014). This could be performed similar to UDS CLI's implementation of this feature and / or by deprecating some of the existing less-used formats for configuration. | ||
|
|
||
| The `--set` syntax will change somewhat how variables are interpreted on the CLI (i.e. `--set VAR=100` will no monger represent `"100"` and instead will just be `100` internally). For `###` templates this can be mitigated by simply representing the value as a string for backwards compatibility though this will likely need to either be marked as breaking for Helm overrides or have additional flag changes / opt ins for this specific feature. |
There was a problem hiding this comment.
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.
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?
| This will require some additional processing of the `zarf-config` files to allow them to be processed properly because of Viper's long-running [case-insensitive keys issue](https://github.com/spf13/viper/issues/1014). This could be performed similar to UDS CLI's implementation of this feature and / or by deprecating some of the existing less-used formats for configuration. | ||
|
|
||
| The `--set` syntax will change somewhat how variables are interpreted on the CLI (i.e. `--set VAR=100` will no longer represent `"100"` and instead will just be `100` internally). For `###` templates this can be mitigated by simply representing the value as a string for backwards compatibility though this will likely need to either be marked as breaking for Helm overrides or have additional flag changes / opt ins for this specific feature. | ||
|
|
There was a problem hiding this comment.
The biggest risk I'm not seeing here mentioned is the validation of the input. How can we ensure that injected data is reasonable, not leading to in-memory code execution, and matching the expected values in helm templates. I'm seeing this as the biggest risk, which will require the most logic to ensure we're not silently introducing bugs, or allowing overusing of the broad interface{} object.
There was a problem hiding this comment.
Added a risk section on this
|
|
||
| ##### Unit tests | ||
|
|
||
| Variable interfaces and libraries should be updated to ensure that interfaces are properly handled as opposed to strings. |
There was a problem hiding this comment.
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.
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
|
|
||
| ### Risks and Mitigations | ||
|
|
||
| This will require some additional processing of the `zarf-config` files to allow them to be processed properly because of Viper's long-running [case-insensitive keys issue](https://github.com/spf13/viper/issues/1014). This could be performed similar to UDS CLI's implementation of this feature and / or by deprecating some of the existing less-used formats for configuration. |
There was a problem hiding this comment.
If we're considering changing those, it would be nice to explicitly call out the possibility of deprecating and removing formats other than yaml.
1bc9701 to
8d641d0
Compare
Signed-off-by: Wayne Starr <me@racer159.com>
8d641d0 to
2e60973
Compare
soltysh
left a comment
There was a problem hiding this comment.
One minor nit, and make sure to sign all of your commits to satisfy DCO.
AustinAbro321
left a comment
There was a problem hiding this comment.
A few small comments, once those are resolved I'm ready to approve
|
|
||
| This will require some additional processing of the `zarf-config` files to allow them to be processed properly because of Viper's long-running [case-insensitive keys issue](https://github.com/spf13/viper/issues/1014). This could be performed similar to UDS CLI's implementation of this feature and / or by deprecating some of the existing less-used formats for configuration. (TODO: (@AustinAbro321) - link this to the other ZEP for viper reconsideration) | ||
|
|
||
| The `--set` syntax will change somewhat how variables are interpreted on the CLI (i.e. `--set VAR=100` will no longer represent `"100"` and instead will just be `100` internally). For `###` templates this can be mitigated by simply representing the value as a string for backwards compatibility though this will likely need to either be marked as breaking for Helm overrides or have additional flag changes / opt ins for this specific feature. |
There was a problem hiding this comment.
We should make the language here more clear. Call out that this will not be a breaking change for templates, but will be a breaking change for variable overrides. I don't think we should add flags
Co-authored-by: Brandt Keller <43887158+brandtkeller@users.noreply.github.com> Signed-off-by: Wayne Starr <me@racer159.com>
8c39077 to
c15675d
Compare
Signed-off-by: Wayne Starr <me@racer159.com>
c15675d to
34e8961
Compare
AustinAbro321
left a comment
There was a problem hiding this comment.
LGTM. P.S. we enabled contributor signing in the UI, so future users won't run into the DCO error ✔️
