✨ Update Installed status condition handling by trgeiger · Pull Request #1314 · operator-framework/operator-controller

trgeiger · 2024-09-26T18:29:33Z

Description

Updates Installed status condition per status condition RFC. Closes#1291.

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-09-26T18:29:54Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`7783af5`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/66faefedfd9121000746ff9e
😎 Deploy Preview	https://deploy-preview-1314--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

everettraven · 2024-09-27T16:49:48Z

+ installedBundle, _ := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext)
+ if installedBundle == nil {
+ setInstallStatus(ext, nil)
+ setInstalledStatusConditionFailed(ext, err.Error())


I know we talked about this about during our sync yesterday, but wanted to pose the question here as well in case other maintainers have opinions on it:
Do we need to set the Installed condition to False here and overwrite any errors that may exist in it with the finalizer error?
My feeling is that we do not need to do this as a user may want to still be able to see the original failed installation message. We already set the Progressing condition with the finalizer error so I think that should be sufficient here.
Additionally, if the Installed condition did not previously exist, I don't think there is any reason to set it here. Non-existent Installed condition should be interpreted as "Unknown".
Maybe a more fundamental question (that popped into my head as I was typing ^) - if there is no reason for us to change any of the status related to installation of the extension can we just remove any logic to do with the installed status from the finalizer error handling?

codecov · 2024-09-30T14:28:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.95%. Comparing base (f169414) to head (7783af5).
Report is 2 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1314 +/- ##
==========================================
+ Coverage 75.76% 75.95% +0.19% 
==========================================
Files 40 41 +1 Lines 2430 2429 -1 ==========================================
+ Hits 1841 1845 +4 + Misses 415 411 -4 + Partials 174 173 -1

Flag	Coverage Δ
e2e	`58.25% <100.00%> (+0.06%)`	⬆️
unit	`52.69% <66.66%> (+0.39%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Tayler Geiger <tayler@redhat.com>

trgeiger requested a review from a team as a code owner September 26, 2024 18:29

trgeiger force-pushed the update-installed branch 3 times, most recently from a7841e0 to 6dcc6f8Compare September 27, 2024 14:52