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

chore(chart): Updated release logic to use SemVer #5561

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

stevehipwell
Copy link
Contributor

@stevehipwell stevehipwell commented Jan 30, 2024

Fixes #4248

Description
This PR changes the chart release implementation to use a valid SemVer version (x.y.z) for the chart version by removing the tag's v prefix; this matches the modification to Chart.yaml made in the stable branch PR. This PR also updates the documentation to support this change.

Once this has been merged and the first release has been cut with this code it'd make sense to also create SemVer tags for the other versions supporting v1beta1 (0.32.0, 0.32.1, 0.32.2, 0.32.3, 0.32.4, 0.32.5, 0.32.6, 0.33.0, 0.33.1, 0.33.2 & 0.34.0). The chart packages would need to be pulled and the chart version modified to remove the v prefix before being pushed back up.

How was this change tested?
Ad-hoc

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@stevehipwell stevehipwell requested a review from a team as a code owner January 30, 2024 15:58
@stevehipwell stevehipwell requested a review from ljosyula January 30, 2024 15:58
@stevehipwell
Copy link
Contributor Author

CC @jonathan-innis

Copy link

netlify bot commented Jan 30, 2024

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit f443c20
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/65cc84bc9d213c000848dc0e
😎 Deploy Preview https://deploy-preview-5561--karpenter-docs-prod.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jonathan-innis
Copy link
Contributor

This looks like it adds the semver change combined with the artifacthub change. Can we break those changes apart?

@stevehipwell
Copy link
Contributor Author

@jonathan-innis it could be broken apart but I'm not sure of the value in doing so? Both changes are release process changes which make sense to be tested together. The actual Artifact Hub change is ignored until the org has been configured, the repos adopted or recreated and the config file content added.

That said, I'll split it up if you'd like.

@jonathan-innis
Copy link
Contributor

That said, I'll split it up if you'd like

Yeah, I get that it's really not that difficult to add one way or the other. Mainly in the spirit of making sure all of the PRs accurately describe what they are doing and the commit squash history correctly captures everything

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

Few comments. Nice work!

hack/release/common.sh Outdated Show resolved Hide resolved
hack/toolchain.sh Outdated Show resolved Hide resolved
@stevehipwell
Copy link
Contributor Author

Yeah, I get that it's really not that difficult to add one way or the other. Mainly in the spirit of making sure all of the PRs accurately describe what they are doing and the commit squash history correctly captures everything

@jonathan-innis I'm of a similar opinion; but the reality of a non-contributor contribution is high friction, so bunching similar changes into a single PR makes much better use of available time.

Shall we get all of the changes into this PR and agreed on to cut the initial load? Once the changes have been agreed on in principal I can split it up for testing if required.

@jonathan-innis jonathan-innis self-assigned this Feb 1, 2024
@jonathan-innis
Copy link
Contributor

Shall we get all of the changes into this PR and agreed on to cut the initial load

The changes look good to me as-is, aside from changing the format of the Karpenter version parameter. I'd still opt for separate PRs, but I don't think there will be a ton of back-and-forth since these changes already look alright

@stevehipwell
Copy link
Contributor Author

@jonathan-innis I'm planning on changing the meaning of KARPENTER_VERSION in the docs and all of the scripts for consistency, so there will be a bit of reviewing required. I'll make the change here and then split the PR up once you're happy in principal. I'm thinking 3 PRs; 1 for the chart SemVer & docs, 1 for signing the chart artifact & 1 for managing the Artifact Hub config.

@stevehipwell stevehipwell force-pushed the helm-chart-semver branch 3 times, most recently from cc91174 to 08f4dd2 Compare February 2, 2024 14:50
@stevehipwell
Copy link
Contributor Author

@jonathan-innis this should be all of the changes. I spent a long time looking at the scripts in hack but on reflection I decided that I was going to need to effectively re-write them for very little real world benefit.

@jonathan-innis
Copy link
Contributor

this should be all of the changes

Sounds good. Do you mind breaking up the changes for the aritfactrepo and the cosigning like we discussed originally so we can stage that in a different PR and it makes it easier for me to see which changes are necessary here?

@coveralls
Copy link

coveralls commented Feb 6, 2024

Pull Request Test Coverage Report for Build 7898945351

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 82.497%

Totals Coverage Status
Change from base Build 7896313409: 0.0%
Covered Lines: 5015
Relevant Lines: 6079

💛 - Coveralls

@stevehipwell stevehipwell force-pushed the helm-chart-semver branch 2 times, most recently from 7dfb018 to b5ceb43 Compare February 6, 2024 09:49
@stevehipwell
Copy link
Contributor Author

@jonathan-innis this PR should now be scoped to only the SemVer change.

jonathan-innis
jonathan-innis previously approved these changes Feb 7, 2024
Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Looks like it just needs a rebase

@stevehipwell
Copy link
Contributor Author

@jonathan-innis this has been rebased and is ready to be merged.

@jonathan-innis
Copy link
Contributor

One question that got raised here: If we are changing the version of our Helm chart to be semver-compliant, we might as well make the change to our images as well. Thoughts on having our tags in our Git repo being v-prefixed but having the assets (images and helm chart remove the prefix) @stevehipwell?

@stevehipwell
Copy link
Contributor Author

One question that got raised here: If we are changing the version of our Helm chart to be semver-compliant, we might as well make the change to our images as well. Thoughts on having our tags in our Git repo being v-prefixed but having the assets (images and helm chart remove the prefix) @stevehipwell?

@jonathan-innis this PR is fixing the fact that Helm explicitly linked the OCI tag to the chart SemVer; OCI images don't have this constraint and both patterns are common. Given this the OCI image tag format is related but isn't logically part of the same changes and the fact that this PR has already being split up to focus on a single change I think it should be discussed in a new issue and if implemented done so in a new PR.

FWIW I personally prefer using the SemVer version for an OCI image tag instead of replicating the git tag v prefix.

@jonathan-innis
Copy link
Contributor

SemVer version for an OCI image tag instead of replicating the git tag v prefix

Agreed. I think we should make a change in both. Happy to approve this change. Are you open to making the PR to update the OCI tag that we use for images as well?

@stevehipwell
Copy link
Contributor Author

Are you open to making the PR to update the OCI tag that we use for images as well?

@jonathan-innis yes I'm happy to do that too.

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jonathan-innis jonathan-innis enabled auto-merge (squash) February 16, 2024 04:26
@jonathan-innis jonathan-innis merged commit 185d220 into aws:main Feb 16, 2024
16 checks passed
@stevehipwell stevehipwell deleted the helm-chart-semver branch February 16, 2024 09:59
@billylaing
Copy link

Hello! Thanks so much for updating the tagging for Karpenter! When can we expect to see the new tags pushed out to oci://public.ecr.aws/karpenter/karpenter?

@stevehipwell
Copy link
Contributor Author

@billylaing check out the 0.35.0 release.

@billylaing
Copy link

Thanks! I just got Karpenter running in our cluster yesterday. Really cool so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support the Semver2 standard
4 participants