-
Notifications
You must be signed in to change notification settings - Fork 505
[helm] expand extraObjects templating
#3022
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
base: main
Are you sure you want to change the base?
Conversation
|
Also eager for this to be merged. YAML merging is very useful and keeps our env-specific values files small. |
Helm throws a fit when the object type changes during coalesce
ptodev
left a 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.
Thank you for the PR! I'm not very familiar with Helm... I suppose this makes sense but it'd be nice if @petewall could sanity check it before merging 😆
| {{ end }} | ||
| {{- if kindIs "map" . }} | ||
| {{- tpl (toYaml .) $ | nindent 0 }} | ||
| {{- else if kindIs "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.
Is it possible that it's neither a map nor a string? E.g. could it contain numbers?
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.
It could, and we could template that, but it would yield invalid kube yaml so ultimately would be rejected. The user does need to define multiple fields, at least a minimum to render valid yaml. As far as I can think, these are the only two object types that would allow defining a complete object.
|
I think it's fine. I'd love to see some more examples in the values.yaml comments, especially with the whole "if its a map, the keys are thrown away". That's non-obvious to me, and we should be very careful adding things that might contribute to confusion. |
Co-authored-by: Paulin Todev <paulin.todev@gmail.com>
Co-authored-by: Paulin Todev <paulin.todev@gmail.com>
|
I use this template internally on one of our charts and recognizing it's variability is quite wide and complex, I wrote out a helm-unittest case for it. Happy to share it here. Note my chart defines the field as |
|
I added another example of using list, but I see your point about the confusion of using dict. Do you have any thoughts on how that could be better-communicated without making the examples doubles the length? |
|
|
|
@ptodev do you need anything from me to keep this moving forward? |
PR Description
Expand
extraObjectsto be far more flexible to the consumer of the chart. Couple top of mind use casesextraObjectsas a map, we can now split it across different values files, including encrypted ones.{{ tpl (toYaml .) $ }}did not allow templating ofkeynames (referencing key, value pairs). The most common case where someone would want to do this is to when defining a new object that imports the standard labels{{ include "alloy.labels" . }}or{{ include "alloy.selectorLabels" . }}.Which issue(s) this PR fixes
None
Notes to the Reviewer
I've tested this pretty extensively in other projects. I looked at seeing what I could do for testing here, but to do any kind of adequate testing in this scenario, requires more than simple "does it template" tests IMO. I have a helm-unittests for this that I've written which I'm happy to share. Note that it uses
.Values.manifestsand not.Values.extraObjects.https://gist.github.com/TheRealNoob/ca7dee0e862a4e5526a115d54e863ec9
PR Checklist