ROSAENG-61040 | refactor: remove github.com/pkg/errors#3421
Conversation
Replace 3 errors.Wrapf calls with fmt.Errorf using the %w verb. The pkg/errors library is deprecated by its author in favor of standard error wrapping.
📝 WalkthroughWalkthroughThe machine pool upgrade command replaces Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: markirish 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: 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 `@cmd/upgrade/machinepool/cmd.go`:
- Line 132: Use the registered flag name consistently: update the validation
error in cmd/upgrade/machinepool/cmd.go at lines 132-132 to reference
--allow-minor-version-updates, and update the expected substring in
cmd/upgrade/machinepool/cmd_test.go at lines 66-67 to match.
- Line 156: Update the five contextual error returns in the machinepool command,
including the one around “failed to get machine pools for hosted cluster” and
the corresponding locations, to wrap the underlying error with %w instead of
interpolating it with %s or %v. Preserve each existing message context and error
arguments so callers can use errors.Is and errors.As.
- Line 197: Update the error message in the machinepool upgrade command to
replace the grammatically incorrect “expected an choice” wording with “expected
a choice” or “expected a choice of versions to target,” while preserving the
existing error detail.
🪄 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: Pro Plus
Run ID: 19777076-380e-4244-9d2c-6068111d99a4
📒 Files selected for processing (3)
cmd/upgrade/machinepool/cmd.gocmd/upgrade/machinepool/cmd_test.gogo.mod
| // Check parameters preconditions | ||
| if currentUpgradeScheduling.Schedule == "" && currentUpgradeScheduling.AllowMinorVersionUpdates { | ||
| return fmt.Errorf("The '--allow-minor-version-upgrades' option needs to be used with --schedule") | ||
| return fmt.Errorf("the '--allow-minor-version-upgrades' option needs to be used with --schedule") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use --allow-minor-version-updates consistently.
The command registers and reads allow-minor-version-updates, but the validation error and its test expectation say --allow-minor-version-upgrades, producing an unusable remediation hint.
cmd/upgrade/machinepool/cmd.go#L132-L132: change the production error to--allow-minor-version-updates.cmd/upgrade/machinepool/cmd_test.go#L66-L67: update the expected substring to the same registered flag name.
📍 Affects 2 files
cmd/upgrade/machinepool/cmd.go#L132-L132(this comment)cmd/upgrade/machinepool/cmd_test.go#L66-L67
🤖 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 `@cmd/upgrade/machinepool/cmd.go` at line 132, Use the registered flag name
consistently: update the validation error in cmd/upgrade/machinepool/cmd.go at
lines 132-132 to reference --allow-minor-version-updates, and update the
expected substring in cmd/upgrade/machinepool/cmd_test.go at lines 66-67 to
match.
| _, exists, err := r.OCMClient.GetNodePool(cluster.ID(), machinePoolID) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to get machine pools for hosted cluster '%s': %v", clusterKey, err) | ||
| return fmt.Errorf("failed to get machine pools for hosted cluster '%s': %v", clusterKey, err) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the underlying errors with %w.
%s and %v interpolate the cause into text, so callers cannot use errors.Is/errors.As and the original error context is lost. Replace these verbs with %w at all five locations.
As per coding guidelines, returned errors must be wrapped with context using %w; the PR objective also explicitly requires %w when replacing errors.Wrapf.
Proposed fix
- return fmt.Errorf("failed to get machine pools for hosted cluster '%s': %v", clusterKey, err)
+ return fmt.Errorf("failed to get machine pools for hosted cluster '%s': %w", clusterKey, err)
- return fmt.Errorf("expected an upgrade type: %s", err)
+ return fmt.Errorf("expected an upgrade type: %w", err)
- return fmt.Errorf("expected an choice on the versions to target: %s", err)
+ return fmt.Errorf("expected a choice of versions to target: %w", err)
- return "", fmt.Errorf("expected a valid machine pool version from interactive prompt: %s", err)
+ return "", fmt.Errorf("expected a valid machine pool version from interactive prompt: %w", err)
- return "", fmt.Errorf("expected a valid machine pool version: %s", err)
+ return "", fmt.Errorf("expected a valid machine pool version: %w", err)Also applies to: 186-186, 197-197, 362-362, 369-369
🤖 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 `@cmd/upgrade/machinepool/cmd.go` at line 156, Update the five contextual error
returns in the machinepool command, including the one around “failed to get
machine pools for hosted cluster” and the corresponding locations, to wrap the
underlying error with %w instead of interpolating it with %s or %v. Preserve
each existing message context and error arguments so callers can use errors.Is
and errors.As.
Source: Coding guidelines
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("Expected an choice on the versions to target: %s", err) | ||
| return fmt.Errorf("expected an choice on the versions to target: %s", err) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the user-facing wording.
expected an choice is grammatically incorrect. Use expected a choice or expected a choice of versions to 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 `@cmd/upgrade/machinepool/cmd.go` at line 197, Update the error message in the
machinepool upgrade command to replace the grammatically incorrect “expected an
choice” wording with “expected a choice” or “expected a choice of versions to
target,” while preserving the existing error detail.
Source: Coding guidelines
|
@markirish: 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. |
1 participant
PR Summary
Replace 3
errors.Wrapfcalls withfmt.Errorfusing the%wverb. Thepkg/errorslibrary is deprecated by its author in favor of standard error wrapping.Detailed Description of the Issue
Our ROSA CLI project currently imports several third-party libraries that are only utilized in a single file (e.g., exclusively within a test file or for a single validation function). Relying on entire external dependencies for isolated use cases unnecessarily increases our security attack surface, complicates dependency management, and bloats the project.
This PR is part of that initiative to remove
github.com/pkg/errorsRelated Issues and PRs
Type of Change
Previous Behavior
Behavior After This Change
How to Test (Step-by-Step)
Preconditions
Test Steps
make testExpected Results
All tests pass
Proof of the Fix
All tests pass
Breaking Changes
Breaking Change Details / Migration Plan
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit