You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "jwitko (via GitHub)" <gi...@apache.org> on 2023/02/13 19:01:09 UTC
[GitHub] [druid] jwitko opened a new pull request, #13796: helm: Add customizable global and per-container env vars to helm chart
jwitko opened a new pull request, #13796:
URL: https://github.com/apache/druid/pull/13796
### 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 `configVars` and per-container `config` maps 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_password` via 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.yaml` version is bumped to `0.3.6` from `0.3.3` because #13747 and #13783 came first.
#### Release note
Add customizable global and per-container env vars to helm chart
<hr>
This PR has:
- [X] been self-reviewed.
- [X] added documentation for new or modified features or behaviors.
- [X] a release note entry in the PR description.
- [X] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [X] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [X] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] georgew5656 commented on a diff in pull request #13796: helm: Add customizable global and per-container env vars to helm chart
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #13796:
URL: https://github.com/apache/druid/pull/13796#discussion_r1137160686
##########
helm/druid/templates/middleManager/statefulset.yaml:
##########
@@ -73,7 +73,7 @@ spec:
topologyKey: kubernetes.io/hostname
labelSelector:
matchLabels:
- app: "{{ template "druid.name" . }}"
+ app: {{ template "druid.name" . | quote }}
Review 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 }}
##########
helm/druid/templates/broker/deployment.yaml:
##########
@@ -65,10 +65,16 @@ spec:
valueFrom: {fieldRef: {fieldPath: metadata.name}}
- name: POD_NAMESPACE
valueFrom: {fieldRef: {fieldPath: metadata.namespace}}
+ {{- with .Values.env }}
+ {{- toYaml . | nindent 10 }}
+ {{- end }}
{{- range $key, $val := .Values.broker.config }}
- name: {{ $key }}
value: {{ $val | quote }}
- {{- end}}
+ {{- end }}
+ {{- with .Values.broker.env }}
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jwitko commented on a diff in pull request #13796: helm: Add customizable global and per-container env vars to helm chart
Posted by "jwitko (via GitHub)" <gi...@apache.org>.
jwitko commented on code in PR #13796:
URL: https://github.com/apache/druid/pull/13796#discussion_r1137579243
##########
helm/druid/templates/broker/deployment.yaml:
##########
@@ -65,10 +65,16 @@ spec:
valueFrom: {fieldRef: {fieldPath: metadata.name}}
- name: POD_NAMESPACE
valueFrom: {fieldRef: {fieldPath: metadata.namespace}}
+ {{- with .Values.env }}
+ {{- toYaml . | nindent 10 }}
+ {{- end }}
{{- range $key, $val := .Values.broker.config }}
- name: {{ $key }}
value: {{ $val | quote }}
- {{- end}}
+ {{- end }}
+ {{- with .Values.broker.env }}
Review Comment:
Yea I can see what you're saying. I'll make the change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jwitko commented on a diff in pull request #13796: helm: Add customizable global and per-container env vars to helm chart
Posted by "jwitko (via GitHub)" <gi...@apache.org>.
jwitko commented on code in PR #13796:
URL: https://github.com/apache/druid/pull/13796#discussion_r1137209117
##########
helm/druid/templates/broker/deployment.yaml:
##########
@@ -65,10 +65,16 @@ spec:
valueFrom: {fieldRef: {fieldPath: metadata.name}}
- name: POD_NAMESPACE
valueFrom: {fieldRef: {fieldPath: metadata.namespace}}
+ {{- with .Values.env }}
+ {{- toYaml . | nindent 10 }}
+ {{- end }}
{{- range $key, $val := .Values.broker.config }}
- name: {{ $key }}
value: {{ $val | quote }}
- {{- end}}
+ {{- end }}
+ {{- with .Values.broker.env }}
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] churromorales commented on pull request #13796: helm: Add customizable global and per-container env vars to helm chart
Posted by "churromorales (via GitHub)" <gi...@apache.org>.
churromorales commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-1430310884
to understand, this allow for more than just name, value envVars, so you can do ValueFrom and grab things from secrets, or other places?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "imcheck (via GitHub)" <gi...@apache.org>.
imcheck commented on code in PR #13796:
URL: https://github.com/apache/druid/pull/13796#discussion_r1301472147
##########
helm/druid/templates/broker/deployment.yaml:
##########
@@ -65,10 +65,16 @@ spec:
valueFrom: {fieldRef: {fieldPath: metadata.name}}
- name: POD_NAMESPACE
valueFrom: {fieldRef: {fieldPath: metadata.namespace}}
+ {{- with .Values.env }}
+ {{- toYaml . | nindent 10 }}
+ {{- end }}
{{- range $key, $val := .Values.broker.config }}
- name: {{ $key }}
value: {{ $val | quote }}
- {{- end}}
+ {{- end }}
+ {{- with .Values.broker.env }}
Review Comment:
@jwitko could you make a commit for this? I agree with @georgew5656.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-2068280859
This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "jwitko (via GitHub)" <gi...@apache.org>.
jwitko closed pull request #13796: helm: Add customizable global and per-container env vars to helm chart
URL: https://github.com/apache/druid/pull/13796
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jwitko commented on pull request #13796: helm: Add customizable global and per-container env vars to helm chart
Posted by "jwitko (via GitHub)" <gi...@apache.org>.
jwitko commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-1430322198
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "asdf2014 (via GitHub)" <gi...@apache.org>.
asdf2014 commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-1956185762
@jwitko Thanks for your contribution. As mentioned in PR [https://github.com/apache/druid/pull/15904](https://github.com/apache/druid/pull/15904), we have migrated the Helm Chart to a [new repository](https://github.com/asdf2014/druid-helm), 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "imcheck (via GitHub)" <gi...@apache.org>.
imcheck commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-1685566914
When could this pr be merged ? @jwitko @georgew5656
I made an issue related this (https://github.com/apache/druid/issues/14867) and this pr will be resolved it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-1928548057
This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jwitko commented on a diff in pull request #13796: helm: Add customizable global and per-container env vars to helm chart
Posted by "jwitko (via GitHub)" <gi...@apache.org>.
jwitko commented on code in PR #13796:
URL: https://github.com/apache/druid/pull/13796#discussion_r1137207649
##########
helm/druid/templates/middleManager/statefulset.yaml:
##########
@@ -73,7 +73,7 @@ spec:
topologyKey: kubernetes.io/hostname
labelSelector:
matchLabels:
- app: "{{ template "druid.name" . }}"
+ app: {{ template "druid.name" . | quote }}
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "jwitko (via GitHub)" <gi...@apache.org>.
jwitko commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-1686479458
@imcheck This PR has been ready to merge (from my perspective) since March.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "jwitko (via GitHub)" <gi...@apache.org>.
jwitko commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-1929896178
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] georgew5656 commented on a diff in pull request #13796: helm: Add customizable global and per-container env vars to helm chart
Posted by "georgew5656 (via GitHub)" <gi...@apache.org>.
georgew5656 commented on code in PR #13796:
URL: https://github.com/apache/druid/pull/13796#discussion_r1137359611
##########
helm/druid/templates/broker/deployment.yaml:
##########
@@ -65,10 +65,16 @@ spec:
valueFrom: {fieldRef: {fieldPath: metadata.name}}
- name: POD_NAMESPACE
valueFrom: {fieldRef: {fieldPath: metadata.namespace}}
+ {{- with .Values.env }}
+ {{- toYaml . | nindent 10 }}
+ {{- end }}
{{- range $key, $val := .Values.broker.config }}
- name: {{ $key }}
value: {{ $val | quote }}
- {{- end}}
+ {{- end }}
+ {{- with .Values.broker.env }}
Review 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
##########
helm/druid/templates/middleManager/statefulset.yaml:
##########
@@ -73,7 +73,7 @@ spec:
topologyKey: kubernetes.io/hostname
labelSelector:
matchLabels:
- app: "{{ template "druid.name" . }}"
+ app: {{ template "druid.name" . | quote }}
Review Comment:
makes sense to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "imcheck (via GitHub)" <gi...@apache.org>.
imcheck commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-1687382196
Who do i have to mention for merging this pr quickly? 😢
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] helm: Add customizable global and per-container env vars to helm chart (druid)
Posted by "lens0021 (via GitHub)" <gi...@apache.org>.
lens0021 commented on PR #13796:
URL: https://github.com/apache/druid/pull/13796#issuecomment-1928661677
Any updates?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org