Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: check chart versions against target version in users config before upgrading #2319

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

daniel-weisse
Copy link
Member

@daniel-weisse daniel-weisse commented Sep 7, 2023

Context

Our CLI embeds the Helm charts used for the Kubernetes cluster.
For charts we pull in from external providers, the version of these charts is dependent on the version we set for that release.
For charts we package ourselves, the version of the chart is dependent on the version of the CLI binary.
For these charts, on constellation upgrade apply, we should only apply an upgrade if the version specifies in the user's config is an upgrade to the version running in their cluster. And only to the version set in the config.
For this the Helm code is currently missing some checks.

Proposed change(s)

  • Check if config version matches CLI version when installing new charts
  • Check if version specified in config is a valid upgrade to the chart running in the Kubernetes cluster
  • If a chart is a valid upgrade, check that the version specified in the user's config matches the version of charts embedded in the CLI
  • Add unit tests for the appendNewAction method

Additional info

  • From what I can tell, this bug was not present in the v2.10 releases

@daniel-weisse daniel-weisse added the no changelog Change won't be listed in release changelog label Sep 7, 2023
@daniel-weisse daniel-weisse added this to the v2.11.0 milestone Sep 7, 2023
@netlify
Copy link

netlify bot commented Sep 7, 2023

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit 60f53f7
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/64fb12e22652470008ce999b

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

Coverage report

Package Old New Trend
cli/internal/helm 43.80% 45.30% ↗️

@daniel-weisse daniel-weisse force-pushed the fix/cli/config-microservice-version-checking branch from db2a183 to 3146c37 Compare September 7, 2023 13:27
@derpsteb
Copy link
Member

derpsteb commented Sep 8, 2023

LGTM. We definitely need those checks. I will let Adrian give the final approval as he is working on that code much more actively.

@derpsteb derpsteb requested review from elchead and removed request for derpsteb September 8, 2023 07:50
"helm.sh/helm/v3/pkg/chart"
)

func TestAppendNewAction(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@daniel-weisse daniel-weisse force-pushed the fix/cli/config-microservice-version-checking branch 2 times, most recently from aaa97d9 to 36fcbdf Compare September 8, 2023 10:20
Copy link
Contributor

@elchead elchead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniel-weisse daniel-weisse force-pushed the fix/cli/config-microservice-version-checking branch from 36fcbdf to 60f53f7 Compare September 8, 2023 12:26
@3u13r 3u13r merged commit 2a1996d into main Sep 8, 2023
@3u13r 3u13r deleted the fix/cli/config-microservice-version-checking branch September 8, 2023 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Change won't be listed in release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants