Conversation
Show Installing when first install is rolling out and Upgrading when an existing bundle is being updated. This gives users clearer status about what's happening instead of the vague Absent reason. Assisted-by: Cursor
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces new "Installing" and "Upgrading" status reasons for the ClusterExtension Installed condition to provide more granular feedback during bundle installation and upgrade operations. The PR title "TEST - IGNORE" suggests this is intended for testing purposes only.
Changes:
- Added
ReasonInstallingandReasonUpgradingconstants to the API to indicate transient states during installation and upgrade operations - Updated controller logic to report
Installingstatus during first-time installations andUpgradingstatus during bundle upgrades when a healthy rollout is in progress - Added E2E test infrastructure to handle checking for transient statuses that may progress to completion before assertions run
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1/common_types.go | Adds ReasonInstalling and ReasonUpgrading constants for the Installed condition |
| internal/operator-controller/conditionsets/conditionsets.go | Registers the new reason constants in the condition reasons list |
| internal/operator-controller/controllers/common_controller.go | Implements logic to detect and report Installing/Upgrading statuses based on revision states |
| internal/operator-controller/controllers/common_controller_test.go | Adds tests for upgrade scenarios and updates existing tests to use Installing instead of Absent |
| test/e2e/steps/steps.go | Implements ClusterExtensionReportsConditionOrProgressed to handle transient status checks in E2E tests |
| test/e2e/features/update.feature | Updates upgrade scenario to verify Upgrading status is reported during updates |
| test/e2e/features/install.feature | Adds scenario to verify Installing status is reported during initial installation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| acceptableReasons, hasProgressedStates := progressedReasons[conditionReason] | ||
| if !hasProgressedStates { | ||
| // If no progressed states defined, use normal wait |
There was a problem hiding this comment.
The progressedReasons map only checks for specific transient reasons ("Installing" and "Upgrading"), but the function has a fallback that calls waitForExtensionCondition for unknown reasons. However, this fallback behavior means that if someone adds a new transient reason in the future and forgets to update this map, tests using this function will silently behave differently (waiting only for exact match instead of allowing progression). Consider documenting this behavior or adding a warning log when falling back to the standard wait behavior.
| // If no progressed states defined, use normal wait | |
| // If no progressed states defined, use normal wait. This means new transient | |
| // reasons must be added to progressedReasons above to allow "progressed" states. | |
| logger.V(1).Info("No progressed states defined for reason; falling back to standard wait behavior", | |
| "conditionType", conditionType, | |
| "conditionStatus", conditionStatus, | |
| "conditionReason", conditionReason) |
| } | ||
| } | ||
|
|
||
| func TestSetInstalledStatusFromRevisionStates_ConfigValidationError(t *testing.T) { |
There was a problem hiding this comment.
The test function name "TestSetInstalledStatusFromRevisionStates_ConfigValidationError" is misleading. The test cases now include scenarios beyond config validation errors, such as "rolling revision with RollingOut reason - uses Installing", "rolling revision with no conditions set - uses Absent", and "rolling revision with unknown reason - uses Absent". Consider renaming this test to something more general like "TestSetInstalledStatusFromRevisionStates_NoInstalledBundle" or "TestSetInstalledStatusFromRevisionStates_InstallationInProgress" to better reflect its broader scope.
| func TestSetInstalledStatusFromRevisionStates_ConfigValidationError(t *testing.T) { | |
| func TestSetInstalledStatusFromRevisionStates_NoInstalledBundle(t *testing.T) { |
| func TestSetInstalledStatusFromRevisionStates_Upgrade(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| revisionStates *RevisionStates | ||
| expectedInstalledCond metav1.Condition | ||
| }{ | ||
| { | ||
| name: "installed bundle with healthy upgrade in progress - uses Upgrading", | ||
| revisionStates: &RevisionStates{ | ||
| Installed: &RevisionMetadata{ | ||
| RevisionName: "rev-1", | ||
| Image: "quay.io/example/bundle:v1.0.0", | ||
| BundleMetadata: ocv1.BundleMetadata{ | ||
| Name: "example.v1.0.0", | ||
| Version: "1.0.0", | ||
| }, | ||
| }, | ||
| RollingOut: []*RevisionMetadata{ | ||
| { | ||
| RevisionName: "rev-2", | ||
| Conditions: []metav1.Condition{ | ||
| { | ||
| Type: ocv1.ClusterExtensionRevisionTypeProgressing, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: ocv1.ReasonRollingOut, | ||
| Message: "Upgrading to v2.0.0", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expectedInstalledCond: metav1.Condition{ | ||
| Type: ocv1.TypeInstalled, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: ocv1.ReasonUpgrading, | ||
| }, | ||
| }, | ||
| { | ||
| name: "installed bundle with no rolling revisions - uses Succeeded", | ||
| revisionStates: &RevisionStates{ | ||
| Installed: &RevisionMetadata{ | ||
| RevisionName: "rev-1", | ||
| Image: "quay.io/example/bundle:v1.0.0", | ||
| BundleMetadata: ocv1.BundleMetadata{ | ||
| Name: "example.v1.0.0", | ||
| Version: "1.0.0", | ||
| }, | ||
| }, | ||
| RollingOut: nil, | ||
| }, | ||
| expectedInstalledCond: metav1.Condition{ | ||
| Type: ocv1.TypeInstalled, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: ocv1.ReasonSucceeded, | ||
| }, | ||
| }, | ||
| { | ||
| name: "installed bundle with rolling revision not yet healthy - uses Succeeded", | ||
| revisionStates: &RevisionStates{ | ||
| Installed: &RevisionMetadata{ | ||
| RevisionName: "rev-1", | ||
| Image: "quay.io/example/bundle:v1.0.0", | ||
| BundleMetadata: ocv1.BundleMetadata{ | ||
| Name: "example.v1.0.0", | ||
| Version: "1.0.0", | ||
| }, | ||
| }, | ||
| RollingOut: []*RevisionMetadata{ | ||
| { | ||
| RevisionName: "rev-2", | ||
| Conditions: []metav1.Condition{}, | ||
| }, | ||
| }, | ||
| }, | ||
| expectedInstalledCond: metav1.Condition{ | ||
| Type: ocv1.TypeInstalled, | ||
| Status: metav1.ConditionTrue, | ||
| Reason: ocv1.ReasonSucceeded, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| ext := &ocv1.ClusterExtension{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-ext", | ||
| Generation: 1, | ||
| }, | ||
| } | ||
|
|
||
| setInstalledStatusFromRevisionStates(ext, tt.revisionStates) | ||
|
|
||
| cond := meta.FindStatusCondition(ext.Status.Conditions, ocv1.TypeInstalled) | ||
| require.NotNil(t, cond) | ||
| require.Equal(t, tt.expectedInstalledCond.Status, cond.Status) | ||
| require.Equal(t, tt.expectedInstalledCond.Reason, cond.Reason) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The TestSetInstalledStatusFromRevisionStates_Upgrade test is missing a test case for when an installed bundle has a rolling revision with Reason: Retrying (indicating an upgrade failure). According to the logic in setInstalledStatusFromRevisionStates (lines 71-79), this should result in Installed: True with Reason: Succeeded (showing the current bundle is still installed despite the upgrade failing). Add a test case to verify this behavior is correct and intentional.
| latestRevision := revisionStates.RollingOut[len(revisionStates.RollingOut)-1] | ||
| progressingCond := apimeta.FindStatusCondition(latestRevision.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing) | ||
|
|
||
| if progressingCond != nil && progressingCond.Reason == string(ocv1.ReasonRollingOut) { |
There was a problem hiding this comment.
The check for upgrade status on line 75 only verifies that progressingCond.Reason equals RollingOut, but doesn't verify the condition Status. This could potentially report Upgrading even if the condition status is False or Unknown. Consider adding a check for progressingCond.Status == metav1.ConditionTrue to ensure the upgrade is actually progressing successfully before reporting the Upgrading status.
| if progressingCond != nil && progressingCond.Reason == string(ocv1.ReasonRollingOut) { | |
| if progressingCond != nil && progressingCond.Status == metav1.ConditionTrue && progressingCond.Reason == string(ocv1.ReasonRollingOut) { |
Description
Reviewer Checklist