OCPBUGS-64852: Progressing fix#467
Conversation
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-ci-robot
added
jira/valid-reference
|
@theobarberbany: This pull request references Jira Issue OCPBUGS-64852, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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:
WalkthroughReconcile now treats resource updates as "progressing" (sync returns a progressing flag driven by applyResources' cumulative anyUpdated). Error handling centralizes transient vs terminal classification via a failureWindow; tests add fake-clock and error-injecting client coverage for progressing and transient-error timing. ChangesSync Progressing Contract and Implementation
Sequence DiagramsequenceDiagram
participant Reconcile
participant sync
participant applyResources
participant Resources
Reconcile->>sync: call sync()
sync->>Resources: load resources
sync->>applyResources: applyResources(resources)
applyResources->>Resources: apply each resource
applyResources-->>applyResources: OR anyUpdated across resources
applyResources->>sync: return (anyUpdated, nil)
sync->>Reconcile: return (progressing, nil)
alt progressing == true
Reconcile->>Reconcile: return early (no Available)
else progressing == false
Reconcile->>Reconcile: continue and set Available
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings, 1 inconclusive)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/cloud_config_sync_controller.go (1)
191-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet the deferred dispatcher own the feature-gate failure paths.
This block still sets Degraded inline and returns raw errors, so it bypasses the new transient/terminal policy. In practice,
CurrentFeatureGates()sync failures degrade immediately instead of after the failure window, andFeatureGateAccess == nilnever reaches the later terminal classification at Line 275.Suggested fix
// Check if FeatureGateAccess is configured (needed early for transformer) if r.FeatureGateAccess == nil { - klog.Errorf("FeatureGateAccess is not configured") - if err := r.setDegradedCondition(ctx); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for cloud config controller: %v", err) - } - return ctrl.Result{}, fmt.Errorf("FeatureGateAccess is not configured") + return ctrl.Result{}, reconcile.TerminalError(fmt.Errorf("FeatureGateAccess is not configured")) } features, err := r.FeatureGateAccess.CurrentFeatureGates() if err != nil { - klog.Errorf("unable to get feature gates: %v", err) - if errD := r.setDegradedCondition(ctx); errD != nil { - return ctrl.Result{}, fmt.Errorf("failed to set conditions for cloud config controller: %v", errD) - } - return ctrl.Result{}, err + return ctrl.Result{}, fmt.Errorf("failed to get feature gates: %w", err) }🤖 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/controllers/cloud_config_sync_controller.go` around lines 191 - 207, The feature-gate failure paths are currently setting degraded inline and returning raw errors; remove the calls to r.setDegradedCondition in the FeatureGateAccess == nil branch and the CurrentFeatureGates error branch so the deferred dispatcher can own transient/terminal classification, and instead just log the problem and return an error (for the nil case create and return a descriptive error) without modifying conditions here; references: r.FeatureGateAccess, r.setDegradedCondition, and r.FeatureGateAccess.CurrentFeatureGates().
🤖 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/controllers/cloud_config_sync_controller.go`:
- Around line 145-153: handleTerminal currently leaves
fw.consecutiveFailureSince intact so a later transient failure inherits the old
window; modify fw.handleTerminal (method on type failureWindow) to clear/reset
the consecutiveFailureSince field when a terminal error is handled (before
returning) so future transient failures start a fresh window, then proceed to
call setDegraded() and return as before.
In `@pkg/controllers/clusteroperator_controller.go`:
- Around line 191-196: In handleTerminalError, clear the transient failure
tracking so past transient windows don't persist after a terminal error;
specifically reset r.failures (or call an existing reset/clearFailures helper)
before calling setStatusDegraded so that when the terminal error is resolved
future transient failures start fresh and won't immediately trip
aggregatedTransientDegradedThreshold; update
CloudOperatorReconciler.handleTerminalError to reset the failure state early on
the terminal path.
In `@pkg/controllers/trusted_ca_bundle_controller_test.go`:
- Line 385: The DeferCleanup call is currently discarding errors from cl.Delete
(DeferCleanup(func() { _ = cl.Delete(ctx, proxy) })), which can leak cluster
state; change this to capture and handle the returned error from cl.Delete(ctx,
proxy) inside the cleanup closure by asserting success (e.g.,
Expect(err).ToNot(HaveOccurred()) / require.NoError) or implementing a short
retry loop that attempts cl.Delete(ctx, proxy) a few times before failing, and
do the same for the other occurrence around line 431; reference the DeferCleanup
closure, cl.Delete, ctx and proxy when updating the test cleanup.
In `@pkg/controllers/trusted_ca_bundle_controller.go`:
- Around line 83-91: The early-return branch that checks req.Namespace ==
OpenshiftConfigNamespace && proxyConfig.Spec.TrustedCA.Name != req.Name
currently sets partialRun=true but still calls setAvailableCondition(), which
can wrongly flip the controller to Available on unrelated ConfigMap events;
remove the call to r.setAvailableCondition(ctx) (or guard it so it is not
invoked when partialRun is true) inside that branch so unrelated
openshift-config updates do not clear a real degraded state; update any tests or
comments referencing partialRun to reflect that setAvailableCondition is not
called for this early-return.
---
Outside diff comments:
In `@pkg/controllers/cloud_config_sync_controller.go`:
- Around line 191-207: The feature-gate failure paths are currently setting
degraded inline and returning raw errors; remove the calls to
r.setDegradedCondition in the FeatureGateAccess == nil branch and the
CurrentFeatureGates error branch so the deferred dispatcher can own
transient/terminal classification, and instead just log the problem and return
an error (for the nil case create and return a descriptive error) without
modifying conditions here; references: r.FeatureGateAccess,
r.setDegradedCondition, and r.FeatureGateAccess.CurrentFeatureGates().
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8b806eb-a534-4fdb-812e-318db193e386
📒 Files selected for processing (7)
pkg/controllers/cloud_config_sync_controller.gopkg/controllers/cloud_config_sync_controller_test.gopkg/controllers/clusteroperator_controller.gopkg/controllers/clusteroperator_controller_test.gopkg/controllers/reconcile_dispatch.gopkg/controllers/trusted_ca_bundle_controller.gopkg/controllers/trusted_ca_bundle_controller_test.go
theobarberbany
force-pushed
the
progressing-fix
branch
from
d8cab99 to
0b26567
Compare
sync() returned nil on success, so Reconcile always called setStatusAvailable immediately after setStatusProgressing. Change sync to return (bool, error) so Reconcile can skip setStatusAvailable when resources were just updated. Also fix applyResources overwriting the updated flag each iteration — only the last resource's status was returned.
theobarberbany
force-pushed
the
progressing-fix
branch
from
0b26567 to
2721bba
Compare
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controllers/clusteroperator_controller_test.go (1)
608-637: ⚡ Quick winMake the "non-final resource" precondition explicit.
This test only covers the regression if the mutated
Deploymentis followed by at least one unchanged resource. Right now it only assertslen(awsResources) >= 2, so ifcloud.GetResources()ever returns theDeploymentlast, the test still passes without proving the OR-accumulation behavior.Suggested tightening
awsResources, err := cloud.GetResources(operatorConfig) Expect(err).To(Succeed()) - Expect(len(awsResources)).To(BeNumerically(">=", 2)) + Expect(len(awsResources)).To(BeNumerically(">=", 2)) + + deploymentIndex := -1 + for i, res := range awsResources { + if _, ok := res.(*appsv1.Deployment); ok { + deploymentIndex = i + break + } + } + Expect(deploymentIndex).To(BeNumerically(">=", 0), "expected AWS resources to include a Deployment") + Expect(deploymentIndex).To(BeNumerically("<", len(awsResources)-1), "test requires the changed resource to be non-final")🤖 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/controllers/clusteroperator_controller_test.go` around lines 608 - 637, The test's precondition is vague because it only asserts len(awsResources) >= 2 but doesn't ensure the mutated Deployment is not the last resource; update the test around resources/awsResources and reconciler.applyResources so the mutated Deployment is guaranteed to be followed by at least one unchanged resource: locate the Deployment in resources (by checking type appsv1.Deployment), record its index, assert that there exists at least one resource with a higher index (or if it is last, append a copy of an unchanged resource after it) before mutating its Spec.Replicas, then call reconciler.applyResources and assert updated is true; use the existing symbols resources, awsResources, dep (the Deployment variable) and reconciler.applyResources to implement this.
🤖 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/controllers/clusteroperator_controller_test.go`:
- Around line 721-722: The delete call for the test Infrastructure currently
ignores its error; update the cleanup to capture the error returned by
cl.Delete(ctx, infra) (the infra variable of type configv1.Infrastructure) and
handle it: if errors.IsNotFound(err) return/continue, otherwise fail the test
(t.Fatalf or require.NoError) so any real delete failure surfaces; use the
existing ctx and infrastructureResourceName to locate the infra object for this
check.
---
Nitpick comments:
In `@pkg/controllers/clusteroperator_controller_test.go`:
- Around line 608-637: The test's precondition is vague because it only asserts
len(awsResources) >= 2 but doesn't ensure the mutated Deployment is not the last
resource; update the test around resources/awsResources and
reconciler.applyResources so the mutated Deployment is guaranteed to be followed
by at least one unchanged resource: locate the Deployment in resources (by
checking type appsv1.Deployment), record its index, assert that there exists at
least one resource with a higher index (or if it is last, append a copy of an
unchanged resource after it) before mutating its Spec.Replicas, then call
reconciler.applyResources and assert updated is true; use the existing symbols
resources, awsResources, dep (the Deployment variable) and
reconciler.applyResources to implement this.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8da2b88b-f372-4612-be3b-af7458a23703
📒 Files selected for processing (2)
pkg/controllers/clusteroperator_controller.gopkg/controllers/clusteroperator_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/clusteroperator_controller.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controllers/clusteroperator_controller_test.go (1)
608-637: ⚡ Quick winMake the "non-final resource" precondition explicit.
This test only covers the regression if the mutated
Deploymentis followed by at least one unchanged resource. Right now it only assertslen(awsResources) >= 2, so ifcloud.GetResources()ever returns theDeploymentlast, the test still passes without proving the OR-accumulation behavior.Suggested tightening
awsResources, err := cloud.GetResources(operatorConfig) Expect(err).To(Succeed()) - Expect(len(awsResources)).To(BeNumerically(">=", 2)) + Expect(len(awsResources)).To(BeNumerically(">=", 2)) + + deploymentIndex := -1 + for i, res := range awsResources { + if _, ok := res.(*appsv1.Deployment); ok { + deploymentIndex = i + break + } + } + Expect(deploymentIndex).To(BeNumerically(">=", 0), "expected AWS resources to include a Deployment") + Expect(deploymentIndex).To(BeNumerically("<", len(awsResources)-1), "test requires the changed resource to be non-final")🤖 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/controllers/clusteroperator_controller_test.go` around lines 608 - 637, The test's precondition is vague because it only asserts len(awsResources) >= 2 but doesn't ensure the mutated Deployment is not the last resource; update the test around resources/awsResources and reconciler.applyResources so the mutated Deployment is guaranteed to be followed by at least one unchanged resource: locate the Deployment in resources (by checking type appsv1.Deployment), record its index, assert that there exists at least one resource with a higher index (or if it is last, append a copy of an unchanged resource after it) before mutating its Spec.Replicas, then call reconciler.applyResources and assert updated is true; use the existing symbols resources, awsResources, dep (the Deployment variable) and reconciler.applyResources to implement this.
🤖 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/controllers/clusteroperator_controller_test.go`:
- Around line 721-722: The delete call for the test Infrastructure currently
ignores its error; update the cleanup to capture the error returned by
cl.Delete(ctx, infra) (the infra variable of type configv1.Infrastructure) and
handle it: if errors.IsNotFound(err) return/continue, otherwise fail the test
(t.Fatalf or require.NoError) so any real delete failure surfaces; use the
existing ctx and infrastructureResourceName to locate the infra object for this
check.
---
Nitpick comments:
In `@pkg/controllers/clusteroperator_controller_test.go`:
- Around line 608-637: The test's precondition is vague because it only asserts
len(awsResources) >= 2 but doesn't ensure the mutated Deployment is not the last
resource; update the test around resources/awsResources and
reconciler.applyResources so the mutated Deployment is guaranteed to be followed
by at least one unchanged resource: locate the Deployment in resources (by
checking type appsv1.Deployment), record its index, assert that there exists at
least one resource with a higher index (or if it is last, append a copy of an
unchanged resource after it) before mutating its Spec.Replicas, then call
reconciler.applyResources and assert updated is true; use the existing symbols
resources, awsResources, dep (the Deployment variable) and
reconciler.applyResources to implement this.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8da2b88b-f372-4612-be3b-af7458a23703
📒 Files selected for processing (2)
pkg/controllers/clusteroperator_controller.gopkg/controllers/clusteroperator_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/clusteroperator_controller.go
🛑 Comments failed to post (1)
pkg/controllers/clusteroperator_controller_test.go (1)
721-722:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle the cleanup delete error instead of discarding it.
Swallowing the
Infrastructuredelete result can leave state behind and make later tests flaky. AcceptIsNotFound, but fail on any other error.Suggested fix
infra := &configv1.Infrastructure{ObjectMeta: metav1.ObjectMeta{Name: infrastructureResourceName}} - cl.Delete(ctx, infra) //nolint:errcheck + err := cl.Delete(ctx, infra) + Expect(err == nil || apierrors.IsNotFound(err)).To(BeTrue(), "Infrastructure should be deleted during cleanup")As per coding guidelines,
**/*.go: Go security (prodsec-skills): Never ignore error returns.🤖 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/controllers/clusteroperator_controller_test.go` around lines 721 - 722, The delete call for the test Infrastructure currently ignores its error; update the cleanup to capture the error returned by cl.Delete(ctx, infra) (the infra variable of type configv1.Infrastructure) and handle it: if errors.IsNotFound(err) return/continue, otherwise fail the test (t.Fatalf or require.NoError) so any real delete failure surfaces; use the existing ctx and infrastructureResourceName to locate the infra object for this check.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controllers/clusteroperator_controller_test.go (1)
1037-1049: ⚡ Quick winMake operand cleanup independent of mid-test assignment.
operandResourcesis only populated at Line 1066, after the firstReconcile(Line 1054). If that reconcile (or the assertions before Line 1066) fails,operandResourcesstaysniland the operands created byReconcile(Deployment/Service/PDB inDefaultManagedNamespace) are never deleted. Sincecl/envtest is shared across suites, leaked operands can cause cross-suite flakiness.Derive the cleanup set in
BeforeEachsoAfterEachalways cleans up regardless of where theItfails.♻️ Populate operandResources in BeforeEach
BeforeEach(func() { ... Expect(cl.Status().Update(ctx, co)).To(Succeed()) + + operatorConfig := config.OperatorConfig{ + ManagedNamespace: DefaultManagedNamespace, + ImagesReference: config.ImagesReference{ + CloudControllerManagerOperator: "registry.ci.openshift.org/openshift:cluster-cloud-controller-manager-operator", + CloudControllerManagerAWS: "registry.ci.openshift.org/openshift:aws-cloud-controller-manager", + }, + PlatformStatus: &configv1.PlatformStatus{Type: configv1.AWSPlatformType}, + } + operandResources, err = cloud.GetResources(operatorConfig) + Expect(err).NotTo(HaveOccurred()) })Then drop the duplicated tracking block at Lines 1057-1067 in the
It.🤖 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/controllers/clusteroperator_controller_test.go` around lines 1037 - 1049, operandResources is populated only after the first Reconcile in the It block, so if that Reconcile or earlier assertions fail the AfterEach cleanup loop (which iterates operandResources) is skipped and test resources leak; fix by initializing/populating the operandResources slice during BeforeEach so the AfterEach deletion loop (which uses cl and client.ObjectKeyFromObject) always has the expected Deployment/Service/PDB entries for DefaultManagedNamespace to remove, and then remove the duplicate tracking block inside the It (the earlier Reconcile tracking that fills operandResources) so creation and cleanup are driven from BeforeEach/AfterEach rather than mid-test.
🤖 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/controllers/reconcile_dispatch_test.go`:
- Around line 19-20: The Get method on errorInjectingClient dereferences
c.getErr without checking for nil; add a nil check: first verify c.getErr != nil
before inspecting *c.getErr, and only inject/return the error when c.getErr !=
nil && *c.getErr != nil; otherwise delegate to the embedded/underlying client's
Get (preserve existing behavior). This change should be made inside the
errorInjectingClient.Get function to avoid panics when getErr was never
initialized.
---
Nitpick comments:
In `@pkg/controllers/clusteroperator_controller_test.go`:
- Around line 1037-1049: operandResources is populated only after the first
Reconcile in the It block, so if that Reconcile or earlier assertions fail the
AfterEach cleanup loop (which iterates operandResources) is skipped and test
resources leak; fix by initializing/populating the operandResources slice during
BeforeEach so the AfterEach deletion loop (which uses cl and
client.ObjectKeyFromObject) always has the expected Deployment/Service/PDB
entries for DefaultManagedNamespace to remove, and then remove the duplicate
tracking block inside the It (the earlier Reconcile tracking that fills
operandResources) so creation and cleanup are driven from BeforeEach/AfterEach
rather than mid-test.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cf021e66-54ce-424a-8b7f-9526a4220bb0
📒 Files selected for processing (2)
pkg/controllers/clusteroperator_controller_test.gopkg/controllers/reconcile_dispatch_test.go
theobarberbany
force-pushed
the
progressing-fix
branch
from
29fdf3e to
25fcb4a
Compare
theobarberbany
force-pushed
the
progressing-fix
branch
from
25fcb4a to
4c92225
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controllers/cloud_config_sync_controller_test.go (1)
878-888: ⚡ Quick winAdd explicit timeouts to the new
Eventuallycalls.Line 878 and Line 886 rely on default
Eventuallytiming, which can make teardown behavior less predictable in CI. Passtimeoutexplicitly here, like the rest of this file.Proposed diff
- Eventually(func() error { + Eventually(func() error { err := cl.Delete(ctx, co) if err == nil || apierrors.IsNotFound(err) { return nil } return err - }).Should(Succeed()) + }, timeout).Should(Succeed()) } Eventually(func() error { return cl.Get(ctx, client.ObjectKey{Name: clusterOperatorName}, co) - }).Should(MatchError(apierrors.IsNotFound, "IsNotFound")) + }, timeout).Should(MatchError(apierrors.IsNotFound, "IsNotFound"))As per coding guidelines, "Ginkgo test code should follow quality requirements: each test should have single responsibility, use BeforeEach/AfterEach for setup/cleanup, include appropriate timeouts on Eventually/Consistently calls, provide meaningful assertion failure messages, and follow codebase patterns for fixtures, clients, and wait structures".
🤖 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/controllers/cloud_config_sync_controller_test.go` around lines 878 - 888, The two new Eventually calls (the delete loop that calls cl.Delete(ctx, co) and the Get call that invokes cl.Get(ctx, client.ObjectKey{Name: clusterOperatorName}, co)) rely on default timing; update both to pass an explicit timeout value consistent with the rest of the file (e.g., the shared timeout variable or a matching time.Duration) by changing Eventually(func() error { ... }).Should(...) to Eventually(func() error { ... }, timeout).Should(...) so teardown behaves predictably in CI.
🤖 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.
Nitpick comments:
In `@pkg/controllers/cloud_config_sync_controller_test.go`:
- Around line 878-888: The two new Eventually calls (the delete loop that calls
cl.Delete(ctx, co) and the Get call that invokes cl.Get(ctx,
client.ObjectKey{Name: clusterOperatorName}, co)) rely on default timing; update
both to pass an explicit timeout value consistent with the rest of the file
(e.g., the shared timeout variable or a matching time.Duration) by changing
Eventually(func() error { ... }).Should(...) to Eventually(func() error { ... },
timeout).Should(...) so teardown behaves predictably in CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e5c03226-7993-47b4-8823-86fdce5dad79
📒 Files selected for processing (3)
pkg/controllers/cloud_config_sync_controller_test.gopkg/controllers/clusteroperator_controller_test.gopkg/controllers/reconcile_dispatch_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/clusteroperator_controller_test.go
theobarberbany
force-pushed
the
progressing-fix
branch
from
4c92225 to
41382df
Compare
|
/pipeline auto |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@theobarberbany: This pull request references Jira Issue OCPBUGS-64852, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: sunzhaohua2. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
theobarberbany
force-pushed
the
progressing-fix
branch
from
41382df to
44d3d6b
Compare
theobarberbany
force-pushed
the
progressing-fix
branch
from
44d3d6b to
0ebf17c
Compare
|
@theobarberbany: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Leo6Leo
left a comment
Leo6Leo
left a comment
There was a problem hiding this comment.
I can understand what is happening here, and it passed my local AI review skill check. No suspicious code included either, the CI is all green. The unit tests are solid and cover the exact bugs.
/verified by CI
/lgtm
openshift-ci-robot
added
the
verified
|
@Leo6Leo: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/approve |
openshift-ci
Bot
added
the
approved
|
/approve Makes sense, and cleans up some of the degraded work, too |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nrb, theobarberbany The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Builds on #436
Summary by CodeRabbit
Improvements
Tests