-
Notifications
You must be signed in to change notification settings - Fork 112
docs: update helm installation commands for image tag compatibility #640
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
Conversation
README.md
Outdated
| Run the following command after replacing `<VERSION>` with the desired release version: | ||
| ```sh | ||
| helm upgrade -i kai-scheduler oci://ghcr.io/nvidia/kai-scheduler/kai-scheduler -n kai-scheduler --create-namespace --version <VERSION> | ||
| helm upgrade -i kai-scheduler oci://ghcr.io/nvidia/kai-scheduler/kai-scheduler -n kai-scheduler --create-namespace --version <VERSION> --set-string global.tag=<VERSION> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? when not set the helm version should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 reasons:
chat.versionis a semantic version by its definition like1.2.3, sov1.2.3is not a standard version.- There is another
chat.appVersionwhich is a right choice for image tag because usually we may publish 1 chart to registry with multiple app versions(images)
In conclusion
chat.versionis for the helm chart itself. We need to bump it if we make changes to helm chart. The chart need to be published to registry if its version is changed.chat.appVersionis for application/docker images. No need to publish a new version of helm chart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand that when approving the other PR. Maybe we should change the chart so that it is easy to set up instead of adding more arguments for it to work well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for misleading you by the previous commit.
It's not add an additional argument.
It's replacing the --version <VERSION> to --set-string global.tag=<VERSION>.
I've pushed the latest commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then which version of the chart will be installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we need to make changes to the chart so that it will support the current commands, because there are already many scripts that depend on them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me close this PR.
I checked the details of building helm chart step.
It sets both version and appVersion so that it can select a correct chart by specify the version argument.
And images reference to appVersion which is the same value as version.
- name: Build helm chart
run: |
sed -i 's#registry/local/kai-scheduler#${{ env.DOCKER_REGISTRY }}#' deployments/kai-scheduler/values.yaml
helm package ./deployments/kai-scheduler -d ./charts --app-version $PACKAGE_VERSION --version $PACKAGE_VERSION
- name: Push Helm Chart
run: |
helm push ./charts/kai-scheduler-$PACKAGE_VERSION.tgz oci://${{ env.DOCKER_REGISTRY }}
Description
This PR updates the helm installation commands in the documentation to include the
--set-string global.tag=<VERSION>parameter, which is required after the changes introduced in PR #628.Background:
PR #628 changed the image tag resolution mechanism from using
{{ .Chart.Version }}directly to using a fallback chain:component.image.tag → global.tag → Chart.AppVersion. Sinceglobal.tagnow defaults to an empty string invalues.yaml, the--versionflag alone no longer controls the image tags used by the deployment.Impact:
Without this fix, users installing KAI Scheduler with
--version X.Y.Zwould get the correct chart version but incorrect image tags (usingChart.AppVersioninstead of the specified version).Changes:
README.mdto include--set-string global.tag=<VERSION>docs/developer/building-from-source.mdto include--set-string global.tag=0.0.0This ensures that both the Helm chart version and container image tags align with the user's intended version.
Related Issues
Related to #628
Checklist
Breaking Changes
No breaking changes - this PR fixes documentation to match the current implementation.
Additional Notes
Files Modified:
README.md- Added--set-string global.tag=<VERSION>to the production installation commanddocs/developer/building-from-source.md- Added--set-string global.tag=0.0.0to the local build installation commandVerification:
Searched all markdown files in the repository to ensure all helm installation commands have been updated.