make deployment.image.tag and job.image.tag optional #234

slauger · 2023-03-24T17:49:50Z

Hi,

i really like the idea of creating a generic chart which can be used for many use cases, but i have a small issue with the Chart.

Currently you use the the following code block for the image definition:

        {{- if .Values.deployment.image.digest }}
        image: "{{ .Values.deployment.image.repository }}@{{ .Values.deployment.image.digest }}"
        {{- else if .Values.deployment.image.tag }}
        image: "{{ .Values.deployment.image.repository }}:{{ .Values.deployment.image.tag }}"
        {{- end }}

This means that currently deployment.image.tag is a required param. The current default value is v1.0.0. This is a bit odd, because a image definition is also valid w/o a tag. For example, the following definitions are also valid and not all of them are currently supported by the chart:

quay.io/xyz/foo:v3.0.5
quay.io/xyz/baz:latest
quay.io/xyz/bar@sha256:686d8c9dfa6f3ccfc8230bc3178d23f84eeaf7e457f36f271ab1acc53015037c
quay.io/xyz/foo:latest@sha256:686d8c9dfa6f3ccfc8230bc3178d23f84eeaf7e457f36f271ab1acc53015037c

Also it's very common that you receive the image as complete "all-in-one" string from a CI/CD pipeline. Extracting just the digest or tag and hand it over to helm could be very frustrating and complex sometimes. So it would be great if you just set the image with the tag and digest as a single parameter.

Attached you will find a pull request which makes deployment.image.tag (and job.image.tag) optional and set the default value to null.

Let me know what you think about this change.

github-actions · 2023-03-24T17:53:01Z

@d3adb5 validation successful`

rasheedamir · 2023-03-24T18:22:03Z

@slauger please update both readme and changelog! thanks for contribution

slauger · 2023-03-24T20:04:30Z

@rasheedamir changelog and readme is updated

d3adb5

Probably completely out of line since I'm not a repository maintainer, but I thought I'd leave a review anyway. Let me know your thoughts, everyone.

application/values.yaml

d3adb5 · 2023-03-26T21:15:26Z

application/templates/cronjob.yaml

+            {{- if $job.image.tag }}
            {{- if $job.image.digest }}
-            image: "{{ $job.image.repository }}@{{ $job.image.digest }}"
-            {{- else if $job.image.tag }}
+            image: "{{ $job.image.repository }}:{{ $job.image.tag }}@{{ $job.image.digest }}"
+            {{- else }}
            image: "{{ $job.image.repository }}:{{ $job.image.tag }}"
            {{- end }}
+            {{- else if $job.image.digest }}
+            image: "{{ $job.image.repository }}@{{ $job.image.digest }}"
+            {{- else }}
+            image: "{{ $job.image.repository }}"
+            {{- end }}


To avoid repetition, here's a suggestion:

{{- $image := $job.image.repository }} {{- with $job.image.tag }}{{ $image = print $image ":" . }}{{ end }} {{- with $job.image.digest }}{{ $image = print $image "@" . }}{{ end }} image: {{ $image }}

I do believe, however, that if an image repository isn't specified, we should have a templating error, for containers can't be without an image:

{{- $image := $job.image.repository }} {{- if not $image }}{{ print "Undefined image at CronJob definition at: " $name | fail }}{{ end }} {{- with $job.image.tag }}{{ $image = print $image ":" . }}{{ end }} {{- with $job.image.digest }}{{ $image = print $image "@" . }}{{ end }} image: {{ $image }}

If we go this route, there should be a unit test for it!

It would be possible to remove the default for image.repository via

{{ required "A valid value for .Values.repository.image is required!" .Values.image.repository }}

but since the default was already set to repository/image-name i did not change this behaviour.

Looks like generic default values like those could be refactored out to allow for these safeguards to be put in place. That's probably outside the scope of this pull request, however.

d3adb5 · 2023-03-26T21:16:05Z

application/templates/deployment.yaml

+        {{- if .Values.deployment.image.tag }}
        {{- if .Values.deployment.image.digest }}
-        image: "{{ .Values.deployment.image.repository }}@{{ .Values.deployment.image.digest }}"
-        {{- else if .Values.deployment.image.tag }}
+        image: "{{ .Values.deployment.image.repository }}:{{ .Values.deployment.image.tag }}@{{ .Values.deployment.image.digest }}"
+        {{- else }}
        image: "{{ .Values.deployment.image.repository }}:{{ .Values.deployment.image.tag }}"
        {{- end }}
+        {{- else if .Values.deployment.image.digest }}
+        image: "{{ .Values.deployment.image.repository }}@{{ .Values.deployment.image.digest }}"
+        {{- else }}
+        image: "{{ .Values.deployment.image.repository }}"
+        {{- end }}


See other comment about lowering repetition on the CronJob template file.

d3adb5 · 2023-03-26T21:20:24Z

application/templates/_helpers.tpl

+{{- if .Values.deployment.image.tag }}
 app.kubernetes.io/version: {{ include "application.version" . }}
+{{- end }}


With an empty string as the default value for .Values.deployment.image.tag, we could discard this if statement, though we would end up with an empty value for a version label. How about leaving out this conditional and instead editing the application.version template so that it:

Tries to use the tag as it currently does, but failing that, it

Tries to use the digest with a little bit of RegEx substitution magic, but if not provided either, it

Simply says "latest", as is default behavior for container images?

How does that sound?

I think the application version should not be mixed up with the image tag. The application version should always be a semantic version. Tags for containers does not not always have a semver. Currently i don't like the usage of the function.

But in general i think it would be a good idea to move the logic in a seperate function to reduce the code duplication.

The application version should always be a semantic version.

I'm not sure if I agree with this one. Helm does require chart versions to be semantic, but the versioning scheme for an application is an incredibly arbitrary choice, usually left to the core maintainers or company behind the project. Hence Helm will let appVersion be just about anything in Chart.yaml — in fact .Chart.AppVersion is often used as the default value when an image tag isn't provided, but this is a generic chart and such a value can't be set during installation.

So that said, whatever logic is present in application.version, now or in the future, this could be changed to a simple:

Suggested change

{{- if .Values.deployment.image.tag }}

app.kubernetes.io/version: {{ include "application.version" . }}

{{- end }}

{{- with include "application.version" . }}

app.kubernetes.io/version: {{ quote . }}

{{- end }}

What do you think?

Okay, i see. But what if there is no tag defined? Maybe the tag is a good default for the label, but we could allow overriding the label - maybe with another Value, e.g. .Values.applicationVersion?
Also there could be situations where the version of the application stays the same (e.g. the wordpress version) but the underlying base container receives an update (e.g. packages updates of the alpine/ubi container).

An appVersion value sounds good to me. In the case of no such value and no tag being defined, however, we can just skip the label altogether as it makes no sense to set it, and hence the suggested with being appropriate.

d3adb5 · 2023-03-26T21:22:09Z

application/templates/deployment.yaml

-{{- end }}
+{{- end }}


Whitespace changes mess with blame information. Harmless as it is, I'd suggest removing this change from any commits that'll be merged.

rasheedamir · 2023-03-29T20:06:39Z

@slauger @d3adb5 @aslafy-z I have added all three of you as maintainers of this repo 👍 lets manage it together! We can discuss stuff on slack

slauger · 2023-03-29T20:10:38Z

So how shall we continue here?

btw why do you maintain the CHANGELOG.md manually? You could avoid those merge conflicts easily. The most common tool for releases semantic-release could help maybe.

slauger · 2023-03-29T20:12:42Z

@rasheedamir ah i see. On which slack workspace can i reach you? I'm already on the Kubernetes slack workspace for example.

rasheedamir · 2023-03-29T20:49:18Z

Please join here - https://slack.stakater.com/ and we can have discussions in #help-and-support channel

Yeah i agree we should automate the CHANGELOG

d3adb5 · 2023-04-01T15:14:46Z

@slauger Do you mind if I push any commits to your branch? Also, there are conflicts now that other PRs have been merged. Let me know what you think of the changes I requested.

rasheedamir · 2023-04-10T14:14:16Z

@slauger can we proceed on this issue and get it done? @d3adb5 plz suggest/push your changes so, we can merge this PR

Simplify the logic in determining the image string for containers in a CronJob or Deployment resource.

Add unit test to test assembly of the container image string when tag and digest are given or omitted.

Check whether the output of the 'application.version' partial template is empty instead of the image tag when deciding whether to add the Kubernetes recommended application version label. The partial template was adjusted to avoid crashes when applying the regexReplace function.

Stale review, plus became one of the PR authors.

d3adb5 · 2023-04-11T23:24:53Z

I rebased @slauger's branch on master, added tests and did some minor refactoring. I'd appreciate a review since the code did change a little. It's ultimately up to you, @rasheedamir whether we go for another review round or merge this as is.

Strange thing I noticed: the logs for the unittest check show 35 tests, but there are in fact 45 in the latest commit here. Wonder what's up with that. Feels like it's not actually running off the latest commit. :(

EDIT: Looks like the unittest job is checking out master! This is odd, considering it's invoking the actions/checkout@v3 action directly, without passing any references.

rasheedamir · 2023-04-14T13:48:34Z

@mustafaStakater can you plz do the final review of this?

application/templates/cronjob.yaml

application/tests/cronjob_test.yaml

aslafy-z · 2023-04-15T21:49:16Z

@d3adb5 the use of pull_request_target event cause the CI to run on the base branch context. See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

We might want to reference the pull request head ref in the unittest checkout action so latest code is gathered before executing the actual tests.

d3adb5 · 2023-04-15T22:21:00Z

Thank you for the clarification, @aslafy-z! I just submitted a fix in #251 that should be merged before any other PRs, as it could otherwise lead to PRs breaking unit tests being merged.

rasheedamir · 2023-04-16T11:06:17Z

LGTM

slauger · 2023-04-18T06:59:24Z

Sorry, i was out of office for a few days. Thanks for taking care of this! ❤️

rasheedamir requested a review from mustafaStakater March 24, 2023 20:30

d3adb5 previously requested changes Mar 26, 2023

View reviewed changes

slauger and others added 6 commits April 10, 2023 20:02

fix: make deployment.image.tag and job.image.tag optional

a825efc

docs: update documentation and changelog

b55f443

fix: default to empty string instead of null

edb3106

refactor: simplify logic in determining image string

3efeba9

Simplify the logic in determining the image string for containers in a CronJob or Deployment resource.

test: add unit tests to test image tag & digest behavior

7653522

Add unit test to test assembly of the container image string when tag and digest are given or omitted.

d3adb5 force-pushed the fix/make-tag-optional branch from 1518c6a to a6d0cbf Compare April 11, 2023 23:20

aslafy-z reviewed Apr 14, 2023

View reviewed changes

application/templates/cronjob.yaml Show resolved Hide resolved

aslafy-z reviewed Apr 14, 2023

View reviewed changes

application/tests/cronjob_test.yaml Show resolved Hide resolved

rasheedamir approved these changes Apr 16, 2023

View reviewed changes

rasheedamir merged commit 9507bde into stakater:master Apr 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make deployment.image.tag and job.image.tag optional #234

make deployment.image.tag and job.image.tag optional #234

slauger commented Mar 24, 2023

github-actions bot commented Mar 24, 2023 •

edited

Loading

rasheedamir commented Mar 24, 2023

slauger commented Mar 24, 2023

d3adb5 left a comment

d3adb5 Mar 26, 2023

slauger Mar 27, 2023

d3adb5 Mar 27, 2023

d3adb5 Mar 26, 2023

d3adb5 Mar 26, 2023

slauger Mar 27, 2023

d3adb5 Mar 27, 2023

slauger Mar 29, 2023

d3adb5 Mar 29, 2023

d3adb5 Mar 26, 2023 •

edited

Loading

rasheedamir commented Mar 29, 2023

slauger commented Mar 29, 2023

slauger commented Mar 29, 2023

rasheedamir commented Mar 29, 2023

d3adb5 commented Apr 1, 2023

rasheedamir commented Apr 10, 2023

d3adb5 commented Apr 11, 2023 •

edited

Loading

rasheedamir commented Apr 14, 2023

aslafy-z commented Apr 15, 2023

d3adb5 commented Apr 15, 2023

rasheedamir commented Apr 16, 2023

slauger commented Apr 18, 2023

make deployment.image.tag and job.image.tag optional #234

make deployment.image.tag and job.image.tag optional #234

Conversation

slauger commented Mar 24, 2023

github-actions bot commented Mar 24, 2023 • edited Loading

rasheedamir commented Mar 24, 2023

slauger commented Mar 24, 2023

d3adb5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3adb5 Mar 26, 2023 • edited Loading

Choose a reason for hiding this comment

rasheedamir commented Mar 29, 2023

slauger commented Mar 29, 2023

slauger commented Mar 29, 2023

rasheedamir commented Mar 29, 2023

d3adb5 commented Apr 1, 2023

rasheedamir commented Apr 10, 2023

d3adb5 commented Apr 11, 2023 • edited Loading

rasheedamir commented Apr 14, 2023

aslafy-z commented Apr 15, 2023

d3adb5 commented Apr 15, 2023

rasheedamir commented Apr 16, 2023

slauger commented Apr 18, 2023

github-actions bot commented Mar 24, 2023 •

edited

Loading

d3adb5 Mar 26, 2023 •

edited

Loading

d3adb5 commented Apr 11, 2023 •

edited

Loading