-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
helm: Add customizable global and per-container env vars to helm chart #13796
Changes from all commits
ad14738
ff644db
9fe3441
0ecea65
12c99e4
b61b607
71e7be6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense to me |
||
release: "{{ .Release.Name }}" | ||
component: "{{ .Values.middleManager.name }}" | ||
{{- end }} | ||
|
@@ -108,10 +108,16 @@ spec: | |
valueFrom: {fieldRef: {fieldPath: metadata.name}} | ||
- name: POD_NAMESPACE | ||
valueFrom: {fieldRef: {fieldPath: metadata.namespace}} | ||
{{- with .Values.env }} | ||
{{- toYaml . | nindent 8 }} | ||
{{- end }} | ||
{{- range $key, $val := .Values.middleManager.config }} | ||
- name: {{ $key }} | ||
value: {{ $val | quote }} | ||
{{- end}} | ||
{{- end }} | ||
{{- with .Values.middleManager.env }} | ||
{{- toYaml . | nindent 8 }} | ||
{{- end }} | ||
envFrom: | ||
- configMapRef: | ||
name: {{ template "druid.name" . }} | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I can see what you're saying. I'll make the change
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.
@jwitko could you make a commit for this? I agree with @georgew5656.