lunny/helm-chart
1
0
Fork 0
Code Issues 36 Pull Requests 14 Actions Packages Projects Releases 93 Wiki Activity

Add support for image.digest #444

Merged
pat-s merged 13 commits from refs/pull/444/head into main 2023-09-09 15:36:20 +00:00
Conversation 11 Commits 13 Files Changed 4 +90 -4
pat-s commented 2023-05-04 21:11:31 +00:00 (Migrated from gitea.com)
Copy Link

fix #398

fix #398
justusbunsi (Migrated from gitea.com) reviewed 2023-05-04 21:45:55 +00:00
justusbunsi commented 2023-05-05 04:24:20 +00:00 (Migrated from gitea.com)
Copy Link

Thanks for implementing this. Could you add tests to ensure correct rendering behavior with/without digest?

Thanks for implementing this. Could you add tests to ensure correct rendering behavior with/without digest?
pat-s commented 2023-05-29 20:07:25 +00:00 (Migrated from gitea.com)
Copy Link

@justusbunsi Waiting for your review after having added a test.

@justusbunsi Waiting for your review after having added a test.
zocimek (Migrated from gitea.com) reviewed 2023-05-29 20:30:10 +00:00
templates/_helpers.tpl
zocimek (Migrated from gitea.com) commented 2023-05-29 20:30:10 +00:00
Copy Link

this will fail with message like:

metadata.labels: Invalid value: "dev-linux-arm64@sha256:1b75efb0e3b2ac7ac62a786cbb0a48ca2c89f0b80c3ff7b435f55b4aa10744ec": must be no more than 63 characters

on gitea-inline-config configmap - it should be truncated or just use the tag (kube v1.27)

this will fail with message like: ``` metadata.labels: Invalid value: "dev-linux-arm64@sha256:1b75efb0e3b2ac7ac62a786cbb0a48ca2c89f0b80c3ff7b435f55b4aa10744ec": must be no more than 63 characters ``` on `gitea-inline-config` configmap - it should be truncated or just use the tag (kube v1.27)
zocimek (Migrated from gitea.com) commented 2023-05-30 09:58:59 +00:00
Copy Link

The idea is to only use the digest and no tag, see the respective test for it. In your case then gitea/gitea@sha256:1b75efb0e3b2ac7ac62a786cbb0a48ca2c89f0b80c3ff7b435f55b4aa10744ec.

You can add the tag information as a comment alongside the value.

The idea is to only use the digest and no tag, see the respective test for it. In your case then `gitea/gitea@sha256:1b75efb0e3b2ac7ac62a786cbb0a48ca2c89f0b80c3ff7b435f55b4aa10744ec`. You can add the tag information as a comment alongside the value.
zocimek (Migrated from gitea.com) commented 2023-05-30 10:39:17 +00:00
Copy Link

anyway it will fail according to the https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set

Valid label value:
- must be 63 characters or less (can be empty),
- unless empty, must begin and end with an alphanumeric character ([a-z0-9A-Z]),
- could contain dashes (-), underscores (_), dots (.), and alphanumerics between.

in that case digest have 71 characters so it will not be a valid label value

anyway it will fail according to the https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set ``` Valid label value: - must be 63 characters or less (can be empty), - unless empty, must begin and end with an alphanumeric character ([a-z0-9A-Z]), - could contain dashes (-), underscores (_), dots (.), and alphanumerics between. ``` in that case digest have 71 characters so it will not be a valid label value
zocimek (Migrated from gitea.com) commented 2023-05-30 16:46:09 +00:00
Copy Link

Thanks for the persistence! I think the simplest way is that we just trim the resulting label to 63 chars. The full digest is still visible in the image reference.

Thanks for the persistence! I think the simplest way is that we just trim the resulting label to 63 chars. The full digest is still visible in the image reference.
justusbunsi (Migrated from gitea.com) reviewed 2023-05-30 17:49:24 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

@pat-s Happy to talk about my comments below. :)

@pat-s Happy to talk about my comments below. :)
templates/_helpers.tpl
@ -34,4 +48,3 @@
{{/*
Create chart name and version as used by the chart label.
*/}}
justusbunsi (Migrated from gitea.com) commented 2023-05-30 18:02:52 +00:00
Copy Link

I guess you mentioned it somewhere; don't know where exactly. I've noticed you adapted the Bitnami functionality1. What is the benefit for leaving out the image tag and only use the digest? It somehow makes it less obvious which image version is actually used.

Do you have objections against combining tag and digest in the image reference?

- gitea/gitea@sha256:7b85098a953adc337ef4b53abc3533e4242a3e21cdf92166594df937687045fd
+ gitea/gitea:1.19-nightly@sha256:7b85098a953adc337ef4b53abc3533e4242a3e21cdf92166594df937687045fd
I guess you mentioned it somewhere; don't know where exactly. I've noticed you adapted the Bitnami functionality[^1]. What is the benefit for leaving out the image tag and only use the digest? It somehow makes it less obvious which image version is actually used. Do you have objections against combining tag and digest in the image reference? ```diff - gitea/gitea@sha256:7b85098a953adc337ef4b53abc3533e4242a3e21cdf92166594df937687045fd + gitea/gitea:1.19-nightly@sha256:7b85098a953adc337ef4b53abc3533e4242a3e21cdf92166594df937687045fd ``` [^1]: https://github.com/bitnami/charts/blob/main/bitnami/common/templates/_images.tpl#L6
justusbunsi (Migrated from gitea.com) commented 2023-06-04 07:54:36 +00:00
Copy Link

It somehow makes it less obvious which image version is actually used.

I agree. I thought it might help with the length discussion initially but then again this only occurs for labels and not for tag. Hence I agree the tag should be kept!

> It somehow makes it less obvious which image version is actually used. I agree. I thought it might help with the length discussion initially but then again this only occurs for `labels` and not for `tag`. Hence I agree the tag should be kept!
templates/_helpers.tpl
@ -81,7 +91,7 @@ imagePullSecrets:
Storage Class
*/}}
justusbunsi (Migrated from gitea.com) commented 2023-05-30 17:49:24 +00:00
Copy Link

The whole define "gitea.labels" part can be reverted to its pre-PR state. The if conditions are now more or less identical. The only change between if and else is the trunc 63 of the image tag (appVersion), which is not longer than 63 characters. This was only the case for .Values.image.digest.

The whole `define "gitea.labels"` part can be reverted to its pre-PR state. The if conditions are now more or less identical. The only change between if and else is the `trunc 63` of the image tag (appVersion), which is not longer than 63 characters. This was only the case for `.Values.image.digest`.
justusbunsi (Migrated from gitea.com) commented 2023-06-04 07:55:06 +00:00
Copy Link

Agree.

Agree.
unittests/statefulset/image-configuration.yaml
justusbunsi (Migrated from gitea.com) commented 2023-05-30 18:10:54 +00:00
Copy Link

Based on the outcome of the "combining tag and digest in the image reference" conversation, we probably should add more test cases to cover the correct templating behavior for gitea.image helper function. It kinda looks prone to future 🐛s due to its complexity. If you want, I can add tests for that to the branch when the mentioned conversation is resolved. I didn't wanted to add changes without asking you.

Based on the outcome of the "combining tag and digest in the image reference" conversation, we probably should add more test cases to cover the correct templating behavior for `gitea.image` helper function. It kinda looks prone to future 🐛s due to its complexity. If you want, I can add tests for that to the branch when the mentioned conversation is resolved. I didn't wanted to add changes without asking you.
justusbunsi (Migrated from gitea.com) commented 2023-06-04 07:56:23 +00:00
Copy Link

Agree. Feel free to go ahead! I think tests for all "only tag", "tag + digest" and "only digest" would be great.

Agree. Feel free to go ahead! I think tests for all "only tag", "tag + digest" and "only digest" would be great.
justusbunsi commented 2023-09-09 14:25:27 +00:00 (Migrated from gitea.com)
Copy Link

@pat-s I updated the PR branch to match the current main state.

Based on our review comment conversations, I...

  • added extensive tests for various combinations
  • reverted the conditional label handling
  • ensured that the digest value is an addition to the image tag, not a replacement

Anything you want to add, change or discuss?

@pat-s I updated the PR branch to match the current main state. Based on our review comment conversations, I... - added extensive tests for various combinations - reverted the conditional label handling - ensured that the digest value is an addition to the image tag, not a replacement Anything you want to add, change or discuss?
pat-s commented 2023-09-09 14:54:55 +00:00 (Migrated from gitea.com)
Copy Link

Thanks for continueing! It fell off my list a bit...
And was anyhow targeted towards you 😄

All good for me.

Thanks for continueing! It fell off my list a bit... And was anyhow targeted towards you 😄 All good for me.
👍 1
justusbunsi (Migrated from gitea.com) approved these changes 2023-09-09 15:35:59 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

🎉 Let's goooo

🎉 Let's goooo
mmalyska commented 2023-09-15 11:06:24 +00:00 (Migrated from gitea.com)
Copy Link

@justusbunsi by merging with upstream you have removed commit d9188bfe2a
I use renovate for automatic update of images and it doesn't support digest parameter. My values looks like

image:
    repository: gitea/gitea
    tag: 1.20.4@sha256:95ad1dc17f78eef1f12807a8f28e94e416587b266aa5153f7da3eafddc037b27
    pullPolicy: IfNotPresent
    rootless: true # only possible when running 1.14 or later

So the version is not trimmed and the labels are too long.

@justusbunsi by merging with upstream you have removed commit https://gitea.com/gitea/helm-chart/commit/d9188bfe2ae5a1b94aab0e2dae96c6c26a796d3f I use `renovate` for automatic update of images and it doesn't support `digest` parameter. My values looks like ``` image: repository: gitea/gitea tag: 1.20.4@sha256:95ad1dc17f78eef1f12807a8f28e94e416587b266aa5153f7da3eafddc037b27 pullPolicy: IfNotPresent rootless: true # only possible when running 1.14 or later ``` So the `version` is not trimmed and the labels are too long.
justusbunsi commented 2023-09-15 11:33:26 +00:00 (Migrated from gitea.com)
Copy Link

@mmalyska

@justusbunsi by merging with upstream you have removed commit d9188bfe2a
I use renovate for automatic update of images and it doesn't support digest parameter. My values looks like

image:
   repository: gitea/gitea
   tag: 1.20.4@sha256:95ad1dc17f78eef1f12807a8f28e94e416587b266aa5153f7da3eafddc037b27
   pullPolicy: IfNotPresent
   rootless: true # only possible when running 1.14 or later

So the version is not trimmed and the labels are too long.

The removal/restore was intentional, because with the digest field, there is no need (from the chart perspective) to specify the digest in tag field. So it won't be too long for labels.

I was sure there is a built-in way for renovate to look up the digest field instead. I need to check this.
For now you could use regex manager and let the docker datasource update your image spec. This will work.

@mmalyska >@justusbunsi by merging with upstream you have removed commit https://gitea.com/gitea/helm-chart/commit/d9188bfe2ae5a1b94aab0e2dae96c6c26a796d3f >I use `renovate` for automatic update of images and it doesn't support `digest` parameter. My values looks like >``` >image: > repository: gitea/gitea > tag: 1.20.4@sha256:95ad1dc17f78eef1f12807a8f28e94e416587b266aa5153f7da3eafddc037b27 > pullPolicy: IfNotPresent > rootless: true # only possible when running 1.14 or later >``` >So the `version` is not trimmed and the labels are too long. The removal/restore was intentional, because with the digest field, there is no need (from the chart perspective) to specify the digest in tag field. So it won't be too long for labels. I was sure there is a built-in way for renovate to look up the digest field instead. I need to check this. For now you could use regex manager and let the docker datasource update your image spec. This will work.
justusbunsi commented 2023-09-15 11:59:26 +00:00 (Migrated from gitea.com)
Copy Link

I was sure there is a built-in way for renovate to look up the digest field instead. I need to check this.

Hrm. Couldn't find what I had in mind. Looks like extracting from digest field would be a neat addition to the helm-values manager in Renovate.

Anyway, you can use the regex manager for that use case.

>I was sure there is a built-in way for renovate to look up the digest field instead. I need to check this. Hrm. Couldn't find what I had in mind. Looks like extracting from digest field would be a neat addition to the helm-values manager in Renovate. Anyway, you can use the regex manager for that use case.
pat-s commented 2023-09-18 15:15:38 +00:00 (Migrated from gitea.com)
Copy Link

@mmalyska Out of interest: why are you using a digest when you already referring to a fixed tag? Tags like 1.20.4 are not going to change ever, they are not rebuilt. Only rolling tags like :1 or 1.20 are.

@justusbunsi Given our custom setup here in this chart, we might want to add some instructions to the README how to make it work with the renovate regex manager? You might already use it?

@mmalyska Out of interest: why are you using a digest when you already referring to a fixed tag? Tags like 1.20.4 are not going to change ever, they are not rebuilt. Only rolling tags like `:1` or `1.20` are. @justusbunsi Given our custom setup here in this chart, we might want to add some instructions to the README how to make it work with the renovate regex manager? You might already use it?
mmalyska commented 2023-09-19 07:59:01 +00:00 (Migrated from gitea.com)
Copy Link

@mmalyska Out of interest: why are you using a digest when you already referring to a fixed tag? Tags like 1.20.4 are not going to change ever, they are not rebuilt. Only rolling tags like :1 or 1.20 are.

I use pinning digest as tags are not immutable in docker(they are just shortcuts for digests). In my whole solution I use the same approach for all deployed apps(renovate pinning digests). To be safe that the image will never change you are required to pin the digest of image.

This is example of my regex manager for gitea that replaces digest https://github.com/mmalyska/home-ops/blob/main/.github/renovate/regexmanager.json5

> @mmalyska Out of interest: why are you using a digest when you already referring to a fixed tag? Tags like 1.20.4 are not going to change ever, they are not rebuilt. Only rolling tags like `:1` or `1.20` are. I use pinning digest as tags are not immutable in docker(they are just shortcuts for digests). In my whole solution I use the same approach for all deployed apps(renovate pinning digests). To be safe that the image will never change you are required to pin the digest of image. This is example of my regex manager for gitea that replaces digest https://github.com/mmalyska/home-ops/blob/main/.github/renovate/regexmanager.json5
mmalyska commented 2023-09-19 09:10:45 +00:00 (Migrated from gitea.com)
Copy Link

@pat-s I've create PR with updated README https://gitea.com/gitea/helm-chart/pulls/514

@pat-s I've create PR with updated README https://gitea.com/gitea/helm-chart/pulls/514
pat-s commented 2023-09-19 09:57:02 +00:00 (Migrated from gitea.com)
Copy Link

I use pinning digest as tags are not immutable in docker(they are just shortcuts for digests).

I am aware but I still don't understand the "fear" behind it. So far I thought there must be "something different" than the irrational fear of a tag changing the image content without one noticing it. But I think I understood it now, thanks for the explanation.

And thanks for the PR!

> I use pinning digest as tags are not immutable in docker(they are just shortcuts for digests). I am aware but I still don't understand the "fear" behind it. So far I thought there must be "something different" than the irrational fear of a tag changing the image content without one noticing it. But I think I understood it now, thanks for the explanation. And thanks for the PR!
Sign in to join this conversation.
Reviewers
No Reviewers
justusbunsi
Labels
Clear labels
has/backport
in progress
invalid
kind/breaking
kind/bug
kind/build
kind/dependency
kind/deployment
kind/docs
kind/enhancement
kind/feature
kind/lint
kind/proposal
kind/question
kind/refactor
kind/security
kind/testing
kind/translation
kind/ui
need/backport
priority/critical
priority/low
priority/maybe
priority/medium
reviewed/duplicate
reviewed/invalid
reviewed/wontfix
skip-changelog
status/blocked
status/needs-feedback
status/needs-reviews
status/wip
upstream/gitea
upstream/other
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lunny/helm-chart#444
No description provided.
Delete Branch "refs/pull/444/head"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?