Update golangci-lint to v2.11.4 and enable new linters#301
Update golangci-lint to v2.11.4 and enable new linters#301saschagrunert wants to merge 1 commit intocri-o:mainfrom
Conversation
openshift-ci
Bot
added
release-note-none
openshift-ci
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert 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
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe pull request expands the active linter set in golangci-lint configuration, upgrades the tool version to v2.11.4, and updates codebase patterns to comply with new linters. Changes include context propagation for improved cancellation support, magic number extraction to named constants, JSON tag annotation suppressions for tagliatelle compliance, struct field reordering, and modernization of Go build tags. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 45 minutes and 50 seconds.Comment |
saschagrunert
force-pushed
the
update-golangci-lint-v2.11.4
branch
from
e2b9ae8 to
b9bd8cb
Compare
|
@cri-o/cri-o-maintainers PTAL |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tools/ocicnitool/ocicnitool.go (3)
14-23: ⚡ Quick winKeep
minRequiredArgstightly coupled to positional arg indexing.
minRequiredArgs = 5is used to guard the later indexing (flag.Args()[0]..[4]). The current value matches the switch dispatch + 4 positional fields, so it looks correct.To reduce future drift, consider deriving it from the indexing intent (e.g.,
const minRequiredArgs = 1 /*cmd*/ + 4 /*pod_* */) or adding a short comment tying it topodNetworkfields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/ocicnitool/ocicnitool.go` around lines 14 - 23, minRequiredArgs is brittle because it's used to guard indexing into flag.Args()[0]..[4]; change it so the intent is explicit by either computing it from the positional pieces (e.g., minRequiredArgs := 1 /* cmd */ + 4 /* podNetwork fields */) or add a short comment next to the constant tying it to the podNetwork positional fields and the switch dispatch (referencing minRequiredArgs and flag.Args() usage) so future edits won’t drift the required count.
102-119: ⚡ Quick winHandle unknown commands explicitly (missing
defaultin switch).The
switch flag.Args()[0]has cases forCmdAdd,CmdStatus, andCmdDel, but nodefault. If a user passes an unknown command,main()will fall through to the end and exit successfully without showing usage.Add a
defaultthat callsflag.Usage()and exits non-zero.🛠️ Proposed change
switch flag.Args()[0] { case CmdAdd: results, err := plugin.SetUpPod(podNetwork) if err == nil { printSandboxResults(results) } exit(err) case CmdStatus: results, err := plugin.GetPodNetworkStatus(podNetwork) if err == nil { printSandboxResults(results) } exit(err) case CmdDel: exit(plugin.TearDownPod(podNetwork)) + default: + flag.Usage() + os.Exit(2) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/ocicnitool/ocicnitool.go` around lines 102 - 119, The switch over flag.Args()[0] currently handles CmdAdd, CmdStatus, and CmdDel but lacks a default branch, so unknown commands silently fall through; add a default case in the switch (alongside the existing cases for CmdAdd, CmdStatus, CmdDel) that calls flag.Usage() and then exits non-zero (e.g., exit(fmt.Errorf("unknown command"))) so the program reports usage and returns an error when an unrecognized command is passed.
68-72: ⚡ Quick winUsage/validation failure currently exits with code 0.
When
len(flag.Args()) < minRequiredArgs, the code callsflag.Usage()and thenreturn, which makes the process exit with status0. For CLI tools, “wrong invocation” typically should exit non-zero (commonly2).🛠️ Proposed change
if len(flag.Args()) < minRequiredArgs { flag.Usage() - - return + os.Exit(2) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/ocicnitool/ocicnitool.go` around lines 68 - 72, The CLI currently calls flag.Usage() and returns when len(flag.Args()) < minRequiredArgs, causing a zero exit status; update the failure path in the same function (where flag.Args(), minRequiredArgs and flag.Usage() are used) to call os.Exit(2) after printing usage so the process exits non-zero on invalid invocation (importing os if not already present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/ocicni/types.go`:
- Around line 116-123: The exported struct NetResult had its exported fields
reordered, which can break downstream positional composite literals; revert the
struct field order to the original order (restore the exact sequence of
NetResult's fields so NetAttachment and Result appear in the original order),
ensure the NetResult definition uses the previous ordering of NetAttachment and
Result, and avoid reordering fields of exported structs in future changes (refer
to the NetResult type and its embedded NetAttachment and Result fields when
making the fix).
In `@pkg/ocicni/util_linux.go`:
- Around line 77-80: The error message reports the wrong line when link parsing
fails: parsing uses lines[1] but the fmt.Errorf references lines[0]; update the
error construction in the link-parse branch (the block that computes fields =
strings.Fields(lines[1]) and checks len(fields) < minLinkFields) to include
lines[1] (or otherwise the offending content) instead of lines[0] so the logged
content matches the parsed input; ensure the change is made where fields,
minLinkFields and the fmt.Errorf call are used.
---
Nitpick comments:
In `@tools/ocicnitool/ocicnitool.go`:
- Around line 14-23: minRequiredArgs is brittle because it's used to guard
indexing into flag.Args()[0]..[4]; change it so the intent is explicit by either
computing it from the positional pieces (e.g., minRequiredArgs := 1 /* cmd */ +
4 /* podNetwork fields */) or add a short comment next to the constant tying it
to the podNetwork positional fields and the switch dispatch (referencing
minRequiredArgs and flag.Args() usage) so future edits won’t drift the required
count.
- Around line 102-119: The switch over flag.Args()[0] currently handles CmdAdd,
CmdStatus, and CmdDel but lacks a default branch, so unknown commands silently
fall through; add a default case in the switch (alongside the existing cases for
CmdAdd, CmdStatus, CmdDel) that calls flag.Usage() and then exits non-zero
(e.g., exit(fmt.Errorf("unknown command"))) so the program reports usage and
returns an error when an unrecognized command is passed.
- Around line 68-72: The CLI currently calls flag.Usage() and returns when
len(flag.Args()) < minRequiredArgs, causing a zero exit status; update the
failure path in the same function (where flag.Args(), minRequiredArgs and
flag.Usage() are used) to call os.Exit(2) after printing usage so the process
exits non-zero on invalid invocation (importing os if not already present).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78021350-d11a-4ca5-a1c0-5bdd24add983
📒 Files selected for processing (8)
.golangci.ymlMakefilepkg/ocicni/ocicni.gopkg/ocicni/ocicni_test.gopkg/ocicni/types.gopkg/ocicni/types_unix.gopkg/ocicni/util_linux.gotools/ocicnitool/ocicnitool.go
💤 Files with no reviewable changes (1)
- pkg/ocicni/types_unix.go
saschagrunert
force-pushed
the
update-golangci-lint-v2.11.4
branch
from
b9bd8cb to
5e6c833
Compare
Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
saschagrunert
force-pushed
the
update-golangci-lint-v2.11.4
branch
from
5e6c833 to
17751d7
Compare
1 participant
/kind cleanup
What this PR does / why we need it:
Bumps golangci-lint from v2.0.1 to v2.11.4 and enables 13 new linters. Fixes all reported lint issues across the codebase.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?
Summary by CodeRabbit
Chores
Refactor