helm: Add customizable global and per-container env vars to helm chart #13796
Conversation
a2l007
added
the
Helm Chart
|
to understand, this allow for more than just name, value envVars, so you can do ValueFrom and grab things from secrets, or other places? |
Yes correct. It also allows you to direct the key/values inside configmaps/secrets to be populated into an envVar of your choice. In my example in this PR I'm taking the dynamically generated postgresDB secret which is created by a postgresDB operator and pushing it into the Druid accepted envVar for DB passwords. |
| {{- range $key, $val := .Values.broker.config }} | ||
| - name: {{ $key }} | ||
| value: {{ $val | quote }} | ||
| {{- end}} | ||
| {{- end }} | ||
| {{- with .Values.broker.env }} |
There was a problem hiding this comment.
can we call these values extraEnv or extraEnvVars? for additional env variables this seems to be more the format
https://github.com/codecentric/helm-charts/tree/master/charts/keycloak
https://artifacthub.io/packages/helm/bitnami/nginx
https://artifacthub.io/packages/helm/bitnami/mysql
There was a problem hiding this comment.
Since there is no other thing referencing environment variables I didn't see a need to get more complicated than env but if its what is needed to get this merged then sure, I'm not married to the name.
There was a problem hiding this comment.
i just prefer it this way to make it clear we aren't overwriting the entire environment variables section with the extra env variable, just appending
There was a problem hiding this comment.
Yea I can see what you're saying. I'll make the change
There was a problem hiding this comment.
@jwitko could you make a commit for this? I agree with @georgew5656.
| @@ -73,7 +73,7 @@ spec: | |||
| topologyKey: kubernetes.io/hostname | |||
| labelSelector: | |||
| matchLabels: | |||
| app: "{{ template "druid.name" . }}" | |||
| app: {{ template "druid.name" . | quote }} | |||
There was a problem hiding this comment.
is there a reason this field has to use the quote function instead of just quoting the value? it seems like most of the other fields in this chart use quotes, e.g.
"{{ .Release.Name }}" instead of {{ .Release.Name | quote }}
There was a problem hiding this comment.
quote can take a list and quote all the values but that almost certainly would never come into play here. I personally just think its cleaner.
|
When could this pr be merged ? @jwitko @georgew5656 |
|
@imcheck This PR has been ready to merge (from my perspective) since March. |
|
Who do i have to mention for merging this pr quickly? 😢 |
|
This pull request has been marked as stale due to 60 days of inactivity. |
|
Any updates? |
No unfortunately not. I've pinged in slack but I cannot get a response. The project seems to not be interested in these PRs. |
|
@jwitko Thanks for your contribution. As mentioned in PR #15904, we have migrated the Helm Chart to a new repository, you are welcome to raise up new PR there and we can maintain the Druid Helm Chart together in new repository. And this PR will closed soon. Thank you again for your understanding and collaboration. |
|
This pull request has been marked as stale due to 60 days of inactivity. |
Description
This PR adds customizable global and per-container environment variable appended array objects.
There already exists the ability to add basic key/value configurable variables via the global
configVarsand per-containerconfigmaps however the format they are implemented in does not support the broader Kubernetes environment variable use cases.For example if a person wanted to pass in
druid_metadata_storage_connector_passwordvia an existing Kubernetes secret this would not be possible in the existing config.This PR would allow for pulling of environment variables from secrets, configmaps, and any other K8s supported environment variable configuration on a global and per-container basis.
It should also be noted that the
Chart.yamlversion is bumped to0.3.6from0.3.3because #13747 and #13783 came first.Release note
Add customizable global and per-container env vars to helm chart
This PR has: