Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

BenCoughlan15
Copy link

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.

  annotations:
    argocd.argoproj.io/sync-wave: "-1"

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 the values.yaml file.
  • helm/nessie/values.yaml: Introduced a new field configMapAnnotations to allow users to specify additional annotations for the ConfigMap.

@CLAassistant
Copy link

CLAassistant commented Mar 5, 2025

CLA assistant check
All committers have signed the CLA.

@dimas-b dimas-b requested a review from adutra March 5, 2025 20:10
Copy link
Member

@dimas-b dimas-b left a 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 !

Copy link
Contributor

@adutra adutra left a 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

Comment on lines +29 to +32
annotations:
{{- if .Values.configMapAnnotations }}
{{- toYaml .Values.configMapAnnotations | nindent 4 }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor suggestions:

  1. Put the if block start before annotations:
  2. Use tpl to templetize the input (I know we don't do this for configMapLabels line 27 – but maybe we should, in fact)
Suggested change
annotations:
{{- if .Values.configMapAnnotations }}
{{- toYaml .Values.configMapAnnotations | nindent 4 }}
{{- end }}
{{- if .Values.configMapAnnotations }}
annotations:
{{- tpl (toYaml .Values.configMapAnnotations) . | nindent 4 }}
{{- end }}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants