-
Notifications
You must be signed in to change notification settings - Fork 147
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: Allow annotations on the configmap. #10510
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for your contribution, @BenCoughlan15 !
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.
LGTM with some minor remarks.
Also could you please create a new test file configmap_test.yaml
and add tests like the ones below:
release:
name: nessie
namespace: nessie-ns
templates:
- configmap.yaml
tests:
- it: should not create annotations by default
asserts:
- notExists:
path: metadata.annotations
- it: should create annotations if defined
set:
configMapAnnotations:
foo: bar
asserts:
- equal:
path: metadata.annotations
value:
foo: bar
- it: should create annotations with templates
set:
configMapAnnotations:
foo: configmap-{{ .Release.Name }}-{{ .Release.Namespace }}
asserts:
- equal:
path: metadata.annotations
value:
foo: configmap-nessie-nessie-ns
annotations: | ||
{{- if .Values.configMapAnnotations }} | ||
{{- toYaml .Values.configMapAnnotations | nindent 4 }} | ||
{{- end }} |
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.
Two minor suggestions:
- Put the if block start before
annotations:
- Use
tpl
to templetize the input (I know we don't do this forconfigMapLabels
line 27 – but maybe we should, in fact)
annotations: | |
{{- if .Values.configMapAnnotations }} | |
{{- toYaml .Values.configMapAnnotations | nindent 4 }} | |
{{- end }} | |
{{- if .Values.configMapAnnotations }} | |
annotations: | |
{{- tpl (toYaml .Values.configMapAnnotations) . | nindent 4 }} | |
{{- end }} |
This pull request introduces enhancements to the Helm chart for Nessie by adding support for custom annotations in the ConfigMap. The changes ensure that users can define additional annotations through the
values.yaml
file, which will be applied to the ConfigMap.This would allow us to set the argocd sync-wave, which ensures the CM syncs first, leading to a more stable Nessie deployment.
i.e.
There may of course, be other use cases.
Key changes include:
helm/nessie/templates/configmap.yaml
: Added support for custom annotations by including a conditional block to insert annotations if they are defined in thevalues.yaml
file.helm/nessie/values.yaml
: Introduced a new fieldconfigMapAnnotations
to allow users to specify additional annotations for the ConfigMap.