CNTRLPLANE-3791: make pre-commit hooks resilient to mock generation failures#9016
CNTRLPLANE-3791: make pre-commit hooks resilient to mock generation failures#9016bryan-cox wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references CNTRLPLANE-3791 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe pre-commit configuration separates API and main Go lint-fix hooks and allows later hooks to run after failures. Makefile lint targets now tolerate missing base references, conditionally pass revision options, and add Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
193-194: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the mock cleanup step in
generate
Removing it can leave stale*_mock.gofiles behind when interfaces are renamed or deleted, becausego generate ./...won’t prune orphaned outputs.🤖 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 `@Makefile` around lines 193 - 194, Restore the mock cleanup step in the Makefile generate target before running $(GO) generate ./..., ensuring stale *_mock.go files are removed when interfaces change while preserving the existing mock generation command.
🧹 Nitpick comments (3)
Makefile (1)
127-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
lint-fixduplicates themain-lint-fixcommand instead of reusing it.Now that
main-lint-fix(Lines 127-129) encapsulates the exact main-module fix command,lint-fix(Lines 132-135) could call$(MAKE) main-lint-fixinstead of repeating the raw$(GOLANGCI_LINT) run --config ./.golangci.yml --fix -vinvocation, avoiding two places to keep in sync.♻️ Proposed refactor
lint-fix: generate - $(MAKE) api-lint-fix; api_rc=$$?; \ - $(GOLANGCI_LINT) run --config ./.golangci.yml --fix -v; main_rc=$$?; \ + $(MAKE) api-lint-fix; api_rc=$$?; \ + $(MAKE) main-lint-fix; main_rc=$$?; \ exit $$(( api_rc > main_rc ? api_rc : main_rc ))🤖 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 `@Makefile` around lines 127 - 135, Update the lint-fix target to invoke $(MAKE) main-lint-fix instead of duplicating the raw golangci-lint command, while preserving the existing api_rc/main_rc status aggregation and exit behavior. Keep main-lint-fix as the single definition of the main-module fix command..pre-commit-config.yaml (2)
38-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHook names/descriptions understate actual scope.
Both hooks are named/described as sorting imports, but they run
golangci-lint run --fix, which applies fixes for every enabled linter/formatter with autofix support, not just import sorting. Consider renaming/describing them to reflect the broader auto-fix behavior so contributors aren't surprised by unrelated changes appearing in their diffs.Also applies to: 45-45
🤖 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 @.pre-commit-config.yaml at line 38, Rename and update the descriptions of both hooks around “Sort API imports” and the corresponding hook at the additional location to reflect that they run golangci-lint autofixes broadly, not only import sorting. Ensure the wording clearly communicates that enabled linter and formatter fixes may modify unrelated code.
37-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHook
filespatterns don't excludevendor/.
api-lint-fix(^api/.*\.go$) andmain-lint-fix(\.go$excluding^api/) will also match staged.gofiles undervendor/(main module) orapi/vendor/(API module), if such files are ever staged (e.g. aftergo mod vendor). DEVELOPMENT.md explicitly states vendor directories should not be modified directly and are managed bymake update; auto-fixing vendored files here would work against that guidance.♻️ Proposed fix
- id: api-lint-fix ... files: '^api/.*\.go$' + exclude: '(^|/)vendor/' - id: main-lint-fix ... files: '\.go$' - exclude: '^api/' + exclude: '(^api/|(^|/)vendor/)'🤖 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 @.pre-commit-config.yaml around lines 37 - 51, Update the files patterns for the api-lint-fix and main-lint-fix hooks so they exclude vendor directories, including api/vendor/ and the main module’s vendor/. Preserve the existing module separation and ensure staged Go files under any vendor path are not auto-fixed.
🤖 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.
Outside diff comments:
In `@Makefile`:
- Around line 193-194: Restore the mock cleanup step in the Makefile generate
target before running $(GO) generate ./..., ensuring stale *_mock.go files are
removed when interfaces change while preserving the existing mock generation
command.
---
Nitpick comments:
In @.pre-commit-config.yaml:
- Line 38: Rename and update the descriptions of both hooks around “Sort API
imports” and the corresponding hook at the additional location to reflect that
they run golangci-lint autofixes broadly, not only import sorting. Ensure the
wording clearly communicates that enabled linter and formatter fixes may modify
unrelated code.
- Around line 37-51: Update the files patterns for the api-lint-fix and
main-lint-fix hooks so they exclude vendor directories, including api/vendor/
and the main module’s vendor/. Preserve the existing module separation and
ensure staged Go files under any vendor path are not auto-fixed.
In `@Makefile`:
- Around line 127-135: Update the lint-fix target to invoke $(MAKE)
main-lint-fix instead of duplicating the raw golangci-lint command, while
preserving the existing api_rc/main_rc status aggregation and exit behavior.
Keep main-lint-fix as the single definition of the main-module fix command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4708708c-1e4f-4cd5-ba07-d7209768157a
📒 Files selected for processing (3)
.pre-commit-config.yamlDEVELOPMENT.mdMakefile
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9016 +/- ##
==========================================
+ Coverage 44.11% 44.13% +0.01%
==========================================
Files 772 772
Lines 96226 96255 +29
==========================================
+ Hits 42448 42478 +30
+ Misses 50832 50831 -1
Partials 2946 2946 see 6 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/area ci-tooling |
openshift-ci
Bot
added
area/ci-tooling
Remove the destructive find-delete of *_mock.go files from the generate target — go generate overwrites in place, so deleting first creates a fragile window where any failure leaves all mocks gone. Split the monolithic make-lint-fix pre-commit hook into two scoped hooks (api-lint-fix and main-lint-fix) so API import reordering doesn't fire on unrelated commits, and add the missing $(GOLANGCI_LINT) prerequisite to the new main-lint-fix target. Make PULL_BASE_SHA fall back gracefully: upstream remote/main → local main → empty (lint all files). When empty, --new-from-rev is omitted so api-lint targets work in any environment. Change fail_fast from true to false so all hook problems are visible in one pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
06e2b44 to
fa3dde5
Compare
|
@bryan-cox: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Local Verification ResultsSix tests verifying each change in this PR. All output captured from local runs. Test 1: Mock file survival through
|
2 participants
What this PR does / why we need it:
The periodic review agent fails when it modifies test files that depend on generated mocks. The root cause is three interacting issues in the repo's build tooling:
generatetarget destructively deletes all*_mock.gofiles before regenerating — ifgo generatefails for any reason, all mocks are gone and nothing compilesPULL_BASE_SHAresolution fails in environments without anopenshift/hypershiftremote, causingapi-lint-fixto error outfail_fast: trueaborts all hooks on the first failure, leaving the working tree in a partially-modified stateThis PR fixes all three:
find -deletefromgeneratetarget —go generateoverwrites mock files in place, so deleting first is unnecessary and fragilePULL_BASE_SHAfall back gracefully — upstream remote/main → local main → empty (omit--new-from-rev, lint all files)make-lint-fixinto two scoped hooks —api-lint-fix(API Go files only) andmain-lint-fix(main module Go files only), so API import reordering doesn't fire on unrelated commitsfail_fasttofalse— all hooks run so all problems are visible in one passWhich issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/CNTRLPLANE-3791
Special notes for your reviewer:
The Jira description originally cited
git clean -fx -- '*_mock.go'in a pre-commit hook — this command doesn't exist in the repo. The actual destructive step isfind . -name '*_mock.go' -deletein the Makefilegeneratetarget, which is transitively invoked by the pre-commit hook viamake lint-fix→make generate.The
main-lint-fixtarget includes$(GOLANGCI_LINT)as a prerequisite so it works on fresh checkouts where the binary hasn't been built yet.Checklist:
Summary by CodeRabbit
Documentation
make generateregenerates mock files in place and that generated files shouldn’t be edited manually.Developer Experience