fix(controller): cascade Resource deletions and surface precise finalize statuses - #3917

internal/controller/resource/controller_finalize_test.go (1)

127-140: ⚡ Quick win

Assert that binding deletion was actually triggered in the blocking-path test.

The test proves the Resource finalizer is held, but it does not verify that cascade deletion was initiated on the owned ResourceReleaseBinding. Adding a DeletionTimestamp assertion on the binding makes this regression test stricter for the new active-delete behavior.

As per coding guidelines, "Add tests for critical logic or regressions."

Suggested test assertion

 		// 2nd reconcile: binding still present, requeue without acting.
 		result, err := r.Reconcile(finCtx, req)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(result.RequeueAfter).To(BeNumerically(">", 0))
+
+		updatedBinding := &openchoreov1alpha1.ResourceReleaseBinding{}
+		Expect(cli.Get(finCtx, client.ObjectKeyFromObject(binding), updatedBinding)).To(Succeed())
+		Expect(updatedBinding.DeletionTimestamp).NotTo(BeNil(),
+			"binding should be marked for deletion by resource finalizer")
 
 		updated := &openchoreov1alpha1.Resource{}
 		Expect(cli.Get(finCtx, client.ObjectKeyFromObject(res), updated)).To(Succeed())

🤖 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/controller/resource/controller_finalize_test.go` around lines 127 -
140, The test verifies that the Resource finalizer is held but does not verify
that cascade deletion was actually initiated on the owned
ResourceReleaseBinding. Add an assertion after the existing Resource status
checks to fetch the ResourceReleaseBinding object associated with the Resource
using cli.Get and the appropriate object key, then assert that the binding's
DeletionTimestamp is not nil to prove that cascade deletion was triggered on the
owned binding resource.

Source: Coding guidelines

internal/controller/resource/controller_finalize.go (1)

82-84: ⚡ Quick win

Guard status writes behind MarkTrueCondition change checks.

Line 82 and Line 95 call controller.UpdateStatusConditions even when the condition is unchanged. This can create unnecessary status-write traffic during every requeue loop.

As per coding guidelines, "Ensure status updates handle conflicts and avoid API spam."

Suggested patch

-		controller.MarkTrueCondition(res, ConditionFinalizing, ReasonFinalizing, "Waiting for ResourceReleaseBindings to be deleted")
-		if err := controller.UpdateStatusConditions(ctx, r.Client, old, res); err != nil {
-			return ctrl.Result{}, err
-		}
+		if controller.MarkTrueCondition(res, ConditionFinalizing, ReasonFinalizing, "Waiting for ResourceReleaseBindings to be deleted") {
+			if err := controller.UpdateStatusConditions(ctx, r.Client, old, res); err != nil {
+				return ctrl.Result{}, err
+			}
+		}
 		return ctrl.Result{RequeueAfter: requeueWaitForChildren}, nil
 	}
@@
-		controller.MarkTrueCondition(res, ConditionFinalizing, ReasonFinalizing, "Waiting for ResourceReleases to be deleted")
-		if err := controller.UpdateStatusConditions(ctx, r.Client, old, res); err != nil {
-			return ctrl.Result{}, err
-		}
+		if controller.MarkTrueCondition(res, ConditionFinalizing, ReasonFinalizing, "Waiting for ResourceReleases to be deleted") {
+			if err := controller.UpdateStatusConditions(ctx, r.Client, old, res); err != nil {
+				return ctrl.Result{}, err
+			}
+		}
 		return ctrl.Result{RequeueAfter: requeueWaitForChildren}, nil
 	}

Also applies to: 95-97

🤖 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/controller/resource/controller_finalize.go` around lines 82 - 84,
Guard the `controller.UpdateStatusConditions` calls on lines 82-84 and 95-97
behind condition change checks to avoid unnecessary API writes. Before calling
`UpdateStatusConditions` after the `MarkTrueCondition` call for
`ConditionFinalizing`, verify that the condition on the resource object actually
differs from the previous state. Only proceed with the status update if the
condition has genuinely changed, preventing redundant API calls during requeue
loops.

Source: Coding guidelines