Skip to content

Conversation

@ArangoGutierrez
Copy link
Collaborator

No description provided.

@ArangoGutierrez ArangoGutierrez self-assigned this Aug 20, 2025
Copy link

Copilot AI left a comment

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 fixes the prepare-release.sh script to improve compatibility with macOS environments and updates various deployment files to version 0.17.3. The script now handles GNU sed availability more gracefully on macOS systems.

  • Enhanced macOS compatibility by preferring gsed over Docker when available
  • Added proper error handling and user-friendly error messages for missing dependencies
  • Updated all deployment manifests and charts from version 0.17.1 to 0.17.3

Reviewed Changes

Copilot reviewed 9 out of 12 changed files in this pull request and generated 2 comments.

File Description
hack/prepare-release.sh Enhanced macOS sed handling with gsed preference and improved error handling
deployments/static/*.yml Updated container image versions from v0.17.1 to v0.17.3
deployments/static/*.yaml Updated app.kubernetes.io/version labels from 0.17.1 to 0.17.3
deployments/helm/nvidia-device-plugin/Chart.yaml Updated chart version, appVersion, and NFD dependency to 0.17.3

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

-exec $SED -E -i \
-e s",^([[:space:]]+)app.kubernetes.io\/version:.+$,\1app.kubernetes.io\/version: $semver," \
-e s",^([[:space:]]+)- image:.+$,\1- image: $container_image," \
-e s",^([[:space:]]*)app.kubernetes.io\/version:.+$,\1app.kubernetes.io\/version: $semver," \
Copy link

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

The regex pattern uses [[:space:]]* (zero or more spaces) instead of [[:space:]]+ (one or more spaces). This change could match lines that start directly with 'app.kubernetes.io/version:' without any indentation, which may not be the intended behavior and could cause unintended replacements.

Suggested change
-e s",^([[:space:]]*)app.kubernetes.io\/version:.+$,\1app.kubernetes.io\/version: $semver," \
-e s",^([[:space:]]+)app.kubernetes.io\/version:.+$,\1app.kubernetes.io\/version: $semver," \

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

@ArangoGutierrez was there a case that required this to be changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did a dry run, and the image: on the files at deployments/static were not edited.

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't really make sense since the output for the following (on main) are the same:

$ grep -R -E "^\s+-\s+image:" deployments/static
deployments/static/gpu-feature-discovery-daemonset.yaml:        - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/nvidia-device-plugin-compat-with-cpumanager.yml:      - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/gpu-feature-discovery-job.yaml.template:        - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/nvidia-device-plugin.yml:      - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/gpu-feature-discovery-daemonset-with-mig-mixed.yaml:        - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/gpu-feature-discovery-daemonset-with-mig-single.yaml:        - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1

and

$ grep -R -E "^\s*-\s+image:" deployments/static
deployments/static/gpu-feature-discovery-daemonset.yaml:        - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/nvidia-device-plugin-compat-with-cpumanager.yml:      - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/gpu-feature-discovery-job.yaml.template:        - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/nvidia-device-plugin.yml:      - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/gpu-feature-discovery-daemonset-with-mig-mixed.yaml:        - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1
deployments/static/gpu-feature-discovery-daemonset-with-mig-single.yaml:        - image: nvcr.io/nvidia/k8s-device-plugin:v0.17.1

Also looking at the diff here I don't see any changes where there are no leading spaces for the pattern that we're trying to match.

Copy link
Member

Choose a reason for hiding this comment

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

Also: Running on main I see the following:

$ git diff --name-only upstream/main deployments/static | cat
deployments/static/gpu-feature-discovery-daemonset-with-mig-mixed.yaml
deployments/static/gpu-feature-discovery-daemonset-with-mig-single.yaml
deployments/static/gpu-feature-discovery-daemonset.yaml
deployments/static/gpu-feature-discovery-job.yaml.template
deployments/static/nvidia-device-plugin-compat-with-cpumanager.yml
deployments/static/nvidia-device-plugin.yml

Which -- with the exception of nfd.yaml -- seems to match the changes you have here.

When comparing it directly to your branch we see:

$ git diff --name-only static_deployments  deployments/static | cat
deployments/static/nfd.yaml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Umm I see, I'll review why on my hosts the script didn't worked as expected.

-e s",^([[:space:]]+)app.kubernetes.io\/version:.+$,\1app.kubernetes.io\/version: $semver," \
-e s",^([[:space:]]+)- image:.+$,\1- image: $container_image," \
-e s",^([[:space:]]*)app.kubernetes.io\/version:.+$,\1app.kubernetes.io\/version: $semver," \
-e s",^([[:space:]]*)- image:.+$,\1- image: $container_image," \
Copy link

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

Similar to the previous line, changing from [[:space:]]+ to [[:space:]]* could match lines that start directly with '- image:' without any indentation, potentially causing unintended replacements in YAML files where indentation is critical.

Suggested change
-e s",^([[:space:]]*)- image:.+$,\1- image: $container_image," \
-e s",^([[:space:]]+)- image:.+$,\1- image: $container_image," \

Copilot uses AI. Check for mistakes.

@ArangoGutierrez
Copy link
Collaborator Author

As mentioned here NVIDIA/cloud-native-docs#235, some platforms don't support Helm by design, making it important for us to keep our static files up to date.

- name: node-feature-discovery
alias: nfd
version: "0.16.6"
version: "0.17.3"
Copy link
Member

Choose a reason for hiding this comment

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

We should bump the nfd dependency as a separate PR. It is especially confusing that we happen to have the same version here.

@elezar
Copy link
Member

elezar commented Sep 9, 2025

As mentioned here NVIDIA/cloud-native-docs#235, some platforms don't support Helm by design, making it important for us to keep our static files up to date.

I don't quite agree. Would generating a static file using helm template not be a better fit for this -- especially as the set of supported features increase.

@ArangoGutierrez
Copy link
Collaborator Author

By comment #1379 (comment) I will close this PR and open a PR focused only on bumping NFD version

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.

2 participants