-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[prometheus-ipmi-exporter] Optionally pull config from a Secret #4536
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nick Jones <nick@dischord.org>
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.
Thank you, @yankcrime, for your PR. Please, see my comments below.
@@ -1,4 +1,4 @@ | |||
{{- if not .Values.configMapFile -}} |
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.
The field configMapFile
does not occur elsewhere in the chart, I reckon its purpose was somehow not implemented and can therefore safely be removed.
@@ -1,4 +1,4 @@ | |||
{{- if not .Values.configMapFile -}} | |||
{{- if not .Values.useSecret -}} |
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'd suggest a differently named toggle (more below):
{{- if not .Values.useSecret -}} | |
{{- if not .Values.configSecret.enabled -}} |
{{- if .Values.useSecret }} | ||
secret: | ||
secretName: {{ .Values.secret.name }} | ||
items: | ||
- key: {{ .Values.secret.key }} | ||
path: config.yml | ||
{{- else }} |
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.
With respect to the comment above:
{{- if .Values.useSecret }} | |
secret: | |
secretName: {{ .Values.secret.name }} | |
items: | |
- key: {{ .Values.secret.key }} | |
path: config.yml | |
{{- else }} | |
{{- if .Values.configSecret.enabled }} | |
secret: | |
secretName: {{ .Values.configSecret.name }} | |
items: | |
- key: {{ .Values.configSecret.key }} | |
path: config.yml | |
{{- else }} |
useSecret: false | ||
secret: | ||
name: ipmi-secret | ||
key: config.yml |
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.
We should be a bit more specific of the secret. If using a map for a single feature, I'd suggest putting the toggle in the map.
useSecret: false | |
secret: | |
name: ipmi-secret | |
key: config.yml | |
configSecret: | |
enabled: false | |
name: ipmi-secret | |
key: config.yml |
What this PR does / why we need it
The way this currently works is that it builds a config file from values inlined via Helm, and this may need to include some sensitive information such as usernames and passwords that should be otherwise kept out of a repo in the cases where you're managing all of your cluster configuration via git.
This PR update the Chart to add an option to be able to reference a Secret instead.
Checklist
[prometheus-couchdb-exporter]
)