chore: remove --new-from-rev scoping from api-lint to prove pre-exist…#9063
chore: remove --new-from-rev scoping from api-lint to prove pre-exist…#9063stevekuznetsov wants to merge 1 commit into
Conversation
…ing failures The api-lint and api-lint-fix targets use --new-from-rev=$(PULL_BASE_SHA) --whole-files, which scopes linting to files changed since the base SHA. On main this means zero files are scanned, hiding 50 pre-existing kube-api-linter violations in hosted_controlplane.go and hostedcluster_types.go. Any PR that touches either of those files triggers the full-file scan, and the auto-fixer produces broken code (e.g. +kubebuilder:default → +default, +kubebuilder:validation:Pattern → +kubebuilder:validation:XValidation), causing make lint-fix to fail. This commit removes the scoping to expose the issue in CI. Signed-off-by: Steve Kuznetsov <stekuznetsov@microsoft.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Please specify an area label 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. |
📝 WalkthroughWalkthroughThe Makefile’s Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/pipeline required |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stevekuznetsov The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@Makefile`:
- Around line 115-116: Update the api-lint-fix target to pass
--new-from-rev=$(PULL_BASE_SHA) --whole-files to the golangci-lint invocation,
ensuring fixes are limited to files changed from the pull request base while
checking complete files. Keep the existing api directory, configuration, fix
mode, and verbosity 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 492ff18c-32c5-474d-8804-0775ffc42650
📒 Files selected for processing (1)
Makefile
| api-lint-fix: $(GOLANGCI_LINT) $(KUBEAPILINTER_PLUGIN) | ||
| cd api && $(GOLANGCI_LINT) run --config ./.golangci.yml --fix -v $(if $(PULL_BASE_SHA),--new-from-rev=$(PULL_BASE_SHA) --whole-files) | ||
| cd api && $(GOLANGCI_LINT) run --config ./.golangci.yml --fix -v |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant files ==\n'
git ls-files 'Makefile' '.pre-commit-config.yaml' | sed 's#^`#-` #'
printf '\n== Makefile excerpt ==\n'
nl -ba Makefile | sed -n '105,125p'
printf '\n== pre-commit excerpt ==\n'
nl -ba .pre-commit-config.yaml | sed -n '30,55p'Repository: openshift/hypershift
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search for api-lint-fix usage ==\n'
rg -n "api-lint-fix|PULL_BASE_SHA|new-from-rev|whole-files|--fix" -S .
printf '\n== golangci-lint docs/config mentions in repo ==\n'
rg -n "new-from-rev|whole-files|golangci-lint" -S README* docs . 2>/dev/null || trueRepository: openshift/hypershift
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Makefile lines 120-130 ==\n'
sed -n '120,130p' Makefile | cat -n
printf '\n== .pre-commit-config.yaml lines 37-44 ==\n'
sed -n '37,44p' .pre-commit-config.yaml | cat -n
printf '\n== Makefile api-lint-fix context ==\n'
sed -n '112,118p' Makefile | cat -nRepository: openshift/hypershift
Length of output: 1445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== golangci-lint flag docs in vendored source ==\n'
sed -n '120,145p' hack/tools/vendor/github.com/golangci/golangci-lint/v2/pkg/commands/flagsets.go | cat -n
printf '\n== any local docs for api-lint-fix ==\n'
sed -n '1,40p' docs/content/contribute/precommit-hook-help.md | cat -nRepository: openshift/hypershift
Length of output: 4843
Scope api-lint-fix to the diff. This target ignores PULL_BASE_SHA, so the pre-commit hook can rewrite any file under api/ instead of only the staged change. Pass --new-from-rev=$(PULL_BASE_SHA) --whole-files through here, or split out a separate full-tree fix target.
🤖 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 115 - 116, Update the api-lint-fix target to pass
--new-from-rev=$(PULL_BASE_SHA) --whole-files to the golangci-lint invocation,
ensuring fixes are limited to files changed from the pull request base while
checking complete files. Keep the existing api directory, configuration, fix
mode, and verbosity behavior unchanged.
|
The I surmise what happened here is that from I would really recommend not trying to be clever in CI - it's really difficult to capture all possible inputs that make something fail and correctly map them to which subset needs to be validated. Just run the full thing. |
Test Resultse2e-aws
Failed TestsTotal failed tests: 13
... and 8 more failed tests e2e-aks
|
|
@stevekuznetsov: The following tests failed, say
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. |
|
FWIW, we have had no issues with |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
3 participants
…ing failures
The api-lint and api-lint-fix targets use --new-from-rev=$(PULL_BASE_SHA) --whole-files, which scopes linting to files changed since the base SHA. On main this means zero files are scanned, hiding 50 pre-existing kube-api-linter violations in hosted_controlplane.go and hostedcluster_types.go.
Any PR that touches either of those files triggers the full-file scan, and the auto-fixer produces broken code (e.g. +kubebuilder:default → +default, +kubebuilder:validation:Pattern → +kubebuilder:validation:XValidation), causing make lint-fix to fail.
This commit removes the scoping to expose the issue in CI.
Summary by CodeRabbit