✨ (helm/v2-alpha) - dynamic value templating#5488
✨ (helm/v2-alpha) - dynamic value templating#5488asergeant01 wants to merge 4 commits intokubernetes-sigs:masterfrom
Conversation
|
Hi @asergeant01. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asergeant01 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| {{- toYaml .Values.manager.env | nindent 20 }} | ||
| {{- else }} | ||
| [] | ||
| {{- end }} |
There was a problem hiding this comment.
Thanks so much for looking into this 🙌
One important point: values should not be overridden unless we explicitly use --force, so users can keep their customizations — and it sounds like that expectation is still respected here (which is great).
Just to make sure I understand, the workflow with this proposal would be:
- Run
kubebuilder edit --plugins=helm/v2-alphato generate the chart - Edit
values.yamlto expose/customize something - Re-run
kubebuilder edit --plugins=helm/v2-alphato update the chart according to what’s invalues.yaml
That’s an interesting idea. I’ll need some time to play around with it, but a couple potential drawbacks come to mind:
Concerns
-
Expose everything?
This creates a big surface area. It’s unclear whether we want the default experience to be “highly configurable” or more “opinionated/curated”. -
Supportability
Without a curated list of “first-class” options, documentation and support get harder (and it becomes harder to test the chart against a well-defined set of supported knobs). -
Fragile injection
The injection appears to rely on string/regex manipulation of YAML. If the manager manifest shape changes, we might inject in the wrong place or miss entirely. There aren’t strong structural guarantees. -
Special characters
Keys likecustom.io/fooget escaped in the values path (custom_io_foo). That can be surprising and may not match user expectations. -
Labels/annotations scope
It should be very clear whether these values are applied only to the manager (Deployment/pod template) vs globally across resources. Applying them globally by default could be surprising.
Overall, I’m not sure yet this is the right direction as-is — we may want to go step-by-step and only expose what clearly makes sense to be configurable, maintain a well-defined/accurate list of supported keys, and aim for good practices + maintainability.
I will need some time to think about and play around.
camilamacedo86
left a comment
There was a problem hiding this comment.
Hi @asergeant01,
Right now we expose the values that already make sense for end users.
I think the proposal is an interesting idea, but it’s brittle and difficult to support long-term. It could also encourage exposing options that don’t make sense for chart consumers. I’m not sure that’s the right direction.
Instead, I’d prefer we discuss each additional item we want to expose on a case-by-case basis. Project authors should be able to customize the config, and that should flow into the chart—but the chart values should remain focused on knobs that are reasonable for end consumers to change.
For clarity, we already expose the install-time customization values under manager (extracted from Kustomize when present and surfaced in values.yaml, overridable via --set or a values file), plus these chart-level manager-related toggles:
- Manager deployment (
manager.*): replicas, image repository/tag/pullPolicy, args, env, imagePullSecrets, podSecurityContext, securityContext, resources, affinity, nodeSelector, tolerations - Chart-level (manager-related):
metrics.enable,metrics.port,webhook.enable,webhook.port
So beyond #5496, what else do we think we should expose? If there are specific additional values you’d like to see, could we track them in a separate issue (or issues) so we can evaluate them individually?
Thanks,
Camila
|
@camilamacedo86 Yes, that's no problem. I actually prefer this more defined approach. it was purely from your comment...
that I thought you wanted something more dynamic. I think then I can close this PR and open issues for the remaining values I think should be configurable. |
Summary
Enhances the Helm v2-alpha plugin to automatically detect custom values added to
values.yamland inject corresponding Helm template syntax when regenerating charts withkubebuilder edit --plugins=helm/v2-alpha.Enhancement #5485