pingidentity / helm-charts

Apache License 2.0
22 stars 31 forks source link

Add repository fully-qualified name #265

Closed johnnyhuy closed 2 years ago

johnnyhuy commented 2 years ago

Hey Ping folk,

Hoping to add this change to allow us the ability to override this chart's default behaviour of splitting up the container image field with a fully-qualified image name.

This is a common pattern in Helm's helm create template chart:

values.yaml

image:
  repository: nginx
  pullPolicy: IfNotPresent
  tag: ""

templates/deployment.yaml

apiVersion: apps/v1
kind: Deployment
spec:
  template:
    spec:
      containers:
        - image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"

Instead of changing two values fields, we can do it with just one.

This also enhances deployment tooling that requires the full-qualified image name like Skaffold.

skaffold.yaml

deploy:
  helm:
    releases:
      - name: my-ping
        chartPath: charts/ping-devops
        artifactOverrides:
          pingfederate-engine.repositoryFqn: gcr.io/my-ping/my-pingfederate
        imageStrategy:
          helm: {}

However, I'm mindful of introducing a backwards-incompatible change. So I've added a conditional image.repositoryFqn value that overrides image.repository & image.name.

The alternative breaking change would be image.repository being the full-qualified image name.

Example

values.yaml

pingfederate-engine:
  enabled: true
  repositoryFqn: docker.io/johnnyhuy/pingfederate

ping-devops/templates/pingfederate-engine/workload.yaml

apiVersion: apps/v1
kind: Deployment
metadata:
  name: ping-devops-pingfederate-engine
spec:
...
      image: docker.io/johnnyhuy/pingfederate:2201
...
johnnyhuy commented 2 years ago

Cleaned up the docs on this change as well 🙏

wesleymccollam commented 2 years ago

@johnnyhuy, this is a great addition! We had to add these changes in our internal pipeline to pass smoke and integration tests, which were then pushed to this GitHub repo. These updates are in the latest release of the Ping Identity Helm chart.