(Blocked by: PR #2296 ) 🐛 Add deprecated reason #2300
Conversation
- stop copying install/validation errors into deprecation conditions - base bundle deprecation on the installed bundle (Unknown when none) - extend unit tests to cover resolver failures with catalog deprecations Assisted-by: Cursor
- leave BundleDeprecated at Unknown/Absent when nothing installs - flip package/channel/bundle to True/Deprecated when the catalog says so - keep the conditions at False/NotDeprecated (empty message) when no warnings exist Assisted-by: Cursor
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/hold Until we get merged: #2296 |
openshift-ci
bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the deprecation condition handling in the ClusterExtension controller to prevent installation errors from leaking into deprecation conditions. The key change is deferring deprecation status updates until after installation attempts complete, ensuring that BundleDeprecated remains Unknown/Absent when no bundle is installed and accurately reflects catalog data only for successfully installed bundles.
- Introduces a new
ReasonNotDeprecatedconstant for non-deprecated resources - Refactors
SetDeprecationStatusto properly handle bundle deprecation based on installation state - Updates deprecation message separator from semicolon to newline for better readability
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/cluster_extension_install_test.go | Adds validation for deprecation conditions in E2E tests, verifying BundleDeprecated status when bundles are/aren't installed |
| internal/operator-controller/controllers/common_controller_test.go | Updates deprecation message separator from semicolon to newline in tests |
| internal/operator-controller/controllers/clusterextension_controller_test.go | Adds comprehensive test coverage for deprecation condition behavior during resolution and applier failures |
| internal/operator-controller/controllers/clusterextension_controller.go | Refactors deprecation handling to use deferred updates and introduces helper functions for building deprecation info |
| internal/operator-controller/controllers/clusterextension_admission_test.go | Updates admission test to handle multiple possible error message formats |
| internal/operator-controller/conditionsets/conditionsets.go | Adds ReasonNotDeprecated to the list of valid condition reasons |
| api/v1/common_types.go | Defines the new ReasonNotDeprecated constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bundleDeprecatedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeBundleDeprecated) | ||
| require.NotNil(ct, bundleDeprecatedCond) | ||
| require.Equal(ct, metav1.ConditionFalse, bundleDeprecatedCond.Status) | ||
| require.Equal(ct, ocv1.ReasonDeprecated, bundleDeprecatedCond.Reason) |
There was a problem hiding this comment.
Incorrect reason for non-deprecated bundle. When Status is ConditionFalse (bundle is not deprecated), the Reason should be ocv1.ReasonNotDeprecated, not ocv1.ReasonDeprecated. This is inconsistent with the controller logic at lines 447-448 in clusterextension_controller.go where ReasonNotDeprecated is set for non-deprecated bundles.
| require.Equal(ct, ocv1.ReasonDeprecated, bundleDeprecatedCond.Reason) | |
| require.Equal(ct, ocv1.ReasonNotDeprecated, bundleDeprecatedCond.Reason) |
1 participant
Closes: #2009
We need first fix the deprecation leak : #2296
This PR is about the second commit: a0dc469