chore: add Zarf values guidelines for UDS Packages#679
Conversation
joelmccoy
left a comment
joelmccoy
left a comment
There was a problem hiding this comment.
Generally on board with the guidance here! Just two small questions
|
|
||
| ## Implementation | ||
|
|
||
| The top-level `values` key for a UDS Package should be defined in the parent `zarf.yaml` of the UDS Package repository and should reference a `zarf-default-values.yaml` file and a `zarf-default-values.schema.json` file located in the root of the repository to handle defaults and validation respectively. |
There was a problem hiding this comment.
Its been a while since reviewing the zarf values proposal/implementation details so bear with me... Default values as in shipped defaults (analogous to bundle variables with defaults set in the bundle.yaml) or just an example, not included in the packaged artifact (analogous variable override definition in a uds-config.yaml with a bundle.yaml with variable overrides and no defaults)?
If the latter this seems reasonable. If the former i would be concerned about who they are actually good defaults for since they are getting baked in. In that case i would want something more like named sets of defaults that are included and could be referenced/applied.
There was a problem hiding this comment.
they are defaults like a Helm chart values default - if you'd like that we could discuss in Zarf as an issue with more context / examples of what those named defaults would be (right now they compose together into a single file).
There was a problem hiding this comment.
I'm generally concerned about prescribing default values as a whole separate thing that needs to be defined. In a lot of cases i dont think we want to be setting default zarf values for exposed values other than what is already the default in the underlying chart (or zarf component helm values) file. Requiring the default values to be filled out seems like adding an additional layer which a bunch of duplication that can potentially lead to drift of the intended config and unintended consequences.
Furthermore, i think the schema needs to be able to be generated and there is already an issue in zarf for that, which is great, but if it has to be derived from the default values file that is problematic for the same reasons i think having to define all of the defaults if we are exposing is problematic.
I have a separate concern about how we replace zarf variables that are being used to build helm values. For example, in the GitLab package we build s3 bucket names from a prefix and suffix var:
lfs:
bucket: ###ZARF_VAR_BUCKET_PREFIX###gitlab-lfs###ZARF_VAR_BUCKET_SUFFIX###
artifacts:
bucket: ###ZARF_VAR_BUCKET_PREFIX###gitlab-artifacts###ZARF_VAR_BUCKET_SUFFIX###
uploads:
bucket: ###ZARF_VAR_BUCKET_PREFIX###gitlab-uploads###ZARF_VAR_BUCKET_SUFFIX###
This doesn't play well with the zarf values feature as i understand it (happy to be wrong) and this is definitely not the only place patterns like this are used. Another that comes to mind is building connection strings.
briantwatson
left a comment
briantwatson
left a comment
There was a problem hiding this comment.
A couple small comments on phrasing.
Co-authored-by: Brian Watson <brianwatson@defenseunicorns.com>
Co-authored-by: Brian Watson <brianwatson@defenseunicorns.com>
4 participants
Description
This adds Zarf values guidance and wiring for implementing this feature within UDS Packages going forward.
Checklist before merging