fix: replaced requeue with error retry #108
Conversation
On-behalf-of: SAP [email protected]
WalkthroughUpdates a workspace initializer to return an OperatorError instead of a requeue when the store ID is empty. Adds a go.mod replace directive to pin github.com/kcp-dev/multicluster-provider v0.2.0 to a specific pseudo-version. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
…tion On-behalf-of: SAP [email protected]
On-behalf-of: SAP [email protected]
| if store.Status.StoreID == "" { | ||
| // Store is not ready yet, requeue | ||
| return ctrl.Result{Requeue: true}, nil | ||
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("the store ID is empty, need restart"), true, true) |
There was a problem hiding this comment.
This is an expected situation, we should not notify sentry here
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/operator.go(0 hunks)go.mod(1 hunks)internal/controller/initializer_controller.go(1 hunks)internal/subroutine/workspace_initializer.go(1 hunks)
💤 Files with no reviewable changes (1)
- cmd/operator.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / lint / lint
🔇 Additional comments (2)
internal/controller/initializer_controller.go (1)
49-49: LGTM! Improved label consistency.The lifecycle identifier now matches the reconciler struct name, improving clarity and maintainability.
go.mod (1)
5-6: Verify the replace directive is documented.The replace directive pins
multicluster-providerto a specific pseudo-version instead of the released v0.2.0. This typically indicates a workaround for a bug, dependency on an unreleased feature, or waiting for a new release.Please confirm:
- Is this a temporary workaround or permanent change?
- What specific issue or feature requires this pseudo-version?
- Is there a tracking issue or documentation for this change?
Consider adding a comment above the replace directive explaining why it's needed and when it can be removed (if temporary).
| if store.Status.StoreID == "" { | ||
| // Store is not ready yet, requeue | ||
| return ctrl.Result{Requeue: true}, nil | ||
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("store id is empty: %w", err), true, false) | ||
| } |
There was a problem hiding this comment.
Fix incorrect error wrapping and respect past feedback on Sentry notifications.
This implementation has two critical issues:
-
Wrapping nil error: The
errvariable being wrapped at line 94 is from theCreateOrUpdateoperation (lines 88-90). Since we only reach line 94 whenerr == nil(the check at line 88 passed), wrappingerrwith%wcreates a misleading error message that wraps a nil error. -
Contradicts past review feedback: A previous review comment states "This is an expected situation, we should not notify sentry here". The second parameter to
NewOperatorErroristrue, which likely enables Sentry notifications for this expected condition.
Apply this diff to fix both issues:
if store.Status.StoreID == "" {
- // Store is not ready yet, requeue
- return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("store id is empty: %w", err), true, false)
+ // Store is not ready yet, retry without Sentry notification (expected condition)
+ return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("store id is empty"), false, false)
}This change:
- Removes the incorrect
%wwrapping of a nilerr - Sets the Sentry notification flag to
falseto align with previous feedback - Updates the comment to clarify this is an expected condition
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if store.Status.StoreID == "" { | |
| // Store is not ready yet, requeue | |
| return ctrl.Result{Requeue: true}, nil | |
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("store id is empty: %w", err), true, false) | |
| } | |
| if store.Status.StoreID == "" { | |
| // Store is not ready yet, retry without Sentry notification (expected condition) | |
| return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("store id is empty"), false, false) | |
| } |
🤖 Prompt for AI Agents
In internal/subroutine/workspace_initializer.go around lines 92 to 95, the code
incorrectly wraps a nil error (using fmt.Errorf(... %w, err)) and sets the
NewOperatorError Sentry flag to true; change the error construction to remove
the %w wrapping and pass a plain error message (no wrapped err), set the
Sentry/notification boolean to false, and update the comment to state this is an
expected condition so we do not notify Sentry.
nexus49
left a comment
nexus49
left a comment
There was a problem hiding this comment.
As discussed, we'll create a follow on ticket ro remove the replace statement once the new version of the multicluster-provider has been released
On-behalf-of: SAP [email protected]
Summary by CodeRabbit
New Features
Bug Fixes
Chores