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

[bitnami/common] Add support for customizing standard labels #18154

Merged
merged 7 commits into from
Aug 22, 2023

Conversation

juan131
Copy link
Contributor

@juan131 juan131 commented Aug 3, 2023

Description of the change

This PR makes it possible to overwrite the standard labels below that are currently set with predefined values based on chart name, chart release name, etc.

  • app.kubernetes.io/name
  • helm.sh/chart
  • app.kubernetes.io/managed-by
  • app.kubernetes.io/instance

Benefits

Users can set any desired custom value to the labels mentioned above.

Possible drawbacks

We'll have to update every single chart right away to make them compatible with new common.labels.standard and common.labels.matchLabels helpers' format.

Applicable issues

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

Additional information

Find below an example for adopting the new helper format on Apache:

diff --git a/bitnami/apache/templates/deployment.yaml b/bitnami/apache/templates/deployment.yaml
index 7fc270be8..525401805 100644
--- a/bitnami/apache/templates/deployment.yaml
+++ b/bitnami/apache/templates/deployment.yaml
@@ -8,16 +8,14 @@ kind: Deployment
 metadata:
   name: {{ include "common.names.fullname" . }}
   namespace: {{ .Release.Namespace | quote }}
-  labels: {{- include "common.labels.standard" . | nindent 4 }}
-    {{- if .Values.commonLabels }}
-    {{- include "common.tplvalues.render" ( dict "value" .Values.commonLabels "context" $ ) | nindent 4 }}
-    {{- end }}
+  labels: {{- include "common.labels.standard" ( dict "customLabels" .Values.commonLabels "context" $ ) | nindent 4 }}
   {{- if .Values.commonAnnotations }}
   annotations: {{- include "common.tplvalues.render" ( dict "value" .Values.commonAnnotations "context" $ ) | nindent 4 }}
   {{- end }}
 spec:
+  {{- $podLabels := merge .Values.podLabels .Values.commonLabels }}
   selector:
-    matchLabels: {{- include "common.labels.matchLabels" . | nindent 6 }}
+    matchLabels: {{- include "common.labels.matchLabels" ( dict "customLabels" $podLabels "context" $ ) | nindent 6 }}
   replicas: {{ .Values.replicaCount }}
   revisionHistoryLimit: {{ .Values.revisionHistoryLimit }}
   {{- if .Values.updateStrategy }}
@@ -25,10 +23,7 @@ spec:
   {{- end }}
   template:
     metadata:
-      labels: {{- include "common.labels.standard" . | nindent 8 }}
-        {{- if .Values.podLabels }}
-        {{- include "common.tplvalues.render" (dict "value" .Values.podLabels "context" $) | nindent 8 }}
-        {{- end }}
+      labels: {{- include "common.labels.standard" ( dict "customLabels" $podLabels "context" $ ) | nindent 8 }}
       {{- if or .Values.podAnnotations (and .Values.metrics.enabled .Values.metrics.podAnnotations) }}
       annotations:
         {{- if .Values.podAnnotations }}
@@ -53,8 +48,8 @@ spec:
       affinity: {{- include "common.tplvalues.render" (dict "value" .Values.affinity "context" $) | nindent 8 }}
       {{- else }}
       affinity:
-        podAffinity: {{- include "common.affinities.pods" (dict "type" .Values.podAffinityPreset "context" $) | nindent 10 }}
-        podAntiAffinity: {{- include "common.affinities.pods" (dict "type" .Values.podAntiAffinityPreset "context" $) | nindent 10 }}
+        podAffinity: {{- include "common.affinities.pods" (dict "type" .Values.podAffinityPreset "customLabels" $podLabels "context" $) | nindent 10 }}
+        podAntiAffinity: {{- include "common.affinities.pods" (dict "type" .Values.podAntiAffinityPreset "customLabels" $podLabels "context" $) | nindent 10 }}
         nodeAffinity: {{- include "common.affinities.nodes" (dict "type" .Values.nodeAffinityPreset.type "key" .Values.nodeAffinityPreset.key "values" .Values.nodeAffinityPreset.values) | nindent 10 }}
       {{- end }}
       {{- if .Values.podSecurityContext.enabled }}

Using the changes mentioned above we can customize labels as shown in the example below:

$ helm template apache bitnami/apache \
  --set 'commonLabels.app\.kubernetes\.io/instance=MainApache' \
  --set 'commonLabels.app\.kubernetes\.io/managed-by=JuanAriza' \
  --set 'commonLabels.foo=bar' \
  --set 'podLabels.foo=baz' \
  -s templates/deployment.yaml
---
# Source: apache/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  (...)
  labels:
    app.kubernetes.io/name: apache
    helm.sh/chart: apache-10.0.2
    app.kubernetes.io/instance: MainApache
    app.kubernetes.io/managed-by: JuanAriza
    foo: bar
spec:
  selector:
    matchLabels:
      app.kubernetes.io/name: apache
      app.kubernetes.io/instance: MainApache
      foo: baz
  (...)
  template:
    metadata:
      labels:
        app.kubernetes.io/name: apache
        helm.sh/chart: apache-10.0.2
        app.kubernetes.io/managed-by: Helm
        app.kubernetes.io/instance: MainApache
        foo: baz
    spec:
      (...)
      affinity:
        podAntiAffinity:
          preferredDuringSchedulingIgnoredDuringExecution:
            - podAffinityTerm:
                labelSelector:
                  matchLabels:
                    app.kubernetes.io/name: apache
                    app.kubernetes.io/instance: MainApache
                    foo: baz
                topologyKey: kubernetes.io/hostname
              weight: 1
  (...)

@github-actions github-actions bot added the common label Aug 3, 2023
@bitnami-bot bitnami-bot added the verify Execute verification workflow for these changes label Aug 3, 2023
bitnami/common/Chart.yaml Outdated Show resolved Hide resolved
bitnami/common/templates/_labels.tpl Outdated Show resolved Hide resolved
bitnami/common/templates/_labels.tpl Outdated Show resolved Hide resolved
Copy link
Contributor

@lindhe lindhe left a comment

Choose a reason for hiding this comment

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

The code as such LGTM, as far as I can tell (just that minor typo in a comment that we should fix).

But I'd like to try this out first before approving, to confirm that it behaves as I expect.

bitnami/common/templates/_labels.tpl Outdated Show resolved Hide resolved
@lindhe
Copy link
Contributor

lindhe commented Aug 3, 2023

Everything seems to work as expected when I tried it out! 👍 Good work!

@github-actions github-actions bot added the triage Triage is needed label Aug 3, 2023
@github-actions github-actions bot removed the triage Triage is needed label Aug 4, 2023
dgomezleon
dgomezleon previously approved these changes Aug 4, 2023
Copy link
Member

@dgomezleon dgomezleon left a comment

Choose a reason for hiding this comment

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

LGTM

@juan131
Copy link
Contributor Author

juan131 commented Aug 25, 2023

Hi @jouve @Qjammer

Is this sth you can fix by upgrading your Helm client version? I'm unable to reproduce it with Helm v3.12.2

@Qjammer
Copy link

Qjammer commented Aug 25, 2023

We're in the process of upgrading our helm version 👍 . I will report back if the issue remains after the upgrade. Otherwise, I consider this case closed for me.

@jouve
Copy link
Contributor

jouve commented Aug 25, 2023

@juan131 I tried to reproduce @Qjammer with helm 3.5 because the prerequites in the README.md of this repo state that helm 3.2+ is supported:

    Kubernetes 1.19+
    Helm 3.2.0+

As I said in my comment , go > 1.16 should fix the issue, and helm > 3.6 are built with such versions of go.

@jouve
Copy link
Contributor

jouve commented Aug 25, 2023

I guess the multiline pipeline could be rewriten like :

{{- define "common.labels.standard" -}}
{{- if and (hasKey . "customLabels") (hasKey . "context") -}}
{{- $default := (dict) -}}
{{- $_ := set $default "app.kubernetes.io/name" (include "common.names.name" .context) -}}
{{- $_ = set $default "helm.sh/chart" (include "common.names.chart" .context) -}}
{{- $_ = set $default "app.kubernetes.io/instance" .context.Release.Name -}}
{{- $_ = set $default "app.kubernetes.io/managed-by" .context.Release.Service -}}
{{ merge (include "common.tplvalues.render" (dict "value" .customLabels "context" .context) | fromYaml) $default | toYaml }}
{{- else -}}
...

@carrodher carrodher assigned juan131 and unassigned dgomezleon Sep 3, 2023
@github-actions github-actions bot added in-progress and removed triage Triage is needed labels Sep 3, 2023
@bitnami-bot bitnami-bot removed the request for review from dgomezleon September 3, 2023 09:08
@carrodher carrodher removed the request for review from fevisera September 3, 2023 09:08
@carrodher carrodher assigned juan131 and unassigned fevisera Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitnami common solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to override managed-by label via commonLabels
8 participants