-
Notifications
You must be signed in to change notification settings - Fork 99
Helm chart: apply license, update description #282
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
| @@ -0,0 +1,202 @@ | |||
|
|
|||
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.
This file is the result of curl https://www.apache.org/licenses/LICENSE-2.0.txt > LICENSE.
| @@ -0,0 +1,4 @@ | |||
| Copyright 2025 NVIDIA CORPORATION | |||
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 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.
Should this include the same / similar text as # SPDX-FileCopyrightText: in the license header?
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.
:) No.
The header facilitates discovering the license applied to a project/file in an automated fashion. It's typically used for SBOM generation / scanning / compliance purposes.
The NOTICE file is an implementation detail of the AL2.
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, I meant to ask whether we should include & AFFILIATES, not the SPDX tags.
This follows the instructions in https://www.apache.org/legal/apply-license.html In particular, this adheres to > If the distribution is a jar or tar file, > try to add the LICENSE file first in order to > place it at the top of the archive. This > covers the collective licensing for the distribution. Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]> Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
2ac78c0 to
4ed4515
Compare
| @@ -1,6 +1,21 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
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.
Taken from internal Wiki page.
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.
Pull Request Overview
This PR applies the Apache License text as header comments to the Helm chart and refines the chart description to better reflect its purpose.
- Added Apache license header information to Chart.yaml
- Updated the chart description to specify that it is the official Helm chart for the NVIDIA DRA driver for Kubernetes
- Revised templating comments regarding the appVersion handling for image tagging
Comments suppressed due to low confidence (1)
deployments/helm/nvidia-dra-driver-gpu/Chart.yaml:1
- The Apache guidelines require the inclusion of a LICENSE file and a NOTICE file in the top-level directory of the distribution. Ensure that these files are added to the root of your Helm chart distribution in addition to the license comment block in Chart.yaml.
# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
|
The copilot got this. 🛩️ Sweet:
|
ArangoGutierrez
left a comment
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.
just one nit
|
|
||
| # Note(JP): this currently defines the default `tag` value in a k8s | ||
| # image specification. A "v" is prefixed in our template helpers to ensure consistency. | ||
| # Note(JP): templating logic consumes `appVersion` for building the default `tag` |
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 am all for notes, maybe we can use your github handle for better bookeeping
Note (@jgehrcke)
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.
changed
| @@ -0,0 +1,202 @@ | |||
|
|
|||
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.
Does it make sense to symlink to the LICENSE file in the root? I suppose their contents are the same.
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 like the idea, maybe all we need to check is if the command helm package would agree with this, if so, I like Evan's idea
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 have confirmed that when making the following changes:
git status
On branch jp/helm-apply-license
Changes to be committed:
(use "git restore --staged <file>..." to unstage)
new file: NOTICE
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
typechange: deployments/helm/nvidia-dra-driver-gpu/LICENSE
typechange: deployments/helm/nvidia-dra-driver-gpu/NOTICE
and generating the package using hack/package-helm-charts.sh the resultant tar does contain the LICENSE and NOTICE files with the expected content.
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 like this ++
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.
Thanks for thinking about linking without blinking.
:) okäse 🧀 pushed a commit.
I thought about this (symlinking) when making this patch. Thoughts were: in this case, cost of duplication is ~zero. I also wanted to quickly get over with it and thought that it might even be smart to keep things decoupled. The most important part is that the tarball contains the proper files. From the git repo point of view we can choose not to care (is a symlinked license an actual license? what would the supreme court say? would elon come to our rescue? is armageddon about to happen soon?).
elezar
left a comment
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.
Thanks @jgehrcke
I have some comments / questions, but these could be considered out of scope. I'd be more than happy to address them in a follow-up instead.
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
Signed-off-by: Dr. Jan-Philip Gehrcke <[email protected]>
4ed4515 to
6d2fee1
Compare
elezar
left a comment
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.
Minor question regarding & AFFILIATES, but otherwise lgtm.
| @@ -1,6 +1,21 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
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.
Good question about "AFFILIATES" @elezar . Also wondered about this specific wording. But I don't want to understand/resolve this today.
We got this instruction:
ensure the Helm chart has the NVIDIA copyright and Apache License header:
url-to-confluence-page
I copy & pasted verbatim from there.
|
Inclusion of proper files confirmed with logging: |
This follows the instructions in https://www.apache.org/legal/apply-license.html. In particular, this adheres to
For the record, a screenshot from a staging environment where we see that the top-level tarball does not contain LICENSE & NOTICE files:
The goal of this patch is to make LICENSE file and NOTICE file pop up there (add them to the top-level tarball directory).