-
Notifications
You must be signed in to change notification settings - Fork 69
🐛 Fix deprecation conditions showing install errors and improve condition semantics #2296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
🐛 Fix deprecation conditions showing install errors and improve condition semantics #2296
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors deprecation status handling in ClusterExtension reconciliation to ensure deprecation conditions accurately reflect catalog data and installed bundle state, preventing install/validation errors from leaking into deprecation conditions.
- Moved deprecation status updates to a deferred function that runs at the end of reconciliation
- Changed BundleDeprecated condition to use
Unknownstatus withReasonAbsentwhen no bundle is installed - Updated test expectations to handle multiple possible error messages for sourceType validation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/operator-controller/controllers/clusterextension_controller.go | Refactored deprecation status logic with deferred updates and new condition semantics for uninstalled bundles |
| internal/operator-controller/controllers/clusterextension_controller_test.go | Added tests for deprecation handling with resolution failures and applier failures; updated test expectations for BundleDeprecated conditions |
| test/e2e/cluster_extension_install_test.go | Updated e2e tests to verify deprecation conditions in success and failure scenarios |
| internal/operator-controller/controllers/clusterextension_admission_test.go | Enhanced sourceType validation test to handle multiple possible error message formats |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_admission_test.go
Outdated
Show resolved
Hide resolved
0891f37 to
4f61e6a
Compare
4f61e6a to
2b26273
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2296 +/- ##
==========================================
+ Coverage 68.82% 68.87% +0.04%
==========================================
Files 100 100
Lines 7641 7665 +24
==========================================
+ Hits 5259 5279 +20
- Misses 1947 1951 +4
Partials 435 435
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2b26273 to
ad9ccfd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/operator-controller/controllers/clusterextension_controller_test.go:1
- The assertion
require.Contains(ct, []string{...}, cond.Reason)is semantically backwards. TheContainsfunction expects a slice as the second argument and the search item as the first. This should berequire.Contains(ct, cond.Reason, []string{ocv1.ReasonFailed, ocv1.ReasonAbsent})or userequire.True(ct, slices.Contains([]string{ocv1.ReasonFailed, ocv1.ReasonAbsent}, cond.Reason)).
package controllers_test
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
ad9ccfd to
a6d0678
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
a6d0678 to
287bede
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
287bede to
b2da76f
Compare
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4b6fc4e to
0dcaea9
Compare
0dcaea9 to
1e4f932
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller_test.go
Outdated
Show resolved
Hide resolved
1e4f932 to
8b8bd6b
Compare
8b8bd6b to
5dccbaa
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pedjak 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 |
5dccbaa to
edc779e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_controller_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/clusterextension_controller_test.go
Outdated
Show resolved
Hide resolved
edc779e to
1bdded5
Compare
Deprecation conditions now only show what matters: - When deprecated: condition shows "True" with deprecation message - When not deprecated: condition is absent (no clutter!) - When we can't check: condition shows "Unknown" This fixes three problems: 1. Install errors were leaking into deprecation conditions 2. Catalog unavailable showed "False" instead of "Unknown" 3. BundleDeprecated was checking the wrong bundle (resolved vs installed) Also improved the code: - Simpler logic (clear all, then add only what's needed) - Better reason values (Deprecated, DeprecationStatusUnknown, Absent) - Comprehensive test coverage for all scenarios Assisted-by: Cursor
1bdded5 to
8b6cfef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hey @joelanford — all your comments have been addressed. We also have an RFC here: https://docs.google.com/document/d/1_XtISZ74rsd9-Zh9oQje1mEHjAfx0CHlf_50UJkXHTQ I think it’s ready to 🚀 now. |
Fixes deprecation conditions showing install errors instead of actual deprecation status.
Closes: #2008
Addresses: Comment #3524229770, Comment #3524241806
Problems Fixed
1. Install errors leaked into deprecation conditions
When installation failed, the error appeared in ALL conditions including deprecation ones:
Users couldn't tell if something was actually deprecated.
2. Wrong bundle checked during upgrades
When upgrading v1.0.0 → v1.0.1, deprecation showed v1.0.1's status even when the upgrade failed.
This violates Kubernetes convention: status must show actual state, not desired state.
3. Too much YAML clutter
Every ClusterExtension showed 4 False deprecation conditions:
Users complained about cluttered output.
Changes Made
What we do now
status: Truestatus: Unknownstatus: Unknown, reason: AbsentKey improvements
Install errors stay separate
ProgressingandInstalledconditions onlyUse installed bundle (not resolved)
Remove False conditions
Correct reason values
Deprecated→ whenTrueDeprecationStatusUnknown→ whenUnknownAbsent→ when no bundle installed (BundleDeprecated only)Before/After Comparison
False(misleading) ❌Unknown(honest) ✅False(noisy) ❌Deprecated❌Examples
Example 1: Not Deprecated (Clean!)
Before:
After:
Example 2: Install Error (Bug #2008)
Before:
After:
Example 3: Catalog Removed
Before:
After:
Demos
Watch the fixes in action: