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

Set version to 25.3.0-rc.1 #277

Merged
merged 1 commit into from
Mar 11, 2025
Merged

Conversation

jgehrcke
Copy link
Contributor

@jgehrcke jgehrcke commented Mar 11, 2025

We decided that we want to make a release candidate for the upcoming release at first.

Note that as of https://helm.sh/docs/topics/charts/#the-chartyaml-file the version field in a Helm chart YAML is required to be semver-compliant, and a v prefix isn't allowed: https://semver.org/#is-v123-a-semantic-version.

For simplicity, we keep the appVersion field the same (although it doesn't have the same strict constraint).

Edit: no, give appVersion a "v" prefix again because it populates the default value for the image tag at

.

Copy link

copy-pr-bot bot commented Mar 11, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@@ -18,7 +18,7 @@ MODULE := github.com/NVIDIA/$(DRIVER_NAME)

REGISTRY ?= nvcr.io/nvidia/cloud-native

VERSION ?= v25.3.0
VERSION ?= v25.3.0-rc.1
Copy link
Contributor Author

@jgehrcke jgehrcke Mar 11, 2025

Choose a reason for hiding this comment

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

In the Makefile, my assumption is that we want to keep the v prefix, and also our git tag strategy will continue to use the v prefix.

Edit: looking at vVERSION below this VERSION here is meant to not have a v prefix.

In hacks/prepare-helm-release.sh we rely on VERSION to not have a v prefix.

edit: new approach: don't consume vVERSION, conditionally v-strip VERSION in context of Helm.

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the Helm chart version to a release candidate by removing the invalid "v" prefix and appending "-rc.1" to both the version and appVersion fields. The changes ensure the Chart.yaml complies with semver for the version field while synchronizing appVersion with the new release candidate value.

  • Updated version field in Chart.yaml from "v25.3.0" to "25.3.0-rc.1"
  • Updated appVersion field in Chart.yaml from "v25.3.0" to "25.3.0-rc.1"
@@ -27,7 +27,8 @@ ifeq ($(IMAGE_NAME),)
IMAGE_NAME := $(REGISTRY)/$(DRIVER_NAME)
endif

IMAGE_VERSION := $(VERSION)
# Here we would like to use the v prefix.
IMAGE_VERSION := $(vVERSION)
Copy link
Contributor Author

@jgehrcke jgehrcke Mar 11, 2025

Choose a reason for hiding this comment

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

Kevin stated via Slack

the container image should still have the v

This change ensures we use the v prefix here (for the image).

Copy link
Member

Choose a reason for hiding this comment

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

It's not quite as simple as that ...
the SHA-tagged images shouldn't have a v prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SHA-tagged images shouldn't have a v prefix.

Okay. That means we must not set IMAGE_VERSION := $(vVERSION).

We could do

IMAGE_VERSION := $(git rev-parse --short=8)

instead in case of CI. But my intuition is that we don't like this change.

We have a new package-helm-charts.sh in here that always wants a version w/o v prefix:

So, I will revert the IMAGE_VERSION change and amend package-helm-charts.sh to conditionally strip a v prefix from VERSION.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done the above.

@klueska
Copy link
Collaborator

klueska commented Mar 11, 2025

You will also need to make appropriate changes in:

deployments/helm/nvidia-dra-driver-gpu/Chart.yaml

@jgehrcke jgehrcke force-pushed the jp/v25.3.0-rc.1 branch 2 times, most recently from 5b39789 to 6e2e145 Compare March 11, 2025 16:52
@@ -19,10 +19,19 @@ set -o pipefail
# if arg1 is set, it will be used as the version number
if [ -z "$1" ]; then
VERSION=$(awk -F= '/^VERSION/ { print $2 }' versions.mk | tr -d '[:space:]')
# Remove any v prefix, if exists.
VERSION="${VERSION#v}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ varx="v25.3.0"; varx="${varx#v}"; echo $varx
25.3.0


$ varx="bc"; varx="${varx#a}"; echo $varx
bc

@jgehrcke
Copy link
Contributor Author

Thanks for reviews. Incorporated the feedback and squashed into a single commit.

@jgehrcke
Copy link
Contributor Author

@ArangoGutierrez had another important remark, as of which appVersion must have the v prefix. I amended the patch and once again force-pushed. @ArangoGutierrez Thank you. With your approval we should now move ahead and try this out (and see where it breaks). :)

Note that as of
https://helm.sh/docs/topics/charts/#the-chartyaml-file

the `version` field is required to be semver-compliant,
and a `v` prefix isn't allowed:

https://semver.org/#is-v123-a-semantic-version

For simplicity, we keep the appVersion field the same
(although it doesn't have the same strict constraint)

build tooling: version strings: revise v prefix approach

Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
@jgehrcke
Copy link
Contributor Author

This revealed a number of interesting insights and inter-dependencies.

The various constraints could be met with a different implementation. The current patch is just one way to do things. We can iterate in the future. (for example, appVersion could also not be v-prefixed and then we could add the `v in the calculation, to just name one of many different methods).

If this contains further mistakes (because arguably, things got complex and are difficult to check pre-merge) then we will reveal that during the actual release process.

Feeling OK about the current state. Thanks for reviews, merging.

@jgehrcke jgehrcke merged commit 51de3cb into NVIDIA:main Mar 11, 2025
7 checks passed
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.

4 participants