fix: update license check#712
Conversation
📝 WalkthroughWalkthroughThe Makefile's Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 164-165: The Makefile's --allowed_licenses and --ignore flags have
diverged from the CI merge-gate policy; reconcile them by choosing a canonical
policy (either the local Make target or the CI workflow) and updating the other
to match: ensure the --allowed_licenses flag includes or excludes MPL-2.0
consistently and the --ignore list includes the same HashiCorp libraries
(github.com/hashicorp/go-cleanhttp, github.com/hashicorp/go-retryablehttp)
across both places, and run the license check target locally and in CI to
validate parity between the --allowed_licenses and --ignore settings.
- Around line 164-166: Fix the Make recipe by replacing the space-indented
continuation lines for the go-licenses invocation with proper tab-indented
recipe lines so the Make target runs; also change the direct invocation of the
tool to use the version variable defined in .settings.yaml (go_licenses) by
sourcing that variable and invoking the tool via that variable instead of a bare
tool name, and keep the explicit comment "DO NOT CHANGE THIS SET OF ALLOWED
LICENSES, DO NOT ADD IGNORES" above the target as-is; look for the go-licenses
invocation and the flags lines (--allowed_licenses=..., --ignore=...) as well as
the go_licenses variable in .settings.yaml to implement this.
- Around line 160-166: The license-check Makefile target calls go-licenses
without ensuring the tool version matches .settings.yaml; add tools-check as a
prerequisite to the license-check target so the Makefile runs tools-check (which
validates go-licenses v1.6.0) before invoking go-licenses. Update the target
declaration for license-check to depend on tools-check (i.e., change the target
header for license-check to include tools-check) while leaving the existing
go-licenses invocation and ignore logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: bd3d1701-aaeb-4010-8de0-9d441924eb71
📒 Files selected for processing (1)
Makefile
| license-check: ## Check license is approved | ||
| @echo "Checking license headers..." | ||
| @STDLIB_IGNORE=$$(go list std 2>/dev/null | cut -d'/' -f1 | sort -u | paste -sd ',' -) && \ | ||
| go-licenses check ./... \ | ||
| --allowed_licenses=Apache-2.0,BSD-2-Clause,BSD-3-Clause,ISC,MIT,MPL-2.0 \ | ||
| --allowed_licenses=MIT,BSD-2-Clause,BSD-3-Clause,Apache-2.0,ISC,Zlib \ | ||
| --ignore github.com/hashicorp/go-cleanhttp,github.com/hashicorp/go-retryablehttp \ | ||
| --ignore=$$STDLIB_IGNORE |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `license-check` currently enforces tool-version validation path.
rg -n '^\s*license-check:' Makefile
rg -n '\.settings\.yaml|go_licenses|go-licenses|tools-check' Makefile tools/check-tools .settings.yamlRepository: NVIDIA/aicr
Length of output: 2212
🏁 Script executed:
# Get full target definitions for license-check and tools-check
sed -n '54,65p' Makefile
sed -n '160,175p' MakefileRepository: NVIDIA/aicr
Length of output: 1482
🏁 Script executed:
# Check what tools-check actually validates and its implementation
sed -n '54,80p' MakefileRepository: NVIDIA/aicr
Length of output: 1086
🏁 Script executed:
# Look for other targets that invoke go-licenses or similar external tools
rg -A5 'go-licenses|golangci-lint|addlicense' Makefile | head -50Repository: NVIDIA/aicr
Length of output: 1509
🏁 Script executed:
# Check if there are any other targets using prerequisites to enforce tool versions
rg -B1 -A3 '^[a-z-]+:.*[a-z-]+.*##' Makefile | head -60Repository: NVIDIA/aicr
Length of output: 2351
🏁 Script executed:
# Check what go-licenses version check looks like in tools/check-tools
sed -n '85,100p' tools/check-tools
rg -A10 'go_licenses|go-licenses' tools/check-toolsRepository: NVIDIA/aicr
Length of output: 1211
🏁 Script executed:
# Check if other similar targets also lack tool-version prerequisites
rg -B2 'golangci-lint|addlicense|go-licenses' Makefile | grep -E '^[0-9]+-[a-z]+-[a-z]+:|golangci-lint|addlicense|go-licenses'Repository: NVIDIA/aicr
Length of output: 467
🏁 Script executed:
# Look at the full definition of license-check and surrounding targets to see if there's a reason for the current pattern
sed -n '155,180p' MakefileRepository: NVIDIA/aicr
Length of output: 1468
Add tools-check prerequisite to enforce go-licenses version validation.
The license-check target currently invokes go-licenses without verifying it matches the version specified in .settings.yaml (v1.6.0). Add tools-check as a prerequisite to validate the tool version before execution, consistent with the guideline that all Makefile tool invocations must verify against .settings.yaml.
Minimal compliance fix
-license-check: ## Check license is approved
+license-check: tools-check ## Check license is approved📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| license-check: ## Check license is approved | |
| @echo "Checking license headers..." | |
| @STDLIB_IGNORE=$$(go list std 2>/dev/null | cut -d'/' -f1 | sort -u | paste -sd ',' -) && \ | |
| go-licenses check ./... \ | |
| --allowed_licenses=Apache-2.0,BSD-2-Clause,BSD-3-Clause,ISC,MIT,MPL-2.0 \ | |
| --allowed_licenses=MIT,BSD-2-Clause,BSD-3-Clause,Apache-2.0,ISC,Zlib \ | |
| --ignore github.com/hashicorp/go-cleanhttp,github.com/hashicorp/go-retryablehttp \ | |
| --ignore=$$STDLIB_IGNORE | |
| license-check: tools-check ## Check license is approved | |
| `@echo` "Checking license headers..." | |
| `@STDLIB_IGNORE`=$$(go list std 2>/dev/null | cut -d'/' -f1 | sort -u | paste -sd ',' -) && \ | |
| go-licenses check ./... \ | |
| --allowed_licenses=MIT,BSD-2-Clause,BSD-3-Clause,Apache-2.0,ISC,Zlib \ | |
| --ignore github.com/hashicorp/go-cleanhttp,github.com/hashicorp/go-retryablehttp \ | |
| --ignore=$$STDLIB_IGNORE |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 160 - 166, The license-check Makefile target calls
go-licenses without ensuring the tool version matches .settings.yaml; add
tools-check as a prerequisite to the license-check target so the Makefile runs
tools-check (which validates go-licenses v1.6.0) before invoking go-licenses.
Update the target declaration for license-check to depend on tools-check (i.e.,
change the target header for license-check to include tools-check) while leaving
the existing go-licenses invocation and ignore logic intact.
| --allowed_licenses=MIT,BSD-2-Clause,BSD-3-Clause,Apache-2.0,ISC,Zlib \ | ||
| --ignore github.com/hashicorp/go-cleanhttp,github.com/hashicorp/go-retryablehttp \ |
There was a problem hiding this comment.
License policy drift between local Make target and CI workflow.
The updated local allowlist/ignore set now differs from .github/workflows/merge-gate.yaml (which still allows MPL-2.0 and has no HashiCorp ignores). This can cause local/CI disagreement on license compliance. Please align both to one canonical policy source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 164 - 165, The Makefile's --allowed_licenses and
--ignore flags have diverged from the CI merge-gate policy; reconcile them by
choosing a canonical policy (either the local Make target or the CI workflow)
and updating the other to match: ensure the --allowed_licenses flag includes or
excludes MPL-2.0 consistently and the --ignore list includes the same HashiCorp
libraries (github.com/hashicorp/go-cleanhttp,
github.com/hashicorp/go-retryablehttp) across both places, and run the license
check target locally and in CI to validate parity between the --allowed_licenses
and --ignore settings.
| --allowed_licenses=MIT,BSD-2-Clause,BSD-3-Clause,Apache-2.0,ISC,Zlib \ | ||
| --ignore github.com/hashicorp/go-cleanhttp,github.com/hashicorp/go-retryablehttp \ | ||
| --ignore=$$STDLIB_IGNORE |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify actual leading whitespace characters around the changed recipe lines.
sed -n '160,167p' Makefile | nl -ba | cat -teRepository: NVIDIA/aicr
Length of output: 96
🏁 Script executed:
# Read the Makefile around the flagged lines to check actual indentation
cat -n Makefile | sed -n '160,167p'Repository: NVIDIA/aicr
Length of output: 510
🏁 Script executed:
# Also use cat -A to show whitespace characters explicitly
sed -n '160,167p' Makefile | cat -ARepository: NVIDIA/aicr
Length of output: 467
🏁 Script executed:
# Check if .settings.yaml exists and locate it
fd -type f -name '.settings.yaml' -o -name 'settings.yaml'Repository: NVIDIA/aicr
Length of output: 226
🏁 Script executed:
# Check the workflow file mentioned in the review for policy comparison
if [ -f '.github/workflows/merge-gate.yaml' ]; then
sed -n '251,255p' '.github/workflows/merge-gate.yaml'
fiRepository: NVIDIA/aicr
Length of output: 234
🏁 Script executed:
find . -name '*settings*' -type f | head -20Repository: NVIDIA/aicr
Length of output: 72
🏁 Script executed:
# Also check if there's any version specification for go-licenses in the repo
rg 'go-licenses.*version|version.*go-licenses' -iRepository: NVIDIA/aicr
Length of output: 435
🏁 Script executed:
# Check the entire license-check recipe and surrounding context in Makefile
sed -n '155,170p' MakefileRepository: NVIDIA/aicr
Length of output: 797
🏁 Script executed:
cat .settings.yaml | head -100Repository: NVIDIA/aicr
Length of output: 2556
🏁 Script executed:
# Search for GO_LICENSES_VERSION in .settings.yaml
rg 'GO_LICENSES_VERSION' .settings.yamlRepository: NVIDIA/aicr
Length of output: 37
Fix Make recipe indentation and ensure tool version is sourced from .settings.yaml.
Lines 164 and 166 use spaces instead of tabs, breaking the Make recipe syntax. Additionally, the go-licenses tool version must be sourced from .settings.yaml (currently defines go_licenses: 'v1.6.0') per Makefile coding guidelines, rather than using the bare tool invocation.
Note: The local license policy differs from the CI workflow (merge-gate.yaml). Local allows Zlib, CI allows MPL-2.0. Verify this intentional discrepancy with the explicit comment "DO NOT CHANGE THIS SET OF ALLOWED LICENSES, DO NOT ADD IGNORES" above the target.
🔧 Proposed fix for indentation
- --allowed_licenses=MIT,BSD-2-Clause,BSD-3-Clause,Apache-2.0,ISC,Zlib \
+ --allowed_licenses=MIT,BSD-2-Clause,BSD-3-Clause,Apache-2.0,ISC,Zlib \
--ignore github.com/hashicorp/go-cleanhttp,github.com/hashicorp/go-retryablehttp \
- --ignore=$$STDLIB_IGNORE
+ --ignore=$$STDLIB_IGNORE📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --allowed_licenses=MIT,BSD-2-Clause,BSD-3-Clause,Apache-2.0,ISC,Zlib \ | |
| --ignore github.com/hashicorp/go-cleanhttp,github.com/hashicorp/go-retryablehttp \ | |
| --ignore=$$STDLIB_IGNORE | |
| .PHONY: license-check | |
| license-check: ## Check if licenses are permitted | |
| $(GOBIN)/go-licenses check ./... \ | |
| --allowed_licenses=MIT,BSD-2-Clause,BSD-3-Clause,Apache-2.0,ISC,Zlib \ | |
| --ignore github.com/hashicorp/go-cleanhttp,github.com/hashicorp/go-retryablehttp \ | |
| --ignore=$$STDLIB_IGNORE |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 164 - 166, Fix the Make recipe by replacing the
space-indented continuation lines for the go-licenses invocation with proper
tab-indented recipe lines so the Make target runs; also change the direct
invocation of the tool to use the version variable defined in .settings.yaml
(go_licenses) by sourcing that variable and invoking the tool via that variable
instead of a bare tool name, and keep the explicit comment "DO NOT CHANGE THIS
SET OF ALLOWED LICENSES, DO NOT ADD IGNORES" above the target as-is; look for
the go-licenses invocation and the flags lines (--allowed_licenses=...,
--ignore=...) as well as the go_licenses variable in .settings.yaml to implement
this.
Coverage Report ✅
Coverage BadgeNo Go source files changed in this PR. |
2 participants
Summary
Corrected the licence check set of licences.
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
Testing
# Commands run (prefer `make qualify` for non-trivial changes) make qualifyRisk Assessment
Rollout notes:
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info