-
Notifications
You must be signed in to change notification settings - Fork 763
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
fix: Allow annotation templates for services that need them #1182
base: master
Are you sure you want to change the base?
Conversation
I understand your needs. I'm not sure of the correct way to fix it and it seems a really specific needs. The tpl has been introduced last year with #972 from @ivankatliarchuk . Don't you have a way to "print" template code with |
@mloiseleur - Point taken on this being a specific use case but I can see this potentially being a problem with any other service that does template interpolation with annotations which-- moving forward-- may be a lot more than just this one case. In other words, I'd be shocked if this doesn't break something else moving forward. As for solving it, the PR introduces a boolean to flip it off and defaults it to the default behavior of running The only other thing I'll mention is that we use this on a lot of our helm charts and traefik is the first chart I've run into that templates the values out-of-the-box. Chart templates on other popular repos like grafana and the nginx don't template out-of-the-box so, while I agree it is helpful and should be kept in, many other charts don't currently do this. |
@mloiseleur - Wanted to poke this again. Is the purposed PR not viable? Is there another suggested approach? |
@brow86 It's explained in the README: AFAIK, without any PR, one can set its template using a Configmap with Vault and agent injection. |
I guess the question then is whether the following changes takes this from being simple to complex:
This doesn't feel like a heavy change but do those changes break the philosophy? More importantly: could that same logic be used to argue that the original change to template annotations broke that mold? Looking at other popular helm chart annotations in the community, it does not look like the annotations use the template function. |
I understand your point. It's true it's not a big change. I added We will reevaluate as people respond. |
What does this PR do?
This fixes #1181 by doing the following:
templateAnnotations
value to default values file and keeps it astrue
to keep existing behaviorMotivation
This should help avoid issues with Hashicorp Vault and agent injection and any other service which uses templates as a part of its annotations.
More
make test
and all the tests passed