Link third-party notices to their licence files, and add a per-release notices document - #2794
abrarshivani wants to merge 8 commits into
Conversation
3476d8a to
c3e1d1a
Compare
abrarshivani
changed the title
367455e to
3496238
Compare
3496238 to
71941ec
Compare
📝 WalkthroughWalkthroughThe pull request replaces the notice generator with repository and release modes. It resolves vendored dependencies, validates release image and bundled-component metadata, renders linked license sections, and writes output atomically. It adds Bash tests and test helpers. The Makefile validates generated notices and runs tool tests. A manual workflow generates and uploads release notices for a tag or commit. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The current release workflow remains usable, but the stage parser should be corrected to prevent inaccurate notices after future Dockerfile changes; test cleanup is a bounded secondary issue. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
tools/generate-third-party-notices_test.sh-23-23 (1)
23-23: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean all temporary resources with one EXIT trap.
OUTPUTis never removed. The trap on Line 37 removes onlyfixture. An interruption or an early shell exit also bypasses the cleanup at the end of the script.Create one temporary directory. Set its EXIT trap immediately. Create all test fixtures inside that directory.
Based on learnings: “Any temporary file or directory created with
mktempor similar must be cleaned up via a trap, set immediately after creation.”Source: Learnings
🧹 Nitpick comments (2)
tools/generate-third-party-notices.sh (2)
406-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate license-file check. The second identical test is unreachable because the first
dieexits when the file is missing.Proposed fix
[[ -f "${LICENSE_TEXTS_DIR}/${license_file}" ]] \ - || die "${BUNDLED_COMPONENTS} names ${license_file} for ${component}, missing from ${LICENSE_TEXTS_DIR}." - [[ -f "${LICENSE_TEXTS_DIR}/${license_file}" ]] \ || die "${BUNDLED_COMPONENTS} names ${license_file} for ${component}, which is missing from ${LICENSE_TEXTS_DIR}."
339-339: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReset
in_final_stageon everyFROM.
base_image_from_dockerfiletreats the lastFROMas the release stage, butfinal_stage_copiesremains active after the first matchingFROM. A supported Dockerfile with an earlier stage from the same distroless repository can therefore makecheck_bundled_coverageandthird_party_bundled_rowsprocess intermediate-stageCOPY --frompaths as release-image content.Proposed fix
- $1 == "FROM" && index($2, base_repository) == 1 { in_final_stage = 1; next } + $1 == "FROM" { in_final_stage = (index($2, base_repository) == 1); next }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: ff1cfd65-1d3c-466f-84c3-c6ca60b81eea
⛔ Files ignored due to path filters (2)
tools/bundled-components.tsvis excluded by!**/*.tsvtools/license-overrides.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
.github/workflows/third-party-notices-release.yaml.gitignoreMakefileTHIRD_PARTY_NOTICES.mdgpu-operator-v26.7.0-THIRD_PARTY_NOTICES.mdtools/generate-third-party-notices.shtools/generate-third-party-notices_test.shtools/licenses/busybox/LICENSEtools/test-helpers.sh
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
82189e3 to
297539c
Compare
The index's third column repeated the module path and carried no information. It now links to the license file as vendored here, so a link resolves to the exact text reproduced below it and cannot rot when upstream retags, renames or archives a repository. License text now comes from vendor/ rather than from the tree that 'go-licenses save' produces. That tree holds only the one file go-licenses classified, which silently dropped sixteen license-bearing files we redistribute: PATENTS for every golang.org/x module and for google.golang.org/protobuf, go.yaml.in/yaml/v2's LICENSE.libyaml, opencontainers/go-digest's LICENSE.docs, and a vendored AUTHORS. Nothing reads that tree any more, so the 'save' pass and its cache are gone. The nearest license walking up from a package wins, so a module that licenses a subtree separately is still attributed to that subtree's own license rather than the module root's. go-licenses reports a file bundling two licenses as whichever it scores highest, which understated go.yaml.in/yaml/v2 as Apache-2.0 and v3 as MIT when both are Apache-2.0 and MIT. tools/license-overrides.tsv records the identifiers, read from the vendored files by eye rather than derived by grep, and a row naming a package that is no longer shipped fails the run rather than leaving a stale license claim behind. Scanning vendor/ reads far more directories than the old cache did, so license_files_for uses parameter expansion and [[ =~ ]] instead of a basename and grep subshell per file. That is roughly 5,700 process spawns a run, and the difference between 25 and 61 seconds. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
The document on main describes a moving target, so it states no version and cites main, which the freshness check keeps in step with it. A release artifact describes one build, so it states the version of every dependency and cites the commit that build came from. Links are pinned to the commit rather than the tag even when a tag names the release. A tag can be re-pointed at another commit later, at which point a tag-based link starts serving bytes the document never described; a commit cannot move. That is selected by a subcommand, 'repo' or 'release', rather than an environment variable. An inherited variable would have redirected the output to the versioned filename, leaving THIRD_PARTY_NOTICES.md untouched while check-third-party-notices reported success. For the same reason that target no longer regenerates in place and runs git diff: it generates to a temporary file and compares, so a generator that wrote somewhere else fails the check instead of passing it vacuously. The version reaches the index rows in both modes and only its rendering is gated, so one index serves both documents. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
Manually triggered, and it publishes nothing. The document is uploaded as a workflow artifact for a human to review and attach wherever it is wanted. A release asset is public and permanent, so putting one there should be a decision rather than a side effect of running this, and keeping it out means the whole workflow stays read-only. The ref must be a release tag or a full commit SHA. A branch is refused: the release document states versions and links to licenses pinned to them, and a document that looks frozen must not describe a moving target. The commit the tag resolved to is recorded in the job summary, since a tag alone does not pin one. Only tags carrying this tooling can be generated. Earlier tags have no generator and no go-licenses entry in tools/tools.go, so there is nothing at that ref to run, and reaching further back would mean running trusted tooling from another checkout against them. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
The repo document names the base image in prose, because it tracks a Dockerfile that moves. A release describes one build, so it states the version that build actually used, read from the final FROM in the Dockerfile at that commit. Between writing this and finishing it the base went from v4.1.2 to v4.1.4, which is the kind of drift a hand-maintained paragraph absorbs silently. NVIDIA publishes distroless sources per released version, so the source index is derived from the tag. A '-dev' tag is built from the sources published under the corresponding release version, so the suffix is dropped to find the index. Only a digest-pinned literal image is accepted. An ARG-interpolated tag cannot be resolved without the build arguments the release was actually built with, and a base image outside nvcr.io/nvidia/distroless has its sources published somewhere else. Both stop the run rather than put an unverified version, or a link to a page that does not describe the image, into a legal document. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
The release image carries more than Go modules: a busybox binary is copied in from the shell stage and was attributed nowhere. It is GPL-2.0-only, which also carries a corresponding-source obligation the document has to state. The catalogue is keyed by the path each COPY takes from a build stage, not by the stage name. One stage supplies several components under different terms, and a stage name is a refactorable label with no stability contract: the same Dockerfile has no shell stage at older tags. Every copied path must have a row, so a component cannot reach the image without someone recording what it is and how it is licensed. NVIDIA's own components are marked first-party and left out, since they are not third party to NVIDIA. That covers the CUDA compatibility libraries and the cuda-samples vectorAdd binary. Both still need a row, because the guard covers every copied path whether or not the component is listed. apt resolves busybox at build time, so its version cannot be read from the Dockerfile. It is recorded here instead, against the digest of the builder it was observed in, and the generator fails once the Dockerfile stops building from that digest. A version nobody re-checked is worse than no version at all, so bumping the builder has to force the question. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
A 40-character SHA in every Location cell reads badly in a published document, and the tag is what a reader is looking for. The ref the release was cut from is used instead, so a run against v26.3.2 links to v26.3.2. A full SHA is still accepted, and is what a run against a bare commit uses. This does give up an immutability the SHA had: a tag can be re-pointed at another commit later, and these links would then serve bytes the document never described. The job summary records the commit the tag resolved to, so a document can still be traced back to what it was generated from. The generated document is written outside the checkout and uploaded from there. It only ever exists as a workflow artifact, so the repository does not need to ignore a file that is never created in it. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
d7fb4d5 to
f182dc6
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| make install-tools | ||
| artifact_name="gpu-operator-${INPUT_REF}-THIRD_PARTY_NOTICES.md" | ||
| bash tools/generate-third-party-notices.sh release \ | ||
| --version "${INPUT_REF}" \ | ||
| --link-ref "${INPUT_REF}" \ | ||
| --repo-url "${REPO_URL}" \ | ||
| --output "${RUNNER_TEMP}/${artifact_name}" |
There was a problem hiding this comment.
🟡 Existing release tags produce no artifact
For older tags, checkout restores the legacy generate-third-party-notices.sh, which ignores all release arguments. It writes inside the checkout, so artifact upload fails because artifact_path is absent.
Learn more
The dispatched workflow comes from the default branch, but checkout replaces the working tree with the requested release. Tags created before this PR contain the old generator, whose main accepts unused positional arguments and always writes THIRD_PARTY_NOTICES.md. The workflow requests a file under RUNNER_TEMP, so the upload step receives a nonexistent path.
Example: Dispatching the workflow for existing tag v26.7.0 checks out that tag. Its generator ignores release --version ... --output ..., writes THIRD_PARTY_NOTICES.md, and leaves ${RUNNER_TEMP}/gpu-operator-v26.7.0-THIRD_PARTY_NOTICES.md absent.
Recommended fix: Define whether historical releases are supported. If they are, preserve a compatible generator and metadata from the workflow revision before checking out the target, then run them against an isolated checkout of the target. The solution also needs a policy for release metadata files absent from historical tags.
Was this helpful? React with 👍 or 👎 to provide feedback.
| check_bundled_coverage "${BASE_IMAGE_REPOSITORY}" | ||
| third_party_bundled_rows "${BASE_IMAGE_REPOSITORY}" > "${BUNDLED_ROWS}" |
There was a problem hiding this comment.
🟡 Bundled metadata validation never runs
check_bundled_coverage reads BUNDLED_ROWS before third_party_bundled_rows populates it. Invalid third-party versions and license files therefore bypass this guard.
| check_bundled_coverage "${BASE_IMAGE_REPOSITORY}" | |
| third_party_bundled_rows "${BASE_IMAGE_REPOSITORY}" > "${BUNDLED_ROWS}" | |
| third_party_bundled_rows "${BASE_IMAGE_REPOSITORY}" > "${BUNDLED_ROWS}" | |
| check_bundled_coverage "${BASE_IMAGE_REPOSITORY}" |
Was this helpful? React with 👍 or 👎 to provide feedback.
| override=$(LC_ALL=C awk -F'\t' -v key="${package}" ' | ||
| /^#/ { next } | ||
| $1 == key { print $2; found = 1; exit } | ||
| END { exit !found } | ||
| ' "${LICENSE_OVERRIDES}") && printf '%s' "${override}" && return 0 | ||
| printf '%s' "${reported}" |
There was a problem hiding this comment.
🟡 Empty overrides erase license identifiers
An empty override makes license_identifier_for return an empty identifier instead of the reported license. Generated notice tables and sections then publish a blank license.
| override=$(LC_ALL=C awk -F'\t' -v key="${package}" ' | |
| /^#/ { next } | |
| $1 == key { print $2; found = 1; exit } | |
| END { exit !found } | |
| ' "${LICENSE_OVERRIDES}") && printf '%s' "${override}" && return 0 | |
| printf '%s' "${reported}" | |
| override=$(LC_ALL=C awk -F'\t' -v key="${package}" ' | |
| /^#/ { next } | |
| $1 == key { print $2; found = 1; exit } | |
| END { exit !found } | |
| ' "${LICENSE_OVERRIDES}") && { | |
| [[ -n "${override}" ]] || die "${LICENSE_OVERRIDES} has an empty license identifier for ${package}." | |
| printf '%s' "${override}" | |
| return 0 | |
| } | |
| printf '%s' "${reported}" |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (from_stage == "" || first_source == 0) next | ||
| for (i = first_source; i < NF; i++) copies[++copy_count] = $i "\t" $NF |
There was a problem hiding this comment.
🟡 Local third-party copies escape notices
A final-stage local COPY is skipped when from_stage is empty, including the vendored NFD CRD. Released notices contain no attribution for that third-party file.
Learn more
The final image receives files from both build stages and the local build context. The parser records only COPY --from=... instructions, so local files never reach the catalogue coverage check. The repository identifies the node-feature-discovery chart as vendored upstream content, and the final image copy places its CRD in the released image.
Example: COPY deployments/gpu-operator/charts/node-feature-discovery/crds/nfd-api-crds.yaml /opt/gpu-operator/nfd-api-crds.yaml has no --from. The parser skips it, the catalogue needs no row, and the release document omits NFD attribution.
Recommended fix: Parse all final-stage COPY instructions, including local sources. Add catalogue rows and applicable license/source metadata for third-party local content, while classifying project-owned local paths explicitly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| # every copied path needs a row so nothing reaches the image unattributed. | ||
| # Write "-" for an absent value; an empty field is rejected. | ||
| # source_path component disposition version license_identifier license_file notices_url provenance_digest attribution_note | ||
| /busybox busybox third-party 1:1.37.0-6 GPL-2.0-only busybox/LICENSE https://sources.debian.org/data/main/b/busybox/1%3A1.37.0-6/debian/copyright sha256:3a39a0592364683e6bab97937b72cad5a8fa6dcbbee90edb3bb48c7f8e94f258 apt resolves the package at build time, so the version is the one Debian trixie ships for this builder digest |
There was a problem hiding this comment.
🔍 Notices metadata depends on image updates
The new catalogue records Dockerfile versions and digests changed by this branch. Confirm those image updates belong in this notices-focused PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fi | ||
| exit 0 | ||
| fi | ||
| git rev-parse --verify --quiet "refs/tags/${INPUT_REF}^{commit}" >/dev/null \ |
There was a problem hiding this comment.
should we also verify the tag's commit matches head
[[ "$(git rev-parse HEAD)" == \
"$(git rev-parse "refs/tags/${INPUT_REF}^{commit}")" ]]
There was a problem hiding this comment.
Yes, it is good to have this check. Updated.
| done < <(final_stage_copies "${base_repository}") | ||
|
|
||
| local component version license_file | ||
| while IFS=$'\t' read -r component version _ license_file _ _ _; do |
There was a problem hiding this comment.
should we check the other fields as well here
while IFS=$'\t' read -r component version license license_file notices_url _ _; do
[[ "${version}" != "-" ]] || die "${component} has no version."
[[ "${license}" != "-" ]] || die "${component} has no license identifier."
[[ "${license_file}" != "-" ]] || die "${component} has no license file."
[[ "${notices_url}" != "-" ]] || die "${component} has no notices URL."
[[ -f "${LICENSE_TEXTS_DIR}/${license_file}" ]] \
|| die "License file for ${component} does not exist."
done < "${BUNDLED_ROWS}"
why are we skipping them currently ?
There was a problem hiding this comment.
Good catch! I've added checks for license and notices_url as well.
Review of the branch found several ways it could finish successfully
while producing a document that was quietly wrong. All are failures now.
- die inside a process substitution ended only that subshell, so
check_bundled_coverage returned 0 having checked nothing. Rows are
materialised before the checks read them.
- An unresolved ${ARG} left an empty field. Tab is IFS whitespace, so the
field collapsed and every later column shifted: a component's licence
rendered as its version, its notices link as its install path.
Expansions are assigned before use and the catalogue's shape is
asserted where awk preserves empty fields.
- An empty base image repository matched every FROM line, so builder
stages' COPY sources were reported as redistributed. It is a parameter
now, and an empty one is refused.
- grep '^FROM' | tail under pipefail exited before its own die could say
why. cut | grep -q returns 141 on SIGPIPE, which inverted the
Unknown-licence guard. Both are single awk passes.
- A catalogue row with no trailing newline was skipped, disabling the
guard for that row alone.
- COPY parsing took the second field as the source, dropping every source
but the first and ignoring any COPY carrying another flag.
- A self-referential ARG default looped forever; duplicate catalogue keys
let a stale row hide a shipped component; an empty override identifier
rendered a blank licence.
- assert_fails accepted exit 127, so a renamed function passed while
testing nothing. It rejects 127 and reports the output.
Each index row is resolved once and rendered twice rather than resolved
in both renderers, which also moves the failure ahead of any output.
Names that asserted something untrue are corrected, the workflow refuses
a branch named like a tag, and comments are reduced to licence headers
and shellcheck directives.
Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
Not part of the change. Drop this commit before merging. Signed-off-by: Abrar Shivani <ashivani@nvidia.com>
f182dc6 to
ab9b946
Compare
| while IFS=$'\t' read -r source_path component _ _ _ _ _ provenance_digest _; do | ||
| case "${source_path}" in ''|'#'*) continue ;; esac | ||
| [[ "${provenance_digest}" == "-" ]] && continue | ||
| LC_ALL=C grep -qF "${provenance_digest}" "${DOCKERFILE}" \ |
There was a problem hiding this comment.
I think this is a little fragile, we check if the provenance_digest is present somewhere in the Dockerfile. I think for a component we should verify what the source stage is and what the image digest used there is and whether that matches the provenance_digest
example if we expect BusyBox to have A as the digest but we have this in the Dockerfile
FROM debian@sha256:A AS old-shell
FROM alpine@sha256:B AS new-shell
COPY --from=new-shell /busybox /busybox
This will pass currently even though busybox is not being built from A, I think this is not an issue for any of the Dockerfiles currently, but this might break in the future.
2 participants
Adds licence links to the notices document on
main, and a manuallytriggered workflow that builds a richer per-release document.
Problem
PATENTSandNOTICEwere not reproduced alongside the licence they accompany, becausego-licenses savecopies only the file it classified.go.yaml.in/yaml/v2andv3are bothApache-2.0 AND MIT.Changes
Repo document (
THIRD_PARTY_NOTICES.md, gated by CI)vendor/, which is what we actually ship.Locationlinks to each licence file in this repository.PATENTS,NOTICE,AUTHORS,LICENSE.*) reproduced alongside the main licence.tools/license-overrides.tsvcarries curated SPDX identifiers for files bundling two licences, joined withAND.Release artifact (workflow only, never committed)
FROM.Workflow
Guards
The generator fails rather than emitting a document that is quietly wrong:
Unknownor empty licence identifierCOPYinto the final image with no catalogue rowVerification
make check-third-party-noticesgreenNot included
Base-layer packages of the image itself. The base image is named with its version and linked to NVIDIA's per-version OSS source index, which is not a substitute for that image's own licence texts.
The last commit adds a generated sample for review and should be dropped before merge.