test: e2e test for UIPlugin post-uninstall cascade cleanup (COO-1404)#1087
test: e2e test for UIPlugin post-uninstall cascade cleanup (COO-1404)#1087DavidRajnoha wants to merge 1 commit into
Conversation
|
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 pull request adds OpenShift-conditional test infrastructure and a comprehensive e2e test for UIPlugin uninstall cleanup. The changes introduce a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DavidRajnoha 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 |
![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
🧹 Nitpick comments (4)
test/e2e/uiplugin_uninstall_test.go (2)
113-131: ⚡ Quick winSaved Subscription loses metadata like labels/annotations and
installPlanApprovalmay be insufficient on recreate.The cleanup recreates a
Subscriptionwith onlyName,Namespace, andSpec. OLM-related labels/annotations (e.g.,operators.coreos.com/...) and any cluster-specific metadata will be lost. The recreated Subscription may behave slightly differently than the original (e.g., different label selectors used bydelete_olm_subscriptioninrun-e2e.sh). If post-test cleanup or subsequent test runs rely on those labels, this could cause flakes.Consider capturing the full original (e.g.,
savedSub := sub.DeepCopy()and clearingResourceVersion/UID) so the recreated resource matches the original modulo server-managed fields.♻️ Suggested change
- savedSub := &olmv1alpha1.Subscription{ - ObjectMeta: metav1.ObjectMeta{ - Name: sub.Name, - Namespace: sub.Namespace, - }, - Spec: sub.Spec.DeepCopy(), - } + savedSub := sub.DeepCopy() + savedSub.ResourceVersion = "" + savedSub.UID = "" + savedSub.Status = olmv1alpha1.SubscriptionStatus{}🤖 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 `@test/e2e/uiplugin_uninstall_test.go` around lines 113 - 131, The cleanup recreates savedSub with only Name/Namespace/Spec which loses labels/annotations and other metadata; update the cleanup to store the full original Subscription by doing savedSub := sub.DeepCopy() (or similar) before modifications, then before calling f.K8sClient.Create clear server-managed fields like ResourceVersion, UID, CreationTimestamp and ManagedFields but keep labels, annotations and Spec (including installPlanApproval) intact so the recreated Subscription matches the original; reference savedSub in the Cleanup closure and call forceDeleteAllUIPlugins as before, then create the cleaned savedSub.
281-294: ⚡ Quick win
waitForResourceAbsentmutates the caller'sobjand swallows non-NotFound errors.Two subtle issues:
objis reused across poll iterations and is populated byGeton each success path — that's fine here because each call site passes a freshly allocated value, but it's a footgun for future callers.- Errors other than
NotFound(transient API server errors, RBAC, etc.) are silently treated as "not yet absent". A persistent error will only surface as a generic timeout viawait.Interrupted, making test failures harder to diagnose.Consider logging the last error before failing:
♻️ Suggested change
func waitForResourceAbsent(t *testing.T, name, namespace string, obj client.Object, timeout time.Duration) { t.Helper() key := client.ObjectKey{Name: name, Namespace: namespace} + var lastErr error err := wait.PollUntilContextTimeout(context.Background(), 5*time.Second, timeout, true, func(ctx context.Context) (bool, error) { - if err := f.K8sClient.Get(ctx, key, obj); apierrors.IsNotFound(err) { + err := f.K8sClient.Get(ctx, key, obj) + if apierrors.IsNotFound(err) { return true, nil } + lastErr = err return false, nil }) if wait.Interrupted(err) { kind := fmt.Sprintf("%T", obj) - t.Fatalf("%s %s/%s was not cleaned up (waited %v)", kind, namespace, name, timeout) + t.Fatalf("%s %s/%s was not cleaned up (waited %v, lastErr=%v)", kind, namespace, name, timeout, lastErr) } }🤖 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 `@test/e2e/uiplugin_uninstall_test.go` around lines 281 - 294, waitForResourceAbsent currently reuses the caller-supplied obj and swallows all non-NotFound errors; change it so each poll iteration allocates a fresh object instance (e.g., via obj.DeepCopyObject() / type-specific new object) and pass that to f.K8sClient.Get to avoid mutating the caller's obj, and surface non-NotFound errors by returning them from the poll function (i.e., return false, err) while recording the last error; on timeout (in wait.Interrupted(err)) include the captured last error in the t.Fatalf message so failures show the real API error instead of only a timeout.test/run-e2e.sh (1)
185-187: ⚡ Quick winUse an array for
extra_argsto handle empty/multi-value cases safely.Static analysis flags
$extra_argsas unquoted (SC2086). A safer pattern is an array so empty stays empty and any future multi-arg case is preserved.♻️ Suggested change
- local extra_args="" - [[ -n "$POSTPONE_RESTORATION" ]] && extra_args="-postpone-restoration=$POSTPONE_RESTORATION" - go test -v -timeout $TEST_TIMEOUT ./test/e2e/... -run "$RUN_REGEX" -count 1 -args -operatorInstallNS="$OPERATORS_NS" $extra_args | tee "$LOGS_DIR/e2e.log" || ret=1 + local -a extra_args=() + [[ -n "$POSTPONE_RESTORATION" ]] && extra_args+=("-postpone-restoration=$POSTPONE_RESTORATION") + go test -v -timeout "$TEST_TIMEOUT" ./test/e2e/... -run "$RUN_REGEX" -count 1 -args -operatorInstallNS="$OPERATORS_NS" "${extra_args[@]}" | tee "$LOGS_DIR/e2e.log" || ret=1🤖 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 `@test/run-e2e.sh` around lines 185 - 187, The current use of a scalar extra_args is unsafe; change it to an array (e.g., extra_args=()) and when POSTPONE_RESTORATION is set append the argument as an array element (e.g., extra_args+=("--postpone-restoration=$POSTPONE_RESTORATION")), then invoke the go test command passing the array with "${extra_args[@]}" alongside -operatorInstallNS="$OPERATORS_NS" to avoid word-splitting and handle empty or multi-value cases safely; update references to the extra_args variable in the go test invocation accordingly.pkg/operator/operator.go (1)
437-443: 💤 Low valueUse the manager's cached client where possible, or document why a fresh client is required.
Creating a new direct client at shutdown is fine if the manager's cache has already stopped serving reads, but it bypasses any client-side optimizations and may surprise future maintainers. Consider a brief comment explaining why
o.manager.GetClient()is not used here (e.g., cache is torn down by the timectx.Done()fires).🤖 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 `@pkg/operator/operator.go` around lines 437 - 443, The shutdown code currently creates a fresh direct client via client.New (assigned to directClient) which bypasses the manager cache; either switch to using the manager's cached client by calling o.manager.GetClient() for shutdown cleanup, or add a concise comment above the client.New call explaining why a fresh client is required (e.g., the manager cache may already be stopped when ctx.Done() fires), referencing directClient, client.New, o.manager.GetClient(), and o.restConfig so future maintainers understand the choice.
🤖 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 `@pkg/operator/operator.go`:
- Around line 396-400: The shutdown runnable added via
op.newShutdownCleanupRunnable() (registered with mgr.Add when
cfg.FeatureGates.OpenShift.Enabled is true) unconditionally calls
cleanupUIPluginsFromConsole on any shutdown; change it so it only runs when the
operator is actually being uninstalled: either remove the shutdown-time
deregistration entirely and rely on reconciliation/owner-ref GC, or modify
newShutdownCleanupRunnable()/cleanupUIPluginsFromConsole to first detect an
uninstall signal (for example, verify the UIPlugin CRD is no longer served via
discovery or the Console/operator CR shows a DeletionTimestamp or a specific
uninstall sentinel) and only proceed to patch the Console when that uninstall
condition is true; ensure the check is performed inside the runnable before
listing UIPlugin CRs so normal restarts (rolling updates, leader loss, SIGTERM)
do not remove Console entries.
---
Nitpick comments:
In `@pkg/operator/operator.go`:
- Around line 437-443: The shutdown code currently creates a fresh direct client
via client.New (assigned to directClient) which bypasses the manager cache;
either switch to using the manager's cached client by calling
o.manager.GetClient() for shutdown cleanup, or add a concise comment above the
client.New call explaining why a fresh client is required (e.g., the manager
cache may already be stopped when ctx.Done() fires), referencing directClient,
client.New, o.manager.GetClient(), and o.restConfig so future maintainers
understand the choice.
In `@test/e2e/uiplugin_uninstall_test.go`:
- Around line 113-131: The cleanup recreates savedSub with only
Name/Namespace/Spec which loses labels/annotations and other metadata; update
the cleanup to store the full original Subscription by doing savedSub :=
sub.DeepCopy() (or similar) before modifications, then before calling
f.K8sClient.Create clear server-managed fields like ResourceVersion, UID,
CreationTimestamp and ManagedFields but keep labels, annotations and Spec
(including installPlanApproval) intact so the recreated Subscription matches the
original; reference savedSub in the Cleanup closure and call
forceDeleteAllUIPlugins as before, then create the cleaned savedSub.
- Around line 281-294: waitForResourceAbsent currently reuses the
caller-supplied obj and swallows all non-NotFound errors; change it so each poll
iteration allocates a fresh object instance (e.g., via obj.DeepCopyObject() /
type-specific new object) and pass that to f.K8sClient.Get to avoid mutating the
caller's obj, and surface non-NotFound errors by returning them from the poll
function (i.e., return false, err) while recording the last error; on timeout
(in wait.Interrupted(err)) include the captured last error in the t.Fatalf
message so failures show the real API error instead of only a timeout.
In `@test/run-e2e.sh`:
- Around line 185-187: The current use of a scalar extra_args is unsafe; change
it to an array (e.g., extra_args=()) and when POSTPONE_RESTORATION is set append
the argument as an array element (e.g.,
extra_args+=("--postpone-restoration=$POSTPONE_RESTORATION")), then invoke the
go test command passing the array with "${extra_args[@]}" alongside
-operatorInstallNS="$OPERATORS_NS" to avoid word-splitting and handle empty or
multi-value cases safely; update references to the extra_args variable in the go
test invocation accordingly.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 573f555c-d1e9-43a2-86be-22dfc1e3f0fc
📒 Files selected for processing (8)
hack/dev-deploy.shpkg/controllers/uiplugin/controller.gopkg/controllers/uiplugin/plugin_info_builder.gopkg/operator/operator.gotest/e2e/framework/framework.gotest/e2e/main_test.gotest/e2e/uiplugin_uninstall_test.gotest/run-e2e.sh
DavidRajnoha
force-pushed
the
COO-1404-test-pr-1084
branch
4 times, most recently
from
f8dce61 to
cda37d2
Compare
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 `@test/e2e/aaa_uiplugin_uninstall_test.go`:
- Around line 259-261: The test only fails when both foundCSV and foundSub are
nil; change the guard to require both resources be present before proceeding by
failing if either is missing (i.e., replace the current conditional on foundCSV
and foundSub with a check that errors when foundCSV == nil || foundSub == nil)
so the uninstall cannot run with a partial lookup and leave the cluster in a bad
state; update the t.Fatal message to reflect that both Subscription and CSV are
required.
- Around line 271-276: The polling helpers waitForResourceGone and
waitForResourceAbsent currently swallow all non-NotFound errors by returning
(false, nil) in the wait.PollUntilContextTimeout callback; change the callback
so that when f.K8sClient.Get(ctx, key, obj) returns a non-nil error that is not
apierrors.IsNotFound(err) you return (false, err) to propagate the failure to
the poller (instead of retrying silently). Also adjust the callers of
waitForResourceGone / waitForResourceAbsent to check for and handle the returned
error from wait.PollUntilContextTimeout (or from the helper) rather than
treating everything as a timeout/interruption.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c4abc1c6-2091-4fbe-8236-42f2fc2232ab
📒 Files selected for processing (3)
test/e2e/aaa_uiplugin_uninstall_test.gotest/e2e/main_test.gotest/run-e2e.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/main_test.go
- test/run-e2e.sh
| if foundCSV == nil && foundSub == nil { | ||
| t.Fatal("Could not find COO Subscription or CSV — operator may not be installed via OLM") | ||
| } |
There was a problem hiding this comment.
Require both Subscription and CSV before proceeding with uninstall.
This currently only fails when both are missing. If one lookup fails, the test can perform a partial uninstall and skip restoration setup, which can leave the cluster in a bad state for later tests.
Suggested fix
- if foundCSV == nil && foundSub == nil {
- t.Fatal("Could not find COO Subscription or CSV — operator may not be installed via OLM")
- }
+ if foundCSV == nil || foundSub == nil {
+ t.Fatalf(
+ "Could not find both COO Subscription and CSV (foundCSV=%t, foundSub=%t) — refusing partial uninstall",
+ foundCSV != nil,
+ foundSub != nil,
+ )
+ }📝 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 foundCSV == nil && foundSub == nil { | |
| t.Fatal("Could not find COO Subscription or CSV — operator may not be installed via OLM") | |
| } | |
| if foundCSV == nil || foundSub == nil { | |
| t.Fatalf( | |
| "Could not find both COO Subscription and CSV (foundCSV=%t, foundSub=%t) — refusing partial uninstall", | |
| foundCSV != nil, | |
| foundSub != nil, | |
| ) | |
| } |
🤖 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 `@test/e2e/aaa_uiplugin_uninstall_test.go` around lines 259 - 261, The test
only fails when both foundCSV and foundSub are nil; change the guard to require
both resources be present before proceeding by failing if either is missing
(i.e., replace the current conditional on foundCSV and foundSub with a check
that errors when foundCSV == nil || foundSub == nil) so the uninstall cannot run
with a partial lookup and leave the cluster in a bad state; update the t.Fatal
message to reflect that both Subscription and CSV are required.
| err := wait.PollUntilContextTimeout(context.Background(), 5*time.Second, timeout, true, func(ctx context.Context) (bool, error) { | ||
| if err := f.K8sClient.Get(ctx, key, obj); apierrors.IsNotFound(err) { | ||
| return true, nil | ||
| } | ||
| return false, nil | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n test/e2e/aaa_uiplugin_uninstall_test.go | sed -n '260,300p'Repository: rhobs/observability-operator
Length of output: 2053
Propagate non-NotFound API errors from polling helpers instead of silently swallowing them.
Both waitForResourceGone (line 272) and waitForResourceAbsent (line 285) check only for IsNotFound errors and ignore all other errors. Any transient API failure, RBAC denial, or network error triggers return false, nil, which causes the polling loop to retry silently until timeout. This masks real failures and misreports them as cleanup timeouts.
Restructure the error handling to return (false, err) for non-NotFound errors, allowing the polling mechanism to immediately surface the actual error:
if err := f.K8sClient.Get(ctx, key, obj); err != nil {
if apierrors.IsNotFound(err) {
return true, nil
}
return false, err
}
return false, nilThen update the callers to handle the propagated error appropriately instead of relying solely on wait.Interrupted.
📝 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.
| err := wait.PollUntilContextTimeout(context.Background(), 5*time.Second, timeout, true, func(ctx context.Context) (bool, error) { | |
| if err := f.K8sClient.Get(ctx, key, obj); apierrors.IsNotFound(err) { | |
| return true, nil | |
| } | |
| return false, nil | |
| }) | |
| err := wait.PollUntilContextTimeout(context.Background(), 5*time.Second, timeout, true, func(ctx context.Context) (bool, error) { | |
| if err := f.K8sClient.Get(ctx, key, obj); err != nil { | |
| if apierrors.IsNotFound(err) { | |
| return true, nil | |
| } | |
| return false, err | |
| } | |
| return false, nil | |
| }) |
🤖 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 `@test/e2e/aaa_uiplugin_uninstall_test.go` around lines 271 - 276, The polling
helpers waitForResourceGone and waitForResourceAbsent currently swallow all
non-NotFound errors by returning (false, nil) in the
wait.PollUntilContextTimeout callback; change the callback so that when
f.K8sClient.Get(ctx, key, obj) returns a non-nil error that is not
apierrors.IsNotFound(err) you return (false, err) to propagate the failure to
the poller (instead of retrying silently). Also adjust the callers of
waitForResourceGone / waitForResourceAbsent to check for and handle the returned
error from wait.PollUntilContextTimeout (or from the helper) rather than
treating everything as a timeout/interruption.
bf0060a to
853ad02
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/e2e/uiplugin_uninstall_test.go (2)
275-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire both Subscription and CSV before uninstall starts.
Line 275 currently fails only when both are missing; if either one is missing, the test can run a partial uninstall and leave cleanup/restoration in an inconsistent state. Fail when either resource is missing.
Proposed fix
- if foundCSV == nil && foundSub == nil { - t.Fatal("Could not find COO Subscription or CSV — operator may not be installed via OLM") - } + if foundCSV == nil || foundSub == nil { + t.Fatalf( + "Could not find both COO Subscription and CSV (foundCSV=%t, foundSub=%t) — refusing partial uninstall", + foundCSV != nil, + foundSub != nil, + ) + }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 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 `@test/e2e/uiplugin_uninstall_test.go` around lines 275 - 277, The test currently only fails when both foundCSV and foundSub are nil; change the check to fail if either resource is missing by testing if foundCSV == nil || foundSub == nil and call t.Fatal with the same (or slightly updated) message so the uninstall does not proceed when one of the required resources is absent; update the conditional around foundCSV and foundSub and the t.Fatal invocation accordingly.
287-305:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not swallow non-NotFound errors in polling helpers.
At Line 287-292 and Line 300-305, non-NotFound
Geterrors are retried silently, which masks real failures as timeouts. Propagate non-NotFound errors from the poll callback so the test fails with the real root cause.Proposed fix
func waitForResourceGone(t *testing.T, name, namespace string, obj client.Object, timeout time.Duration) bool { t.Helper() key := client.ObjectKey{Name: name, Namespace: namespace} err := wait.PollUntilContextTimeout(context.Background(), 5*time.Second, timeout, true, func(ctx context.Context) (bool, error) { - if err := f.K8sClient.Get(ctx, key, obj); apierrors.IsNotFound(err) { - return true, nil - } - return false, nil + if err := f.K8sClient.Get(ctx, key, obj); err != nil { + if apierrors.IsNotFound(err) { + return true, nil + } + return false, err + } + return false, nil }) - return !wait.Interrupted(err) + if err != nil { + if wait.Interrupted(err) { + return false + } + t.Fatalf("error waiting for %T %s/%s deletion: %v", obj, namespace, name, err) + } + return true } func waitForResourceAbsent(t *testing.T, name, namespace string, obj client.Object, timeout time.Duration) { t.Helper() key := client.ObjectKey{Name: name, Namespace: namespace} err := wait.PollUntilContextTimeout(context.Background(), 5*time.Second, timeout, true, func(ctx context.Context) (bool, error) { - if err := f.K8sClient.Get(ctx, key, obj); apierrors.IsNotFound(err) { - return true, nil - } - return false, nil + if err := f.K8sClient.Get(ctx, key, obj); err != nil { + if apierrors.IsNotFound(err) { + return true, nil + } + return false, err + } + return false, nil }) - if wait.Interrupted(err) { + if err != nil { + if wait.Interrupted(err) { kind := fmt.Sprintf("%T", obj) t.Fatalf("%s %s/%s was not cleaned up (waited %v)", kind, namespace, name, timeout) + } + t.Fatalf("error waiting for %T %s/%s cleanup: %v", obj, namespace, name, err) } }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 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 `@test/e2e/uiplugin_uninstall_test.go` around lines 287 - 305, Both polling helpers currently swallow non-NotFound errors from f.K8sClient.Get and retry until timeout; change the poll callback in both the anonymous function inside wait.PollUntilContextTimeout (used in the first helper returning bool and in waitForResourceAbsent) to propagate any non-nil, non-NotFound error by returning (false, err) instead of (false, nil), so that the wait fails fast with the real error; locate the callbacks around the client.Get call and update the conditional logic that currently only checks apierrors.IsNotFound(err) to handle err != nil && !apierrors.IsNotFound(err) -> return false, err.
🤖 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 `@test/e2e/uiplugin_uninstall_test.go`:
- Around line 344-349: The polling callbacks passed to
wait.PollUntilContextTimeout suppress API errors by returning (false, nil) when
f.K8sClient.List fails; update those callbacks (the ones creating remaining
uiv1.UIPluginList and similar blocks) to propagate the actual error by returning
(false, err) instead of (false, nil), so change the anonymous func passed to
wait.PollUntilContextTimeout that calls f.K8sClient.List to return false, err on
List failures; apply the same fix to the other two polling callbacks that call
f.K8sClient.List.
---
Duplicate comments:
In `@test/e2e/uiplugin_uninstall_test.go`:
- Around line 275-277: The test currently only fails when both foundCSV and
foundSub are nil; change the check to fail if either resource is missing by
testing if foundCSV == nil || foundSub == nil and call t.Fatal with the same (or
slightly updated) message so the uninstall does not proceed when one of the
required resources is absent; update the conditional around foundCSV and
foundSub and the t.Fatal invocation accordingly.
- Around line 287-305: Both polling helpers currently swallow non-NotFound
errors from f.K8sClient.Get and retry until timeout; change the poll callback in
both the anonymous function inside wait.PollUntilContextTimeout (used in the
first helper returning bool and in waitForResourceAbsent) to propagate any
non-nil, non-NotFound error by returning (false, err) instead of (false, nil),
so that the wait fails fast with the real error; locate the callbacks around the
client.Get call and update the conditional logic that currently only checks
apierrors.IsNotFound(err) to handle err != nil && !apierrors.IsNotFound(err) ->
return false, 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 84cdeee6-a993-49ee-b210-c7e4b4ebfd88
📒 Files selected for processing (3)
test/e2e/main_test.gotest/e2e/uiplugin_uninstall_test.gotest/run-e2e.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/main_test.go
- test/run-e2e.sh
| err := wait.PollUntilContextTimeout(ctx, 5*time.Second, 2*time.Minute, true, func(ctx context.Context) (bool, error) { | ||
| var remaining uiv1.UIPluginList | ||
| if err := f.K8sClient.List(ctx, &remaining); err != nil { | ||
| return false, nil | ||
| } | ||
| return len(remaining.Items) == 0, nil |
There was a problem hiding this comment.
Propagate API list errors in remaining polling loops as well.
Line 344-349, Line 366-373, and Line 396-398 also suppress client List errors (return false, nil). This causes RBAC/API/network issues to surface as misleading timeout failures. Return false, err for non-retryable errors in these callbacks too.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 366-373, 396-398
🤖 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 `@test/e2e/uiplugin_uninstall_test.go` around lines 344 - 349, The polling
callbacks passed to wait.PollUntilContextTimeout suppress API errors by
returning (false, nil) when f.K8sClient.List fails; update those callbacks (the
ones creating remaining uiv1.UIPluginList and similar blocks) to propagate the
actual error by returning (false, err) instead of (false, nil), so change the
anonymous func passed to wait.PollUntilContextTimeout that calls
f.K8sClient.List to return false, err on List failures; apply the same fix to
the other two polling callbacks that call f.K8sClient.List.
853ad02 to
6ef3361
Compare
…404) Co-authored-by: Cursor <cursoragent@cursor.com>
DavidRajnoha
force-pushed
the
COO-1404-test-pr-1084
branch
from
6ef3361 to
63998b8
Compare
1 participant
Summary
TestUIPluginUninstallCleanupe2e test that verifies deleting a UIPlugin CR after the operator has been uninstalled via OLM correctly cascade-deletes all child resources through Kubernetes garbage collection (OwnerReferences).Also adds:
CapturePodLogsandFindPodByLabelhelpers to the e2e framework for operator shutdown diagnostics--postpone-restorationflag torun-e2e.shfor delayed Subscription restoration (allows manual cluster inspection)--openshiftflag support inrun-e2e.shfor UIPlugin tests on OpenShift clustersDependencies
Test plan
TestUIPluginUninstallCleanupon OCP cluster with the finalizer fix — should pass (CR deletes immediately, children cascade-deleted)TestUIPluginUninstallCleanupon OCP cluster without the finalizer fix — should fail at "Waiting for UIPlugin CR to be fully deleted" (finalizer blocks deletion)Supersedes #1083.
Made with Cursor