Switch image.version to image.tag #245

kfox1111 · 2023-04-27T21:35:38Z

The convention in most charts is to use image.tag. This pr updates the values to use it instead of the less standard image.version.

marcofranssen · 2023-05-02T23:23:09Z

Most charts depends on what charts you are used to.

https://github.com/sigstore/helm-charts/blob/main/charts/common/templates/_images.tpl

The sigstore charts all use the convention I have adopted here as well. Don't really see to value of changing that.

kfox1111 · 2023-05-02T23:34:35Z

Most charts depends on what charts you are used to.

That's fair.

I'm recommending the convention that helm itself, at least as far back as 2.0.0 (Nov 16, 2016), recommended whatever you issue 'helm create foo". (I don't have access to download 1.0.0)

It really isn't a very big deal at the end of the day. But it is a very very old/common convention.

Its a tiny thing I agree and largely doesn't matter. But lots of little tiny improvements add up to bigger usability gains in the long run.

edwbuck · 2023-05-08T21:49:13Z

@marcofranssen You seemed to reject the request in your clear explanation; but, I can't determine if you actually rejected it because I see an explanation but not the course of action you wish to take.

I would urge you to change your mind, as tags are a superset of versions conceptually, such that a tag may contain a version, or any other text that helps someone manage which specific container should be deployed ("qa", "prod", etc.)

@kfox1111 If you feel that the benefit of using the word "tag" is greater that the benefit of using the word "version". Please ensure that we don't use the two as synonyms of each other. In the examples below, tags are being attached to tagged TARGET_IMAGES.

docker tag 0e5574283393 fedora/httpd:version1.0
docker tag http fedora/httpd:version1.0
docker tag version1.0 fedora/httpd:version1.0

This tounge twister of an explanation is based on the usage statement of
docker tag SOURCE_IMAGE[:TAG] TARGET_IMAGE[:TAG] as noted in the documentation.

As tags only SOMETIMES contain text that seem to look like traditional (or even non-traditional) versions, I would prefer we shift to the use of the word "tag" for consistency, even though older documentation primarily demonstrated tagging with version numbers. This use of "tag" permits hints that the tracked item can change, based on the tag's change, as is common with "latest", "nightly", or even "passed_acceptance" (where such a tag has no meaning outside of the organization).

marcofranssen · 2023-05-09T09:08:44Z

The downside of changing this thing is we have a (unnecessary) breaking change in the API. If everyone feels we need to change this I will follow, but still don't see the value of really changing that. In the end a version covers the same ground, it is just a matter definition.

If you do strict semver then a version can only be

v1.0.0
v2.0.0-rc
v2.0.0-beta

However if you take the version less strict. I don't see why a version can't be named.

latest
unstable
v2-release

It is all a matter of how strict you take the term version.

edwbuck · 2023-05-10T00:27:36Z

I think we've been thinking of this as, if we have "tag", we can't have "version".

This is one of those rare cases where we can have both.

If the more-modern "tag" is set, we use it's value.
If the traditional "version" is set, we use it's value.
If both are set, we issue an error and fail.
If neither are set, we issue an error to set the "tag".

I understand the rules established to deprecate items; and, I understand the rules established that consider the values.yaml as part of the "API" that if changed, needs changed in ways that APIs are managed. What I think all of those rules have done is combine in a way that presents a false dichotomy.

Basically, having one value key doesn't mean we must eject the other, or migrate the other. There's no need to take a stand on the "right" one. There's not even a need to stop supporting "version". We can support both for many years to come, as it is a relatively clear cut case of one being an alias of the other. If this "dual" support causes problems, we can also submit future issues to push the users to migrate into the last "key" standing.

This compromise would have both our old and newer users of Helm satisfied. It's rare, but I think such a solution would literally be the best of both worlds.

kfox1111 · 2023-05-10T01:52:28Z

Yeah. That's a great point, that makes sense to me and feels like a good compromise.

faisal-memon · 2023-05-10T02:39:35Z

So in this scenario version would be a hidden configurable? Not listed in the docs.

marcofranssen · 2023-05-10T08:06:06Z

So in this scenario version would be a hidden configurable? Not listed in the docs.

We can't hide it from the docs, what we can do is add a description Deprecated, please use tag from now on, this setting will be removed in a future release. This offers people a migration path while we shortly support one or the other.

edwbuck · 2023-05-10T13:59:57Z

Another option for the docs, is "version" - "the tag value to pull if tag is not set, to support legacy conventions", assuming we aren't intent on deprecation. If we are intent on deprecation, then something like Marco's suggestion is fine.

edwbuck · 2023-05-10T14:01:29Z

@kfox1111 Now that we have enough alignment to unblock this one, can you provide a code change so we can get this merged with priority?

kfox1111 · 2023-05-12T15:22:10Z

Marco has volunteered to do the agreed changes. Thanks Marco! :)

marcofranssen · 2023-05-14T04:58:36Z

Marco has volunteered to do the agreed changes. Thanks Marco! :)

? 🤔

kfox1111 · 2023-05-14T13:12:19Z

Marco has volunteered to do the agreed changes. Thanks Marco! :)

? thinking

This is what you wrote to me on slack. Did I misunderstand? I'm sorry if I did. I don't want to be putting words in other peoples mouths.

Marco Franssen
https://github.com/spiffe/helm-charts/pull/245 for this one, if we can add a deprecated message to version and do a fallback to version in the macro for now
we will remove it later in 2 or 3 releases
I’m also happy to update on your behalf to take away some load

It sounds good to me.

mrsabath

Thank you for cleaning up and providing a easy migration process. I reviewed the PR and it looks correctly to me. I commented on a nit related to usage of latest tag, but it's not a show stopper.

charts/spire/charts/spire-agent/values.yaml

charts/spire/charts/spire-server/values.yaml

marcofranssen · 2023-05-17T13:14:01Z

Thank you for cleaning up and providing a easy migration process. I reviewed the PR and it looks correctly to me. I commented on a nit related to usage of latest tag, but it's not a show stopper.

Handled those, good catch, copy paste 😞

kfox1111 · 2023-05-17T15:52:36Z

This looks very good but has one issue I can see. Due to a merge happening this line needs an additional check to look for tag as well as version:
https://github.com/spiffe/helm-charts/blob/main/charts/spire/charts/spire-server/templates/_helpers.tpl#L110

marcofranssen · 2023-05-17T18:20:42Z

This looks very good but has one issue I can see. Due to a merge happening this line needs an additional check to look for tag as well as version: main/charts/spire/charts/spire-server/templates/_helpers.tpl#L110

@kfox1111 Would this be sufficient?

https://github.com/spiffe/helm-charts/pull/245/files#diff-f9ef77203566edfa336f7bcc66f084886b74cabff3fb5af5f64b13b3cb2deb3dR110-R112

The convention in most charts is to use image.tag. This patch updates the values to use it instead of the less standard image.version. Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>

Deprecating version allows users of the chart to have a migration path Signed-off-by: Marco Franssen <marco.franssen@gmail.com>

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>

kfox1111

LGTM

* c1c5b11 Merge pull request #306 from spiffe/remove-1.21 * 0df45e3 Fix up docs * ed038fe Upgrade to spire 1.6.4 (#308) * dc5d9cf Fix root README.md * e4447fd Upgrade Tornjak to new image v1.2.1 (#299) * 69f402e Update docs * 38d51d5 Apply suggestions from code review * a1ba235 Update docs * 1922085 Fix hooks for K3s (#305) * 4fb549e Remove 1.21.x testing * 88efc77 Allow to use spire-server as an upstream authority (#304) * 0ba0388 Add support for spire-server ingress (#68) * 4777a30 Bump test chart dependencies (#301) * 00c2c1a Fix the generated pr so that it runs jobs too (#303) * dd1ad49 Update images for cve's found by the cronjob (#290) * 1c69470 Updated Tornjak documenation with Not-for-production labels (#297) * 7809637 Merge pull request #296 from spiffe/dependabot/github_actions/helm/kind-action-1.7.0 * e61ed17 Merge pull request #295 from spiffe/dependabot/github_actions/sigstore/cosign-installer-3.0.5 * 9975e58 Merge pull request #245 from spiffe/tags * 7bb7ece Bump helm/kind-action from 1.6.0 to 1.7.0 * f1623a5 Bump sigstore/cosign-installer from 3.0.4 to 3.0.5 * f8db5a3 Fix Tornjak persistence issue (#294) * b30b412 Tornjak reuse spire-lib.cluster-domain macro (#292) * 90c9eb5 Fix kubectl-image macro to handle version deprecation * 300d1cc Apply deprecation of image.version to Tornjak * d850486 Instead of removing version, first deprecate version * 59e422b Add documentation for all image.tag values * d1f3cdb Switch image.version to image.tag * 31ce704 Cleanup maintainer handbook (#287) * a2da943 Remove manual dispatch from dummy workflow (#288) * 807558b Bump helm/kind-action from 1.5.0 to 1.6.0 (#285) * 3df67db Bump sigstore/cosign-installer from 3.0.3 to 3.0.4 (#286) * 5505d41 Merge pull request #283 from spiffe/additional-k8s-native-feature-tornjak-frontend * 391f093 Allow to configure topologySpreadConstraints for tornjak-frontend * 5cc26d3 Allow to configure tolerations for tornjak-frontend * 3537161 Allow to configure affinity for tornjak-frontend * aed6fdf Use the correct kubectl for the cluster (#248) * ee43c5e Add nodeSelector for tornjak * fc13cbd Merge pull request #234 from spiffe/tornjak * ed472aa Update documentation * a11cfc9 Allow to define the resources for tornjak backend * 382e0d4 Upgrade Tornjak image to version v1.2.0 (#259) * 657c460 Update charts/spire/charts/tornjak-frontend/templates/service.yaml * 7521caf Update charts/spire/charts/spire-server/templates/tornjak-config.yaml * b64c352 Update charts/spire/charts/spire-server/templates/tests/test-tornjak-connection.yaml * 6ddf6ab Improve tornjak docs (#276) * 80d34f0 Use common post-install scripts for testing * f5efa0c Remove dead macros * bd86518 Fixing shellcheck * 91bdea2 Provide minimal resources to prevent accidental crashes due to resource exhaustion * 1675997 Tornjak global image fix (#228) * 5e827ee Add Tornjak Tests (#220) * bdba97b Add empty directory to Tornjak to support npm cache (#224) * da186c5 Split Tornjak Frontend into separate subchart (#179) * 6d22126 Add Tornjak * 2669d8b Add maintainer's handbook. (#265) * 72596ae Skip tests for docs folders (#281) * 7c71738 Bump test chart dependencies (#279) * 05addae Add json to test path (#280) * 8d9b734 Switch the spire tests to always run (#250) Signed-off-by: Marco Franssen <marco.franssen@gmail.com>

kfox1111 requested review from marcofranssen, developer-guy, dfeldman, faisal-memon and mrsabath as code owners April 27, 2023 21:35

marcofranssen force-pushed the tags branch from 118163a to a7a0ceb Compare May 9, 2023 08:27

faisal-memon assigned edwbuck May 10, 2023

edwbuck removed their assignment May 12, 2023

edwbuck self-requested a review May 12, 2023 20:01

faisal-memon added this to the 0.8.x milestone May 15, 2023

marcofranssen self-assigned this May 16, 2023

marcofranssen force-pushed the tags branch 3 times, most recently from b82f063 to 8eef26b Compare May 17, 2023 08:47

marcofranssen enabled auto-merge May 17, 2023 08:54

marcofranssen force-pushed the tags branch 2 times, most recently from 9bf768b to 917e528 Compare May 17, 2023 08:59

marcofranssen force-pushed the tags branch 4 times, most recently from 92b7318 to 39ebade Compare May 17, 2023 09:29

mrsabath approved these changes May 17, 2023

View reviewed changes

charts/spire/charts/spire-agent/values.yaml Outdated Show resolved Hide resolved

charts/spire/charts/spire-server/values.yaml Outdated Show resolved Hide resolved

marcofranssen force-pushed the tags branch 2 times, most recently from 72acb19 to b21a6b7 Compare May 17, 2023 12:48

kfox1111 and others added 5 commits May 17, 2023 20:21

Switch image.version to image.tag

d1f3cdb

The convention in most charts is to use image.tag. This patch updates the values to use it instead of the less standard image.version. Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>

Add documentation for all image.tag values

59e422b

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>

Instead of removing version, first deprecate version

d850486

Deprecating version allows users of the chart to have a migration path Signed-off-by: Marco Franssen <marco.franssen@gmail.com>

Apply deprecation of image.version to Tornjak

300d1cc

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>

Fix kubectl-image macro to handle version deprecation

90c9eb5

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>

marcofranssen force-pushed the tags branch from 5051bd8 to 90c9eb5 Compare May 17, 2023 18:21

kfox1111 commented May 17, 2023

View reviewed changes

faisal-memon approved these changes May 18, 2023

View reviewed changes

marcofranssen merged commit 9975e58 into main May 18, 2023

marcofranssen deleted the tags branch May 18, 2023 07:18

marcofranssen mentioned this pull request May 25, 2023

Bump spire Helm Chart version from 0.7.0 to 0.8.0 #313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch image.version to image.tag #245

Switch image.version to image.tag #245

kfox1111 commented Apr 27, 2023

marcofranssen commented May 2, 2023

kfox1111 commented May 2, 2023

edwbuck commented May 8, 2023

marcofranssen commented May 9, 2023 •

edited

Loading

edwbuck commented May 10, 2023 •

edited

Loading

kfox1111 commented May 10, 2023

faisal-memon commented May 10, 2023

marcofranssen commented May 10, 2023

edwbuck commented May 10, 2023

edwbuck commented May 10, 2023

kfox1111 commented May 12, 2023

marcofranssen commented May 14, 2023

kfox1111 commented May 14, 2023

mrsabath left a comment

marcofranssen commented May 17, 2023

kfox1111 commented May 17, 2023

marcofranssen commented May 17, 2023

kfox1111 left a comment

Switch image.version to image.tag #245

Switch image.version to image.tag #245

Conversation

kfox1111 commented Apr 27, 2023

marcofranssen commented May 2, 2023

kfox1111 commented May 2, 2023

edwbuck commented May 8, 2023

marcofranssen commented May 9, 2023 • edited Loading

edwbuck commented May 10, 2023 • edited Loading

kfox1111 commented May 10, 2023

faisal-memon commented May 10, 2023

marcofranssen commented May 10, 2023

edwbuck commented May 10, 2023

edwbuck commented May 10, 2023

kfox1111 commented May 12, 2023

marcofranssen commented May 14, 2023

kfox1111 commented May 14, 2023

mrsabath left a comment

Choose a reason for hiding this comment

marcofranssen commented May 17, 2023

kfox1111 commented May 17, 2023

marcofranssen commented May 17, 2023

kfox1111 left a comment

Choose a reason for hiding this comment

marcofranssen commented May 9, 2023 •

edited

Loading

edwbuck commented May 10, 2023 •

edited

Loading