kubernetes/controller/internal/controller/deployer/deployer_controller_test.go (2)

353-362: Same concerns as the earlier cleanup block.

This cleanup has the same issues: cluster-wide deployer listing and silently ignored update errors. Apply the same namespace scoping and error logging fix as recommended for lines 64-73.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@kubernetes/controller/internal/controller/deployer/deployer_controller_test.go`
around lines 353 - 362, The cleanup is listing all Deployer resources
cluster-wide and ignoring update errors; change the List call to scope to the
test namespace (use client.ListOptions{Namespace: <test namespace>} or
client.InNamespace(ctxNamespace) equivalent) so v1alpha1.DeployerList named
deployers only returns namespace resources, and check/handle the error returned
by k8sClient.Update(ctx, d) instead of discarding it—log or fail the test on
error; keep using controllerutil.RemoveFinalizer for applySetPruneFinalizer and
resourceWatchFinalizer and continue calling test.DeleteObject(ctx, k8sClient, d)
after ensuring the update succeeded.

---

64-73: Scope cleanup to test namespace and handle update errors.

Two concerns with this cleanup logic:

1. k8sClient.List(ctx, deployers) lists all deployers cluster-wide, which could interfere with parallel test runs by cleaning up deployers from other tests.

2. _ = k8sClient.Update(ctx, d) silently ignores update errors. If the update fails (e.g., conflict), the finalizers remain and the subsequent delete may hang indefinitely.

♻️ Proposed fix to scope listing and log update errors

 By("cleaning up deployers")
 deployers := &v1alpha1.DeployerList{}
-Expect(k8sClient.List(ctx, deployers)).To(Succeed())
+Expect(k8sClient.List(ctx, deployers, client.InNamespace(namespace.GetName()))).To(Succeed())
 for i := range deployers.Items {
     d := &deployers.Items[i]
     controllerutil.RemoveFinalizer(d, applySetPruneFinalizer)
     controllerutil.RemoveFinalizer(d, resourceWatchFinalizer)
-    _ = k8sClient.Update(ctx, d)
+    if err := k8sClient.Update(ctx, d); err != nil {
+        GinkgoLogr.Error(err, "failed to remove finalizers", "deployer", d.Name)
+    }
     test.DeleteObject(ctx, k8sClient, d)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@kubernetes/controller/internal/controller/deployer/deployer_controller_test.go`
around lines 64 - 73, Scope the Deployer cleanup to the test namespace and stop
ignoring update errors: when listing deployers, pass the namespace option (e.g.,
client.InNamespace(testNamespace) or the existing namespace variable) to
k8sClient.List(ctx, deployers, ...); when removing finalizers and calling
k8sClient.Update(ctx, d) capture the error and handle retries/conflicts instead
of silencing it — for example assert Expect(err).To(Succeed()) or on
apierrors.IsConflict fetch the latest object, remove the finalizers again and
retry the Update before calling test.DeleteObject; keep references to deployers,
controllerutil.RemoveFinalizer, applySetPruneFinalizer, resourceWatchFinalizer,
k8sClient.Update, and test.DeleteObject to locate the code.

kubernetes/controller/internal/controller/deployer/deployer_controller_rgd_test.go (1)

188-190: Scope teardown to test-owned objects only.

At Line 189 and Line 201, the test lists all Deployers/RGDs and then deletes everything found. This can cause cross-test interference in shared environments.

Suggested direction

- deployers := &v1alpha1.DeployerList{}
- Expect(k8sClient.List(ctx, deployers)).To(Succeed())
- for i := range deployers.Items { ... }
+ d := &v1alpha1.Deployer{}
+ err := k8sClient.Get(ctx, client.ObjectKey{Name: deployerObjName}, d)
+ if err == nil {
+   // remove finalizers + update + delete
+ } else {
+   Expect(errors.IsNotFound(err)).To(BeTrue())
+ }

- RGDs := &unstructured.UnstructuredList{}
- RGDs.SetGroupVersionKind(listGVK)
- Expect(k8sClient.List(ctx, RGDs)).To(Succeed())
- for i := range RGDs.Items {
-   test.DeleteObject(ctx, k8sClient, &RGDs.Items[i])
- }
+ rgdToDelete := &unstructured.Unstructured{}
+ rgdToDelete.SetGroupVersionKind(gvk)
+ rgdToDelete.SetName(rgdObj.GetName())
+ err = k8sClient.Get(ctx, client.ObjectKeyFromObject(rgdToDelete), rgdToDelete)
+ if err == nil {
+   test.DeleteObject(ctx, k8sClient, rgdToDelete)
+ } else {
+   Expect(errors.IsNotFound(err)).To(BeTrue())
+ }

Also applies to: 199-204

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@kubernetes/controller/internal/controller/deployer/deployer_controller_rgd_test.go`
around lines 188 - 190, The test currently lists and deletes all Deployers/RGDs
via k8sClient.List into the deployers/rgds slices which can remove resources
from other tests; change the List calls to restrict results to test-owned
objects only by passing appropriate client.ListOptions (e.g.,
client.InNamespace(...) if tests use a dedicated namespace, or
client.MatchingLabels{"test-run": testID} if using a per-test label) and/or
filter the returned items by checking a specific label/ownerReference before
deleting; update the code around the deployers variable and the rgds list
handling (the k8sClient.List calls and subsequent deletion loop) to only operate
on items that have the test label or the expected ownerReference.