Repository navigation
chore(operator): replace format_license.py with google/addlicense - #378
Merged
Merged
Conversation
The committed file had the Apache header above `//go:build !ignore_autogenerated`. gofmt canonicalizes a build constraint to the top of the file, so `gofmt -l` flagged it and every `make fmt` rewrote it. The nodewright copy of the same generated file was already correct; only the legacy one had drifted. Cause: a build constraint may be preceded only by blank lines and line comments, and our header is a `/* */` block comment. gofmt therefore hoists the constraint above it, while scripts/format_license.py re-inserts the header at position 0 on the next run. `make fmt` runs license-fmt then `go fmt` and ends correct; `make install` runs license-fmt alone, which is how the un-gofmt'd version reached a commit. No behavior change: `go list` honors the constraint in both orderings. Refs #377 Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
github-actions
Bot
added
doc
7 tasks
Contributor
📝 WalkthroughWalkthroughThe pull request replaces the Python license formatter with addlicense and a shared Apache-2.0 template. The operator Makefile adds formatting and header-check targets and runs the check during linting. A pinned addlicense installation target is added. License-header guidance and repository headers are updated, and a new ConfigMap update assertion fixture is introduced. Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/deps.mk`:
- Around line 155-159: Update the install-deps aggregate target to depend on the
existing addlicense target, ensuring make install-deps installs the tool used by
license-fmt and license-header-check. Keep the addlicense target and its
installation behavior unchanged.
🪄 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: d747d91b-02ca-459d-b296-15d6c1314593
⛔ Files ignored due to path filters (1)
operator/api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go
📒 Files selected for processing (9)
.claude/CLAUDE.mdk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-deploy.yamlk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-update-no-interrupt.yamlk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-update-while-running.yamlk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-update.yamloperator/Makefileoperator/deps.mkscripts/format_license.pyscripts/license-header.tmpl
💤 Files with no reviewable changes (1)
- scripts/format_license.py
Coverage Report for CI Build 30411362928Coverage remained the same at 78.836%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions205 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
lockwobr
force-pushed
the
chore/377-addlicense
branch
from
July 29, 2026 00:01
3e28673 to
cdeea26
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/CLAUDE.md:
- Line 295: Update the “License headers” guidance to accurately describe
generated-file handling: addlicense skips files with recognized generated-code
markers, while zz_generated.* files are not necessarily skipped solely by
filename. If the repository intends to exclude all zz_generated.* files,
document the explicit filename exclusion mechanism instead.
In `@operator/Makefile`:
- Around line 156-157: Update the license_files definition to account for
tracked JSON schemas, then ensure the license-fmt and license-header-check
targets apply a JSON-safe validation or explicitly exclude JSON through the
established policy mechanism. Preserve the existing exclusions for vendor and
chart files.
- Around line 164-165: Update the license-header-check target to validate
tracked source headers exactly against scripts/license-header.tmpl, including
the Apache-2.0/SPDX text, so MIT or malformed headers fail; keep license-fmt
add-only and unchanged.
🪄 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: a5f9c7a2-8d28-4a30-a39d-75ae72eb052c
📒 Files selected for processing (9)
.claude/CLAUDE.mdk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-deploy.yamlk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-update-no-interrupt.yamlk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-update-while-running.yamlk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-update.yamloperator/Makefileoperator/deps.mkscripts/format_license.pyscripts/license-header.tmpl
💤 Files with no reviewable changes (1)
- scripts/format_license.py
scripts/format_license.py inserted the Apache header at position 0 of every file. For Go that meant a `/* */` block comment above any `//go:build` constraint, which is not legal, so gofmt hoisted the constraint back and the two tools fought on every `make fmt`. addlicense emits `//` line comments for Go, which are legal above a constraint, and skips generated files outright. Changes: - deps.mk: pin ADDLICENSE_VERSION and add the install target next to go-licenses - Makefile: license-fmt now runs addlicense with the new template - Makefile: add license-header-check and wire it into `make lint` so a missing header fails CI on every PR. There was no source-header gate before; `license-check` is go-licenses, which covers dependency licenses, not headers. - scripts/license-header.tmpl: the existing wording, unchanged - delete scripts/format_license.py (636 lines) Four chainsaw fixtures had no header and now have one. Every other tracked source file is untouched: addlicense skips anything whose first 1000 bytes already contain a copyright or SPDX identifier, so this is not a repo-wide rewrite. Behavior changes worth knowing: - Headers are only added, never rewritten. The old script compared normalized text without stripping the year, so editing a file silently bumped its copyright year; the tree currently reads 213 files at 2025 and 346 at 2026 as a result. Year of first publication and no bump matches Google, Kubernetes, and the Linux kernel. - A malformed header in an existing file is no longer auto-repaired. license-header-check does not catch that either, since it only detects absence. - New Go files get `//` headers while existing files keep `/* */`. Converting the existing ~570 is deliberately out of scope to avoid a PR touching every file; it can be a separate mechanical commit. - Paths come from `git ls-files` rather than a tree walk, so an untracked local worktree or venv is never rewritten. vendor/ and chart/ stay excluded as before. Closes #377 Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
lockwobr
force-pushed
the
chore/377-addlicense
branch
from
July 29, 2026 00:30
cdeea26 to
325e373
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
operator/Makefile (1)
156-157: 🔒 Security & Privacy | 🟠 MajorInclude tracked JSON files in the enforced license set.
license_filesomits*.json, solicense-fmtandlicense-header-checknever process tracked JSON files. That conflicts with the repository rule requiring Apache-2.0 SPDX headers for tracked JSON source files. Add a JSON-safe validation path or document an explicit exception.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator/Makefile` around lines 156 - 157, Update the Makefile’s license_files definition to include tracked *.json files so license-fmt and license-header-check enforce SPDX headers on them, using a JSON-safe validation approach or explicitly documenting any intentional exception.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@operator/Makefile`:
- Around line 156-157: Update the Makefile’s license_files definition to include
tracked *.json files so license-fmt and license-header-check enforce SPDX
headers on them, using a JSON-safe validation approach or explicitly documenting
any intentional exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9f3ec1fa-1c53-4b6a-84b7-36091cb64306
📒 Files selected for processing (10)
.claude/CLAUDE.mdk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-deploy.yamlk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-update-no-interrupt.yamlk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-update-while-running.yamlk8s-tests/chainsaw/nodewright/config-nodewright/assert-cm-update.yamloperator/Makefileoperator/deps.mkoperator/versions.shscripts/format_license.pyscripts/license-header.tmpl
💤 Files with no reviewable changes (1)
- scripts/format_license.py
rice-riley
approved these changes
Jul 29, 2026
lockwobr
enabled auto-merge (squash)
July 29, 2026 17:04
Collaborator
Author
|
@coderabbitai resolve |
Contributor
✅ Action performedComments resolved. Approval is disabled; enable |
lockwobr
added a commit
that referenced
this pull request
Jul 29, 2026
* fix(operator): make zz_generated.deepcopy.go gofmt-clean The committed file had the Apache header above `//go:build !ignore_autogenerated`. gofmt canonicalizes a build constraint to the top of the file, so `gofmt -l` flagged it and every `make fmt` rewrote it. The nodewright copy of the same generated file was already correct; only the legacy one had drifted. Cause: a build constraint may be preceded only by blank lines and line comments, and our header is a `/* */` block comment. gofmt therefore hoists the constraint above it, while scripts/format_license.py re-inserts the header at position 0 on the next run. `make fmt` runs license-fmt then `go fmt` and ends correct; `make install` runs license-fmt alone, which is how the un-gofmt'd version reached a commit. No behavior change: `go list` honors the constraint in both orderings. Refs #377 Signed-off-by: Brian Lockwood <lockwobr@gmail.com> * chore(operator): replace format_license.py with google/addlicense scripts/format_license.py inserted the Apache header at position 0 of every file. For Go that meant a `/* */` block comment above any `//go:build` constraint, which is not legal, so gofmt hoisted the constraint back and the two tools fought on every `make fmt`. addlicense emits `//` line comments for Go, which are legal above a constraint, and skips generated files outright. Changes: - deps.mk: pin ADDLICENSE_VERSION and add the install target next to go-licenses - Makefile: license-fmt now runs addlicense with the new template - Makefile: add license-header-check and wire it into `make lint` so a missing header fails CI on every PR. There was no source-header gate before; `license-check` is go-licenses, which covers dependency licenses, not headers. - scripts/license-header.tmpl: the existing wording, unchanged - delete scripts/format_license.py (636 lines) Four chainsaw fixtures had no header and now have one. Every other tracked source file is untouched: addlicense skips anything whose first 1000 bytes already contain a copyright or SPDX identifier, so this is not a repo-wide rewrite. Behavior changes worth knowing: - Headers are only added, never rewritten. The old script compared normalized text without stripping the year, so editing a file silently bumped its copyright year; the tree currently reads 213 files at 2025 and 346 at 2026 as a result. Year of first publication and no bump matches Google, Kubernetes, and the Linux kernel. - A malformed header in an existing file is no longer auto-repaired. license-header-check does not catch that either, since it only detects absence. - New Go files get `//` headers while existing files keep `/* */`. Converting the existing ~570 is deliberately out of scope to avoid a PR touching every file; it can be a separate mechanical commit. - Paths come from `git ls-files` rather than a tree walk, so an untracked local worktree or venv is never rewritten. vendor/ and chart/ stay excluded as before. Closes #377 Signed-off-by: Brian Lockwood <lockwobr@gmail.com> --------- Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #377.
Two commits, reviewable separately.
6c949579fix: makezz_generated.deepcopy.gogofmt-clean. One file, the pre-existing bug. The committed version had the header above//go:build !ignore_autogenerated; gofmt canonicalizes a build constraint to the top, sogofmt -lflagged it and everymake fmtrewrote it. No behavior change:go listhonors the constraint in both orderings.3e286736chore: replaceformat_license.pywithgoogle/addlicense. The migration.Why the drift happened
Not a
//go:buildparsing problem in either tool. A build constraint may be preceded only by blank lines and line comments. Our header is a/* */block comment, which is not legal there, so gofmt hoists the constraint above it andformat_license.pyre-inserts the header at position 0 on the next run. addlicense emits//for Go, which is legal above a constraint, and skips generated files outright viaCode generated ... DO NOT EDIT..Correction to the issue
#377 claims this is a zero-diff migration on the basis of a scan showing "570 files, 0 modified". That scan was wrong. It used
xargs -a, which BSD xargs on macOS does not support, and the failure was swallowed by2>/dev/null, so the empty output read as "nothing missing".The real number is 4: four chainsaw fixtures under
k8s-tests/chainsaw/nodewright/config-nodewright/have no header today and gain one here. Verified they still parse to the same 3 documents each, before and after, withyq ea '[.] | length'; the header is a leading comment and semantically inert.Every other tracked source file is untouched, so the substance of the claim holds: this is not a repo-wide rewrite. But 4 is not 0, and the issue text should not be trusted on that point.
What changed
deps.mk: pinADDLICENSE_VERSIONand add the install target next togo-licensesMakefile:license-fmtruns addlicense againstscripts/license-header.tmplMakefile: newlicense-header-check, wired intomake lintso a missing header fails CI on every PR. There was no source-header gate before;license-checkis go-licenses, covering dependency licenses, not headers.scripts/license-header.tmpl: the existing wording, unchangedscripts/format_license.py: deleted, 636 lines.claude/CLAUDE.md: the tooling note, which described the script as adding and replacing headersPaths come from
git ls-filesrather than a tree walk. That is deliberate and worth a look in review: addlicense takes explicit paths and will happily rewrite anything it is pointed at, and a naive walk picks up untracked local git worktrees under.claude/and any stray venv.vendor/andchart/stay excluded as before.Behavior changes worth knowing
license-header-checkdoes not catch this either, since it only detects absence. This is a real capability loss, not just a simplification.//; the existing ~570 keep/* */. Converting them is deliberately out of scope to avoid a PR touching every file, and it never converges on its own since addlicense will not rewrite an existing header. Happy to file a follow-up if you want consistency.Verification
make fmtfrom a coldbin/leaves the tree clean. The drift is gone; that was the point.make license-fmtis idempotent: a second run produces no further changes.make license-header-checkexits 2 with headers missing, 0 once present, so it works as a gate.go build ./...passes.make unit-testsnot run: no Go source changed except the generated file's build-tag position, which is semantically identical.