CNF-23437: Wrap bare error returns for better debuggability - #6319
CNF-23437: Wrap bare error returns for better debuggability#6319sebrandon1 wants to merge 1 commit into
Conversation
|
@sebrandon1: This pull request references CNF-23437 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. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds contextual ChangesError wrapping changes
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 `@internal/clusterconfig/clusterconfig.go`:
- Around line 77-78: FetchClusterConfig wraps fetchIDMS but fetchIDMS currently
drops the original error; update fetchIDMS to return a wrapped error using
fmt.Errorf with %w (e.g., fmt.Errorf("failed to fetch IDMS: %w", err)) so the
original error chain is preserved when FetchClusterConfig returns. Locate the
fetchIDMS function and replace the plain error construction that loses the root
cause with a wrapped error that includes the original err.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: dbf7ea15-a7cc-4875-9301-a649b953df30
📒 Files selected for processing (5)
controllers/ipc_config_handlers.gocontrollers/ipc_idle_handlers.gocontrollers/ipc_rollback_handlers.gointernal/clusterconfig/clusterconfig.gointernal/clusterconfig/lvmconfig.go
|
/retest ci/prow/ipc-e2e-flow |
|
/retest |
1 similar comment
|
/retest |
|
Actionable comments posted: 0 |
75cb0ed to
922193d
Compare
64aa7a9 to
0a6501e
Compare
|
@sebrandon1: This pull request references CNF-23437 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 story 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. |
be75669 to
e653a5d
Compare
b4ffde1 to
0e46dcb
Compare
4d9fab3 to
0a0392d
Compare
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 `@controllers/ipc_config_handlers_test.go`:
- Around line 1431-1434: Update the error assertions at
controllers/ipc_config_handlers_test.go:1431-1434 and 1444-1447, and
internal/clusterconfig/clusterconfig_test.go:754-756, to fail immediately when
the expected error is nil before accessing err.Error(); add the appropriate test
helper/fatal nil checks, then keep each test focused on asserting the immediate
inner error details rather than only the wrapper message.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c245cdf-b6ea-48c7-b0bb-0c9383f16a69
📒 Files selected for processing (7)
controllers/ipc_config_handlers.gocontrollers/ipc_config_handlers_test.gocontrollers/ipc_idle_handlers.gocontrollers/ipc_rollback_handlers.gointernal/clusterconfig/clusterconfig.gointernal/clusterconfig/clusterconfig_test.gointernal/clusterconfig/lvmconfig.go
🚧 Files skipped from review as they are similar to previous changes (4)
- controllers/ipc_rollback_handlers.go
- controllers/ipc_config_handlers.go
- internal/clusterconfig/lvmconfig.go
- controllers/ipc_idle_handlers.go
08a6c3a to
1946e9b
Compare
|
@sebrandon1: This pull request references CNF-23437 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 story 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. |
8d6f833 to
f96218e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/clusterconfig/clusterconfig_test.go (1)
754-756: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify the error unwrap contract.
These tests only verify context text. They also pass if production code changes
%wto%v. Add anerrors.Unwrapassertion, or inject a sentinel client error and useassert.ErrorIs.
internal/clusterconfig/clusterconfig_test.go#L754-L756: Verify that thefetchIDMSerror unwraps.internal/clusterconfig/clusterconfig_test.go#L772-L774: Verify that thefetchICSPserror unwraps.internal/clusterconfig/clusterconfig_test.go#L791-L793: Verify the nestedFetchClusterConfigerror chain.internal/clusterconfig/lvmconfig_test.go#L228-L230: Verify that theFetchLvmConfigerror unwraps.🤖 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 `@internal/clusterconfig/clusterconfig_test.go` around lines 754 - 756, Update the error-path tests to verify wrapped error identity, not only context text: in internal/clusterconfig/clusterconfig_test.go lines 754-756 for fetchIDMS, lines 772-774 for fetchICSPs, and lines 791-793 for the nested FetchClusterConfig chain, inject sentinel client errors and assert ErrorIs (or errors.Unwrap); likewise update internal/clusterconfig/lvmconfig_test.go lines 228-230 for FetchLvmConfig. Preserve the existing message assertions while confirming each returned error unwraps to its originating sentinel.
🤖 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.
Nitpick comments:
In `@internal/clusterconfig/clusterconfig_test.go`:
- Around line 754-756: Update the error-path tests to verify wrapped error
identity, not only context text: in internal/clusterconfig/clusterconfig_test.go
lines 754-756 for fetchIDMS, lines 772-774 for fetchICSPs, and lines 791-793 for
the nested FetchClusterConfig chain, inject sentinel client errors and assert
ErrorIs (or errors.Unwrap); likewise update
internal/clusterconfig/lvmconfig_test.go lines 228-230 for FetchLvmConfig.
Preserve the existing message assertions while confirming each returned error
unwraps to its originating sentinel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae99bd09-d7a7-4c63-9d36-ddc036195895
📒 Files selected for processing (5)
controllers/ipc_config_handlers_test.gocontrollers/ipc_idle_handlers_test.gointernal/clusterconfig/clusterconfig.gointernal/clusterconfig/clusterconfig_test.gointernal/clusterconfig/lvmconfig_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/clusterconfig/clusterconfig.go
|
/retest |
a626b78 to
5a6e26f
Compare
|
/retest |
3e9cbb2 to
7e08193
Compare
bd5e067 to
4d413b9
Compare
|
/retest |
Wrap 20 bare `return err` statements with fmt.Errorf context in controllers/ and internal/clusterconfig/ so that errors surfacing at the controller level carry the full call chain for easier debugging in logs.
4d413b9 to
46fee08
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
@sebrandon1: 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. |
3 participants
Summary
return errstatements withfmt.Errorf("context: %w", err)across 5 files incontrollers/andinternal/clusterconfig/fetchIDMSwherefmt.Errorf("failed to fetch IDMS")discarded the actual error -- now properly wraps with%w%wsoerrors.Is/errors.Aschains are preservedRelated PRs
Test plan
go build ./...compilesgo test ./controllers/... ./internal/clusterconfig/...all passmake golangci-lint0 issuesvalidateAddressChanges,cleanuoUnbootedStateroots,fetchIDMS,fetchICSPs,FetchClusterConfig,FetchLvmConfigJira: CNF-23437 (epic: CNF-23510)