You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2022/05/10 10:30:50 UTC
[GitHub] [couchdb-helm] colearendt opened a new pull request, #73: allow uuid being created and managed by kubernetes
colearendt opened a new pull request, #73:
URL: https://github.com/apache/couchdb-helm/pull/73
<!--
Thank you for contributing to couchdb-helm. Before you submit this PR we'd like to
make sure you are aware of the chart technical requirements and best practices:
* https://github.com/helm/charts/blob/master/CONTRIBUTING.md#technical-requirements
* https://github.com/helm/helm/tree/master/docs/chart_best_practices
For a quick overview across what we will look at reviewing your PR, please read
our review guidelines:
* https://github.com/helm/charts/blob/master/REVIEW_GUIDELINES.md
Following our best practices right from the start will accelerate the review process and
help get your PR merged quicker.
When updates to your PR are requested, please add new commits and do not squash the
history. This will make it easier to identify new changes. The PR will be squashed
anyways when it is merged. Thanks.
Please make sure you test your changes before you push them.
-->
#### What this PR does / why we need it:
We utilize Helm's `lookup` command to store a generated `uuid` in an "internal" secret in Kubernetes. This allows generating the `uuid`, making it persistent, and notifying the user (in `NOTES.txt`) that this auto-generation happened. We also tell the user how to disable the message by making that value persistent in values.
#### Which issue this PR fixes
- fixes #39
#### Special notes for your reviewer:
Happy to discuss this approach, messaging, etc. We have found similar patterns to strike a decent balance when secrets / inputs must exist outside of the application.
(will walk through checklist once validated as a decent approach)
#### Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.
- [ ] Chart Version bumped
- [ ] e2e tests pass
- [ ] Variables are documented in the README.md
- [ ] Chart tgz added to /docs and index updated
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-helm] colearendt commented on a diff in pull request #73: allow uuid being created and managed by kubernetes
Posted by GitBox <gi...@apache.org>.
colearendt commented on code in PR #73:
URL: https://github.com/apache/couchdb-helm/pull/73#discussion_r1064934163
##########
couchdb/templates/secrets.yaml:
##########
@@ -18,4 +18,17 @@ data:
{{- if .Values.adminHash }}
password.ini: {{ tpl (.Files.Get "password.ini") . | b64enc }}
{{- end -}}
-{{- end -}}
+{{- end }}
+---
+apiVersion: v1
+kind: Secret
+metadata:
+ name: {{ template "couchdb.fullname" . }}-internal
+ labels:
+ app: {{ template "couchdb.fullname" . }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ release: "{{ .Release.Name }}"
+ heritage: "{{ .Release.Service }}"
+type: Opaque
+data:
+ uuid: {{- include "couchdb.uuid" . }}
Review Comment:
I'm happy to tie this into the existing secret if you want 😄 I was mostly isolating because it is being used a bit differently than the other secret - in a somewhat "idempotent" type of fashion where we look up values from it / etc.
It's also a bit tricky because whether you use `data:` or `stringData:` because it affects [whether we need to base64 encode the value](https://kubernetes.io/docs/concepts/configuration/secret/#basic-authentication-secret)... although - this actually looks like it might be a bug. I'll do some more sanity checking. It may need to be another secret if we want to pass `stringData` as the verbatim value. Or we can use the existing secret and `b64enc` the value first.
Your call on preference here!
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-helm] willholley commented on a diff in pull request #73: allow uuid being created and managed by kubernetes
Posted by GitBox <gi...@apache.org>.
willholley commented on code in PR #73:
URL: https://github.com/apache/couchdb-helm/pull/73#discussion_r1064481583
##########
couchdb/templates/secrets.yaml:
##########
@@ -18,4 +18,17 @@ data:
{{- if .Values.adminHash }}
password.ini: {{ tpl (.Files.Get "password.ini") . | b64enc }}
{{- end -}}
-{{- end -}}
+{{- end }}
+---
+apiVersion: v1
+kind: Secret
+metadata:
+ name: {{ template "couchdb.fullname" . }}-internal
+ labels:
+ app: {{ template "couchdb.fullname" . }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ release: "{{ .Release.Name }}"
+ heritage: "{{ .Release.Service }}"
+type: Opaque
+data:
+ uuid: {{- include "couchdb.uuid" . }}
Review Comment:
I'm not convinced about adding another secret resource here. The secret values `erlangCookie`, `cookieAuthSecret` are similarly internal only. It seems like a good clarification to split internal/external secrets but that could be a separate PR (and likely major version bump).
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-helm] colearendt commented on a diff in pull request #73: allow uuid being created and managed by kubernetes
Posted by "colearendt (via GitHub)" <gi...@apache.org>.
colearendt commented on code in PR #73:
URL: https://github.com/apache/couchdb-helm/pull/73#discussion_r1090463265
##########
couchdb/templates/secrets.yaml:
##########
@@ -18,4 +18,17 @@ data:
{{- if .Values.adminHash }}
password.ini: {{ tpl (.Files.Get "password.ini") . | b64enc }}
{{- end -}}
-{{- end -}}
+{{- end }}
+---
+apiVersion: v1
+kind: Secret
+metadata:
+ name: {{ template "couchdb.fullname" . }}-internal
+ labels:
+ app: {{ template "couchdb.fullname" . }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ release: "{{ .Release.Name }}"
+ heritage: "{{ .Release.Service }}"
+type: Opaque
+data:
+ uuid: {{- include "couchdb.uuid" . }}
Review Comment:
@willholley Thoughts on the right approach here? Would you prefer me to tie this into the existing secret?
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-helm] colearendt commented on a diff in pull request #73: allow uuid being created and managed by kubernetes
Posted by GitBox <gi...@apache.org>.
colearendt commented on code in PR #73:
URL: https://github.com/apache/couchdb-helm/pull/73#discussion_r1064934163
##########
couchdb/templates/secrets.yaml:
##########
@@ -18,4 +18,17 @@ data:
{{- if .Values.adminHash }}
password.ini: {{ tpl (.Files.Get "password.ini") . | b64enc }}
{{- end -}}
-{{- end -}}
+{{- end }}
+---
+apiVersion: v1
+kind: Secret
+metadata:
+ name: {{ template "couchdb.fullname" . }}-internal
+ labels:
+ app: {{ template "couchdb.fullname" . }}
+ chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
+ release: "{{ .Release.Name }}"
+ heritage: "{{ .Release.Service }}"
+type: Opaque
+data:
+ uuid: {{- include "couchdb.uuid" . }}
Review Comment:
I'm happy to tie this into the existing secret if you want 😄 I was mostly isolating because it is being used a bit differently than the other secret - in a somewhat "idempotent" type of fashion where we look up values from it / etc.
It's also a bit tricky whether you use `data:` or `stringData:` because it affects [whether we need to base64 encode the value](https://kubernetes.io/docs/concepts/configuration/secret/#basic-authentication-secret)... although - this actually looks like it might be a bug. I'll do some more sanity checking. It may need to be another secret if we want to pass `stringData` as the verbatim value. Or we can use the existing secret and `b64enc` the value first.
Your call on preference here!
--
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: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org