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

fix: Allow multiple imagePullSecrets to work #168

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

Conversation

mikebryant
Copy link

@mikebryant mikebryant commented Aug 7, 2024

Purpose

This fixes an issue where the extra indentation causes invalid yaml if 2+ imagePullSecrets are specified.

Also don't set them at all if not asked for - this fixes an issue for us where it conflicts with a kyverno ClusterPolicy that defaults our imagePullSecrets for everything, and the explicit setting of [] was causing a persistent diff in ArgoCD

Changes

Multiple image pull secrets will no longer cause a yaml error

Not setting imagepullsecrets will now template as not setting that parameter, rather than explicitly setting it to []

Testing

Tested locally:

➜  k8s-image-swapper git:(fix-image-pull-secrets) helm template . --show-only templates/serviceaccount.yaml --set 'imagePullSecrets[0].name=blah' --set 'imagePullSecrets[1].name=blah2'
---
# Source: k8s-image-swapper/templates/serviceaccount.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
  name: release-name-k8s-image-swapper
  labels:
    helm.sh/chart: k8s-image-swapper-1.10.3
    app.kubernetes.io/name: k8s-image-swapper
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/component: app
    app.kubernetes.io/version: "1.5.10"
    app.kubernetes.io/managed-by: Helm

imagePullSecrets:
  - name: blah
  - name: blah2


➜  k8s-image-swapper git:(fix-image-pull-secrets) helm template . --show-only templates/serviceaccount.yaml
---
# Source: k8s-image-swapper/templates/serviceaccount.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
  name: release-name-k8s-image-swapper
  labels:
    helm.sh/chart: k8s-image-swapper-1.10.3
    app.kubernetes.io/name: k8s-image-swapper
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/component: app
    app.kubernetes.io/version: "1.5.10"
    app.kubernetes.io/managed-by: Helm

Code Author Checklist

  • Bump the chart version (Chart.yaml -> version)
  • JSON Schema updated (values.schema.json)
  • Update README.md via helm-docs (or make prep)
  • Run pre-commit run --all-files via pre-commit (or make prep)
  • Update Artifacthub annotation (Chart.yaml -> artifacthub.io/changes, artifacthub.io/images)

@github-actions github-actions bot added the bugfix label Aug 7, 2024
@mikebryant mikebryant force-pushed the fix-image-pull-secrets branch from da3b367 to 2688563 Compare August 7, 2024 13:27
@github-actions github-actions bot added bugfix and removed bugfix labels Aug 7, 2024
@mikebryant mikebryant force-pushed the fix-image-pull-secrets branch from 2688563 to 0647b1e Compare August 7, 2024 13:28
@github-actions github-actions bot added bugfix and removed bugfix labels Aug 7, 2024
@mikebryant mikebryant force-pushed the fix-image-pull-secrets branch from 0647b1e to 83c3b42 Compare August 7, 2024 13:29
@mikebryant mikebryant marked this pull request as ready for review August 7, 2024 13:29
@github-actions github-actions bot added bugfix and removed bugfix labels Aug 7, 2024
@mikebryant
Copy link
Author

Heya @estahn, would you be able to take a look at this? :)

This fixes an issue where the extra indentation causes invalid yaml if 2+ imagePullSecrets are specified.

Also don't set them at all if not asked for - this fixes an issue for us where it conflicts with a kyverno ClusterPolicy that defaults our imagePullSecrets for everything, and the explicit setting of [] was causing a persistent diff in ArgoCD
@estahn estahn force-pushed the fix-image-pull-secrets branch from 83c3b42 to b19efe4 Compare October 1, 2024 17:32
@estahn
Copy link
Owner

estahn commented Oct 1, 2024

@mikebryant Looks fine. Could you fix up the helm-docs issue? Otherwise I'll do it :) ... I think if you allow commits from upstream CI should auto-fix it, but not sure.

@mikebryant
Copy link
Author

Hmm, I do have Allow edits by maintainers checked
I've updated the other chart version, since helm-docs is changing the version number in the README. Hopefully that fixes the checks?

@mikebryant
Copy link
Author

(Though if I've still got it wrong and you don't mind fixing it, I'd appreciate it 😄)

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

Successfully merging this pull request may close these issues.

2 participants