🐛 fix(Boxcutter): Use Installing/Upgrading reasons for active operations#2473
🐛 fix(Boxcutter): Use Installing/Upgrading reasons for active operations#2473camilamacedo86 wants to merge 1 commit intooperator-framework:mainfrom
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.
Pull request overview
This PR improves the status reporting for ClusterExtension operations by introducing two new condition reasons (Installing and Upgrading) to replace the generic Absent reason during active operations. This provides clearer status feedback to users about whether a bundle is being installed for the first time or being upgraded.
Changes:
- Added
ReasonInstallingandReasonUpgradingconstants to the API types - Updated controller logic to detect and set these new reasons appropriately
- Added comprehensive test coverage for both installation and upgrade scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| api/v1/common_types.go | Adds new ReasonInstalling and ReasonUpgrading constants for the Installed condition type |
| internal/operator-controller/conditionsets/conditionsets.go | Registers the new condition reasons in the complete set of valid reasons |
| internal/operator-controller/controllers/common_controller.go | Updates setInstalledStatusFromRevisionStates to detect upgrades, renames determineFailureReason to determineInstalledReason with logic to return Installing for healthy rollouts, and adds setInstalledStatusConditionUpgrading helper function |
| internal/operator-controller/controllers/common_controller_test.go | Adds comprehensive test coverage for upgrade scenarios and updates existing tests to verify Installing reason usage during initial installations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2473 +/- ##
==========================================
+ Coverage 69.55% 69.62% +0.06%
==========================================
Files 102 102
Lines 8354 8371 +17
==========================================
+ Hits 5811 5828 +17
Misses 2079 2079
Partials 464 464
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:
|
camilamacedo86
force-pushed
the
absent-fix
branch
2 times, most recently
from
f38b32f to
2e8c1d2
Compare
There was a problem hiding this comment.
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.
camilamacedo86
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
camilamacedo86
force-pushed
the
absent-fix
branch
from
2e8c1d2 to
a8990a2
Compare
camilamacedo86
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 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.
| }, | ||
| }, | ||
| { | ||
| name: "installed bundle with multiple rolling revisions including errors - uses Succeeded", |
There was a problem hiding this comment.
Can you explain this test better? Why is it considered successful, if it's retrying?
camilamacedo86
force-pushed
the
absent-fix
branch
2 times, most recently
from
2ee13cb to
734a6b3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
absent-fix
branch
from
734a6b3 to
526973c
Compare
camilamacedo86
force-pushed
the
absent-fix
branch
from
526973c to
7466b20
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilamacedo86
force-pushed
the
absent-fix
branch
from
7466b20 to
2f546e9
Compare
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: Cursors/Claude
camilamacedo86
force-pushed
the
absent-fix
branch
from
2f546e9 to
54413a9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 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.
openshift-ci
bot
added
lgtm
perdasilva
removed
approved
|
whoops - sorry I didn't mean to approve |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva, tmshort 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 |
openshift-ci
bot
added
the
approved
|
/hold @pedjak seems not sure if we should move with it. Discussion : https://redhat-internal.slack.com/archives/C096LBLFMFV/p1770122308536019?thread_ts=1770054468.987749&cid=C096LBLFMFV So, lets debate this one first |
openshift-ci
bot
added
the
do-not-merge/hold
3 participants
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.
We are also improving the coverage within it.
Address fedback/suggestion; #2465 (comment)