fix(PLT-6661): manage finalizers by using update instead of apply - #36
Conversation
📝 WalkthroughWalkthroughThe release reconciler now persists prune-release finalizer changes with standard updates. End-to-end tests use ChangesRelease persistence and test applications
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/operator/reconciler_release.go`:
- Around line 89-90: Update the error message in the releaseIntf.Update error
path to say “failed to update” instead of “failed to apply,” while preserving
the existing wrapped error and reconciliation behavior.
- Around line 89-95: Update the deletion cleanup branch that removes
finalizerPruneRelease to call releaseIntf.Update with
metav1.UpdateOptions{FieldManager: joyOperator} instead of releaseIntf.Apply
with Force: true. Keep the existing finalizer removal and error handling
unchanged, and ensure both cleanup paths use Update.
🪄 Autofix
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 Plus
Run ID: 4417f291-0668-4bfe-9cea-9ca0b25325cb
📒 Files selected for processing (2)
cmd/operator/main_test.gocmd/operator/reconciler_release.go
| if _, err := releaseIntf.Update(ctx, release, metav1.UpdateOptions{FieldManager: joyOperator}); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to apply prune-release finalizer: %w", err) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the error message.
Line 89 now performs an update, but Line 90 still reports “failed to apply”. Change the message to “failed to update” so reconciliation errors identify the actual operation.
🤖 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/operator/reconciler_release.go` around lines 89 - 90, Update the error
message in the releaseIntf.Update error path to say “failed to update” instead
of “failed to apply,” while preserving the existing wrapped error and
reconciliation behavior.
a8cedef to
80af8e1
Compare
This PR changes the finalizer update operations to use update instead of apply.
This is a simple way of not fighting for fieldManagement ownership and keeping the joy-operators field management restrained to the finalizer.
Perhaps a JSON patch approach would work equally well but this is simpler to implement.
Summary by CodeRabbit