no-jira: Golangci config#10372
Conversation
Prealloc's docs state that using it is an overoptimization. It was causing some failures in ci rehearsal testing, so let's drop it.
We are frequently seeing a flake for package types:
pkg/types/utils_test.go:1:9: var-naming: avoid meaningless package names (revive)
package types
^
Disable this specific linter to avoid these flakes.
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request explicitly references no jira issue. 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. |
WalkthroughModified golangci-lint configuration by removing the prealloc linter from global rules and adding a targeted revive linter override for test files with a var-naming check to enforce meaningful package names in test code. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci-lint-v2.yaml:
- Around line 90-92: The revive exclusion currently listed (linters: revive,
text: "var-naming: avoid meaningless package names") is global; scope it to test
files by adding a path regex that matches _test.go files (e.g. path:
'.*_test\.go$') so the suppression only applies to test-only files; update the
exclusion block containing the revive rule to include that path field while
keeping the existing linters and text entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef9a81a4-bc52-43ba-a586-61375ba07817
📒 Files selected for processing (1)
.golangci-lint-v2.yaml
| - linters: | ||
| - revive | ||
| text: "var-naming: avoid meaningless package names" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In golangci-lint v2, does a linters.exclusions.rulesentry with onlylintersandtextexclude matching issues globally? How should it be scoped so it only applies to_test.go files?
💡 Result:
Yes. In golangci-lint v2, a linters.exclusions.rules item that only specifies linters and text (and no path / path-except) matches all files, so it excludes matching issues “globally” for the selected linters. [1]
To scope it to _test.go files, add a path regex that matches test files, e.g.:
linters:
exclusions:
rules:
- path: '(.+)_test\.go'
linters: [mnd] # your linter(s)
text: "Magic number: 9" # your match text/regex(path-except is the inverse, i.e., apply the rule everywhere except matching paths.) [1]
Sources: [1] [2]
Scope this revive exclusion to _test.go files.
This exclusion rule specifies only linters and text without a path constraint, which makes it apply globally to all files. In golangci-lint v2, test-only scoping requires adding a path regex. As written, the rule suppresses this warning in all packages, not just test files, which is broader than the PR objective.
Suggested fix
- linters:
- revive
text: "var-naming: avoid meaningless package names"
+ - path: _test\.go
+ linters:
+ - revive
+ text: "var-naming: avoid meaningless package names"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci-lint-v2.yaml around lines 90 - 92, The revive exclusion currently
listed (linters: revive, text: "var-naming: avoid meaningless package names") is
global; scope it to test files by adding a path regex that matches _test.go
files (e.g. path: '.*_test\.go$') so the suppression only applies to test-only
files; update the exclusion block containing the revive rule to include that
path field while keeping the existing linters and text entries.
There was a problem hiding this comment.
No, we want to exclude this warning from all packages. This change is correct as intended :D
There was a problem hiding this comment.
@tthvo, understood! Thanks for the clarification — suppressing var-naming: avoid meaningless package names globally makes sense since the warning can surface in any package, not just test files. The change is correct as intended. My apologies for the noise!
📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tthvo 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
|
/verified by ci/prow/golint |
openshift-ci-robot
added
the
verified
|
@tthvo: This PR has been marked as verified by 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. |
|
👍 There's also extensive duplicate linter config in https://github.com/openshift/installer/blob/main/.golangci.yaml - do we know if that gets used for anything any more? Does it need to match, or can it just be deleted at this point? |
Oh no, that's the v1 golangci config, which can be safely removed now. We are using v2 golangci lint :D |
|
/retest-required |
1 similar comment
|
/retest-required |
|
@patrickdillon: 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. |
4 participants
openshift/release#75091 which is needed to get in go1.25 is showing failures for prealloc. @zaneb pointed out that we don't need it, so let's drop it.
Also let's try to avoid the flakes for var-naming that we're seeing all the time:
#10319 (comment)
Summary by CodeRabbit