ZEP-0015: Helm Value Style Variable Interfaces #16
Conversation
Racer159
commented
Feb 3, 2025
Racer159
commented
- 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>
Racer159
force-pushed
the
0015-helm-value-style-variable-interfaces
branch
from
a871ff9 to
3ee8cfd
Compare
|
|
||
| ### 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.
Racer159
force-pushed
the
0015-helm-value-style-variable-interfaces
branch
from
1bc9701 to
8d641d0
Compare
Signed-off-by: Wayne Starr <me@racer159.com>
Racer159
force-pushed
the
0015-helm-value-style-variable-interfaces
branch
from
8d641d0 to
2e60973
Compare
Co-authored-by: Brandt Keller <43887158+brandtkeller@users.noreply.github.com>
| - the first Zarf release where an initial version of the ZEP was available | ||
| - the version of Zarf where the ZEP graduated to general availability | ||
| - when the ZEP was retired or superseded | ||
| --> |
There was a problem hiding this comment.
Nit: missing history, something as simple as:
2025-02-03: Initial version of this document.
|
|
||
| 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
|
|
||
| ### 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. (TODO: (@AustinAbro321) - link this to the other ZEP for viper reconsideration) |
There was a problem hiding this comment.
FYI we're not going to create a ZEP to evaluate viper at this time since there are higher priorities, but we are tentatively planning to deprecate and remove all the config file options besides yaml and toml.
