fix: KubeVersion passed to helm template is incorrectly #22650

sivchari · 2025-04-13T13:05:16Z

close #22606

Checklist:

bunnyshell · 2025-04-13T13:05:21Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

codecov · 2025-04-13T13:44:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.06%. Comparing base (3bbbac4) to head (7607b68).
⚠️ Report is 430 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #22650      +/-   ##
==========================================
+ Coverage   59.98%   60.06%   +0.08%     
==========================================
  Files         343      342       -1     
  Lines       57890    57843      -47     
==========================================
+ Hits        34724    34745      +21     
+ Misses      20382    20325      -57     
+ Partials     2784     2773      -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nitishfy

I tested your changes just now. I get the same error for a version such as 1.30.1+xyz -

Failed to load target state: failed to generate manifest for source 1 of 1: rpc error: code = Unknown desc = Manifest generation error (cached): failed to execute helm template command: failed to get command args to log: `helm template . --name-template helm-webapp --namespace helm --kube-version 1.30.1_xyz --values <path to cached source>/helm-webapp/values-prod.yaml <api versions removed> --include-crds` failed exit status 1: Error: invalid kube version '1.30.1_xyz': Invalid Semantic Version

Maybe this is happening because the kube version of target cluster is not actually 1.30.1_xyz.

Secondly, with the current changes, we will be removing the kube-version support for GKE that was introduced in this PR. I'd like to know other maintainers thoughts on this PR before moving forward.

sivchari · 2025-04-14T12:27:55Z

Hi, @nitishfy ! Thank you for investigating about this issue.

How interesting! You can know that helm can receive the kube-version as a 1.30.1+xyz by seeing the log of test I added new one. Helm would replace + with _, so does this specification results in it ? I'd investigate. Thanks.

Secondly, with the current changes, we will be removing the kube-version support for GKE that was introduced in this PR. I'd like to know other maintainers thoughts on this PR before moving forward.

I totally agree with your thoughts since this change surely break the compatibility. The essence that we found the problem is current API definition accept various semantic version such as vX.Y.Z+x.y.z. We might have to discuss about rule what version is acceptable and what one isn't acceptable.

nitishfy · 2025-04-14T12:44:15Z

util/helm/helm_test.go

 	assert.Equal(t, "sample/v2", objs[0].GetAPIVersion())
 }

+func TestIssue22606(t *testing.T) {


Suggested change

func TestIssue22606(t *testing.T) {

func TestIssue22606(t *testing.T) {

please add a better name for this test.

Agree with @nitishfy. Maybe add a more descriptive name for the function and add the link to the issue as a comment.

Thanks you both, I renamed it.

sivchari · 2025-04-16T14:33:28Z

I'll discuss the backward compatibly on original issue later 👍

nitishfy · 2025-04-16T14:58:54Z

let's bring this issue in this week's meeting.

FourFifthsCode · 2025-04-16T15:09:31Z

@sivchari @nitishfy
It seems like it might be a good idea to remove text.Semver function from all three places (Helm, Kustomize, and Plugins) and make a clean break for version 3.0.

Then add documentation that:

we'll no longer remove + from Kube versions, which may break deployments for APIs which don't have valid semvers
we'll no longer remove + from Kube versions, which may change Helm output, depending on how charts handle the Kube version variable

Then for version 2.14, we could modify text.Semver to only remove the + when there is no trailing characters, which is what caused the original issue.

sivchari · 2025-04-28T23:49:57Z

@FourFifthsCode @nitishfy @leoluz
Sorry for absenting the meeting.
Does this mean creating a pr for each of v3 and v2.14?
And where is the proper documentation for these modifications?

nitishfy · 2025-04-29T14:47:50Z

@FourFifthsCode @nitishfy @leoluz Sorry for absenting the meeting. Does this mean creating a pr for each of v3 and v2.14? And where is the proper documentation for these modifications?

I think we'd be required to do cherry-picking.

cc @FourFifthsCode

crenshaw-dev · 2025-04-29T16:09:01Z

@FourFifthsCode is suggesting we make two different changes, one for master/v3.0 and the other for v2.14.

The master/v3.0 change would remove the + logic entirely, noting the change in release notes. We could merge that PR to master and then cherry-pick to release-3.0.

The 2.14 change would just alter the logic to remove only trailing + characters instead of all + characters. We'd make that change with a PR directly against the release-2.14 branch.

Personally I'd be satisfied with just the first change, since 3.0 goes GA early next week.

crenshaw-dev · 2025-04-29T16:10:24Z

Note that text.SemVer is invoked in 3 different places. We'd want to remove all 3 invocations, since they all could cause the same bug.

sivchari · 2025-04-29T17:36:14Z

I deleted text.Semver from the repo in this commit. That commits for v3.
I don't think it's good way to do cherry-pick to v2.14.X since it's breaking changes for users using Argo v2.14.X

crenshaw-dev · 2025-04-29T17:36:48Z

Code lgtm! Can you add a page to the 2.14-3.0.md file documenting the change?

sivchari · 2025-04-29T17:43:24Z

Can we consider this change as breaking changes since treating tag is changed ?
If you find the good place to write about this, please let me know 🙏

crenshaw-dev · 2025-04-29T18:35:49Z

Yep, it's a breaking change. Here's the place to document it: https://github.com/argoproj/argo-cd/blob/master/docs/operator-manual/upgrading/2.14-3.0.md

sivchari · 2025-04-30T13:04:23Z

Thanks! I added the section about this change.

crenshaw-dev · 2025-05-21T19:48:11Z

@sivchari apologies that we weren't able to get this into 3.0! Could you rebase update the docs to indicate that the change will happen in 3.1?

Signed-off-by: sivchari <[email protected]>

docs/operator-manual/upgrading/3.0-3.1.md

Signed-off-by: Michael Crenshaw <[email protected]>

Signed-off-by: sivchari <[email protected]> Signed-off-by: Michael Crenshaw <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>

Signed-off-by: sivchari <[email protected]> Signed-off-by: Michael Crenshaw <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: dsuhinin <[email protected]>

Signed-off-by: sivchari <[email protected]> Signed-off-by: Michael Crenshaw <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: enneitex <[email protected]>

sivchari requested a review from a team as a code owner April 13, 2025 13:05

sivchari mentioned this pull request Apr 13, 2025

Application helm spec kubeVersion incorrectly passed to helm template #22606

Closed

3 tasks

nitishfy reviewed Apr 14, 2025

View reviewed changes

sivchari force-pushed the fix-22606 branch from a38294a to 7b91d24 Compare April 14, 2025 12:13

nitishfy reviewed Apr 14, 2025

View reviewed changes

sivchari force-pushed the fix-22606 branch 2 times, most recently from cdb69cf to 2179ad8 Compare April 16, 2025 04:27

sivchari force-pushed the fix-22606 branch from 2179ad8 to 8f4124c Compare April 29, 2025 17:33

sivchari force-pushed the fix-22606 branch from 8f4124c to 851eed2 Compare May 2, 2025 12:00

sivchari requested a review from a team as a code owner May 2, 2025 12:00

nitishfy mentioned this pull request May 5, 2025

argocd.argoproj.io/auto-label-cluster-info doesn't work on eks clusters #19385

Open

sivchari self-assigned this May 16, 2025

sivchari requested a review from crenshaw-dev May 16, 2025 04:48

crenshaw-dev added this to the v3.1 milestone May 21, 2025

Delete text.Semver

b60c23a

Signed-off-by: sivchari <[email protected]>

sivchari force-pushed the fix-22606 branch 4 times, most recently from d8897f5 to 568f919 Compare May 30, 2025 07:29

sivchari added 2 commits May 30, 2025 16:30

remove tests for semver

b272045

Signed-off-by: sivchari <[email protected]>

add doc

c4ba2e2

Signed-off-by: sivchari <[email protected]>

sivchari force-pushed the fix-22606 branch from 568f919 to c4ba2e2 Compare May 30, 2025 07:30

crenshaw-dev reviewed Jun 2, 2025

View reviewed changes

docs/operator-manual/upgrading/3.0-3.1.md Outdated Show resolved Hide resolved

crenshaw-dev reviewed Jun 2, 2025

View reviewed changes

docs/operator-manual/upgrading/3.0-3.1.md Outdated Show resolved Hide resolved

crenshaw-dev reviewed Jun 2, 2025

View reviewed changes

docs/operator-manual/upgrading/3.0-3.1.md Outdated Show resolved Hide resolved

Apply suggestions from code review

7607b68

Signed-off-by: Michael Crenshaw <[email protected]>

crenshaw-dev approved these changes Jun 2, 2025

View reviewed changes

crenshaw-dev enabled auto-merge (squash) June 2, 2025 20:11

crenshaw-dev merged commit ea97dec into argoproj:master Jun 2, 2025
27 checks passed

8tomat8 mentioned this pull request Jul 14, 2025

Helm does not support --kube-version with trailing "+" #23778

Closed

agaudreault mentioned this pull request Aug 7, 2025

fix: revert kubeVersion change to preserve trailing + #24066

Merged

	func TestIssue22606(t *testing.T) {
	func TestIssue22606(t *testing.T) {

fix: KubeVersion passed to helm template is incorrectly #22650

fix: KubeVersion passed to helm template is incorrectly #22650

Uh oh!

Conversation

sivchari commented Apr 13, 2025

Uh oh!

bunnyshell bot commented Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Preview Environment deleted from Bunnyshell

Uh oh!

codecov bot commented Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nitishfy left a comment

Choose a reason for hiding this comment

Uh oh!

sivchari commented Apr 14, 2025

Uh oh!

nitishfy Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

leoluz Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

sivchari Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

sivchari commented Apr 16, 2025

Uh oh!

nitishfy commented Apr 16, 2025

Uh oh!

FourFifthsCode commented Apr 16, 2025

Uh oh!

sivchari commented Apr 28, 2025

Uh oh!

nitishfy commented Apr 29, 2025

Uh oh!

crenshaw-dev commented Apr 29, 2025

Uh oh!

crenshaw-dev commented Apr 29, 2025

Uh oh!

sivchari commented Apr 29, 2025

Uh oh!

crenshaw-dev commented Apr 29, 2025

Uh oh!

sivchari commented Apr 29, 2025

Uh oh!

crenshaw-dev commented Apr 29, 2025

Uh oh!

sivchari commented Apr 30, 2025

Uh oh!

crenshaw-dev commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bunnyshell bot commented Apr 13, 2025 •

edited

Loading

codecov bot commented Apr 13, 2025 •

edited

Loading