🐛 Fix deprecation conditions leaking install errors #2296

camilamacedo86 · 2025-11-01T07:58:25Z

Description

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

TL'DR:

Before:

Any install or validation failure copied the error message into every deprecation condition (Deprecated, PackageDeprecated, ChannelDeprecated, BundleDeprecated). Bundle deprecation was taken from the “resolved” bundle, even if nothing actually installed, so a failed install still wrote False/Failed with the installer error.

After:
Deprecation conditions are updated only after reconcile finishes and only from catalog data.

Package/channel deprecations still flip based on catalog info, even when install fails.
Bundle deprecation now reflects the installed bundle: Unknown/Absent when nothing is on cluster, True/Deprecated only when the installed bundle is truly deprecated.

Tests cover both Helm and Boxcutter paths plus resolver failures with catalog deprecations. This stops the install error from leaking into deprecation status and matches the expected behavior described in Issue #2008.

Motivation

Closes: #2008

netlify · 2025-11-01T07:58:30Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`c15272d`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/690b193ae2f68d00086b1244
😎 Deploy Preview	https://deploy-preview-2296--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 project configuration.

Copilot

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 Unknown status with ReasonAbsent when 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

internal/operator-controller/controllers/clusterextension_admission_test.go

Copilot

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

codecov · 2025-11-01T08:46:07Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.29%. Comparing base (7c443eb) to head (c15272d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2296      +/-   ##
==========================================
+ Coverage   74.23%   74.29%   +0.05%     
==========================================
  Files          90       90              
  Lines        7014     7053      +39     
==========================================
+ Hits         5207     5240      +33     
- Misses       1398     1404       +6     
  Partials      409      409

Flag	Coverage Δ
e2e	`46.04% <82.22%> (+0.18%)`	⬆️
experimental-e2e	`48.31% <82.22%> (+0.15%)`	⬆️
unit	`58.98% <100.00%> (+0.19%)`	⬆️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

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. The Contains function expects a slice as the second argument and the search item as the first. This should be require.Contains(ct, cond.Reason, []string{ocv1.ReasonFailed, ocv1.ReasonAbsent}) or use require.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

Copilot

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

Copilot

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

camilamacedo86 · 2025-11-04T12:11:25Z

Hi @pedjak

I think we should also update the API docs:
https://github.com/operator-framework/operator-controller/blob/main/api/v1/clusterextension_types.go#L486-L491
given that we currently say that that we MAY set them if something is deprecated. This PR changes the behavior and sets the deprecated conditions always.

Thanks for calling that out!

I agree the block could be clearer overall, but this PR doesn’t change the behavior the comment describes—we still only surface deprecation conditions when the catalog tells us something is deprecated. The new logic just makes sure the controller actually honors what’s written there (and adds the Unknown case until a bundle lands). I’d rather keep doc polish out of this PR so we can tackle a broader wording update separately.

WDYT in a fallow up make / enhance doc api explaination for .. with this purpose?

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 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.

pedjak · 2025-11-04T12:30:02Z

I agree the block could be clearer overall, but this PR doesn’t change the behavior the comment describes—we still only surface deprecation conditions when the catalog tells us something is deprecated. The new logic just makes sure the controller actually honors what’s written there (and adds the Unknown case until a bundle lands). I’d rather keep doc polish out of this PR so we can tackle a broader wording update separately.

Maybe it is just how I read it, here is the actual snippet:

When the ClusterExtension is sourced from a catalog, if may also communicate a deprecation condition.
These are indications from a package owner to guide users away from a particular package, channel, or bundle.
BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog.
ChannelDeprecated is set if the requested channel is marked deprecated in the catalog.
PackageDeprecated is set if the requested package is marked deprecated in the catalog.
Deprecated is a rollup condition that is present when any of the deprecated conditions are present.

May sound to me that this condition is optional, but it is not anymore, right? Your PR changes the logic so that it is exposed always, in case when there is no deprecation the condition status if False, correct? If so, the description saying that BundleDeprecated is set if the requested bundle version is marked deprecated in the catalog. does not hold anymore, i.e. it should be changed to BundleDeprecated is set to True if the requested bundle version is marked deprecated in the catalog.

WDYT in a fallow up make / enhance doc api explaination for .. with this purpose?

What blocks us from doing it this PR?

camilamacedo86 · 2025-11-04T13:13:38Z

Hi @pedjak

On main we already set these conditions every reconcile. If the catalog had nothing, they still showed up as False. This PR keeps that behavior; the only tweak is that BundleDeprecated now stays Unknown/Absent until a bundle actually installs. See the issue: Install error message getting propagated to most conditions #2008
Once something installs, we look at that bundle: if the catalog marks it deprecated we set BundleDeprecated=True; if not, False; otherwise Unknown. The doc line could be clearer about that.

I’d rather keep this PR focused on the bug fix (stopping the install error leaks and handling Unknown/Absent correctly).
I agree the wording could be improved, but I’m a bit concerned that updating it here might give the wrong impression that we’re changing behavior, which could make it harder for this PR to land. That’s was only my concern.

However, following your suggestion, I updated the descriotion to be more compreenshive.

api/v1/clusterextension_types.go

Copilot

Pull Request Overview

Copilot reviewed 6 out of 6 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

pedjak

/lgtm does PR title need to contain both :bug: (fix): or :bug: is enough?

openshift-ci · 2025-11-04T13:35:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pedjak
Once this PR has been reviewed and has the lgtm label, please assign tmshort for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2025-11-04T14:40:28Z

New changes are detected. LGTM label has been removed.

Copilot

Pull Request Overview

Copilot reviewed 6 out of 6 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.go

api/v1/clusterextension_types.go

Copilot

Pull Request Overview

Copilot reviewed 12 out of 12 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.

- 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 - update docs to provide a more compreehnesinve info Assisted-by: Cursor

pedjak · 2025-11-05T10:52:41Z

internal/operator-controller/controllers/clusterextension_controller.go

+	defer func(resolvedDeprecationPtr **declcfg.Deprecation, revisionStatesPtr **RevisionStates) {
+		installedBundleName := ""
+		if revisionStatesPtr != nil && *revisionStatesPtr != nil && (*revisionStatesPtr).Installed != nil {
+			installedBundleName = (*revisionStatesPtr).Installed.Name
+		}
+		var resolvedDeprecationVal *declcfg.Deprecation
+		if resolvedDeprecationPtr != nil {
+			resolvedDeprecationVal = *resolvedDeprecationPtr
+		}
+		SetDeprecationStatus(ext, installedBundleName, resolvedDeprecationVal)
+	}(&resolvedDeprecation, &revisionStates)


Could we actually simplify the deferring function, i.e. avoid passing args, and declare it as a closure?

defer func() { installedBundleName := "" if revisionStates != nil && revisionStates.Installed != nil { installedBundleName = revisionStates.Installed.Name } SetDeprecationStatus(ext, installedBundleName, resolvedDeprecation) }()

pedjak · 2025-11-05T10:55:59Z

internal/operator-controller/controllers/clusterextension_controller.go

+		channelSet = sets.New(ext.Spec.Source.Catalog.Channels...)
+	} else {
+		channelSet = sets.New[string]()


to avoid else statement, we could simplify to:

channelSet := sets.New[string]() if ext.Spec.Source.Catalog != nil { channelSet.Insert(ext.Spec.Source.Catalog.Channels...) }

Copilot AI review requested due to automatic review settings November 1, 2025 07:58

camilamacedo86 requested a review from a team as a code owner November 1, 2025 07:58

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2025

openshift-ci bot requested a review from dtfranz November 1, 2025 07:58

openshift-ci bot requested a review from oceanc80 November 1, 2025 07:58

Copilot AI reviewed Nov 1, 2025

View reviewed changes

camilamacedo86 force-pushed the fix-issue-conditions branch from 0891f37 to 4f61e6a Compare November 1, 2025 08:27

Copilot AI review requested due to automatic review settings November 1, 2025 08:33

camilamacedo86 force-pushed the fix-issue-conditions branch from 4f61e6a to 2b26273 Compare November 1, 2025 08:33

Copilot AI reviewed Nov 1, 2025

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

camilamacedo86 force-pushed the fix-issue-conditions branch from 2b26273 to ad9ccfd Compare November 1, 2025 08:46

camilamacedo86 changed the title ~~WIP: 🐛 (fix):Fix deprecation conditions leaking install errors~~ 🐛 (fix):Fix deprecation conditions leaking install errors Nov 1, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 1, 2025

camilamacedo86 requested review from Copilot, joelanford and perdasilva November 1, 2025 08:47

Copilot AI reviewed Nov 1, 2025

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

camilamacedo86 force-pushed the fix-issue-conditions branch from ad9ccfd to a6d0678 Compare November 1, 2025 09:31

camilamacedo86 requested a review from Copilot November 1, 2025 09:32

Copilot AI reviewed Nov 1, 2025

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller.go Show resolved Hide resolved

camilamacedo86 force-pushed the fix-issue-conditions branch from a6d0678 to 287bede Compare November 3, 2025 09:38

camilamacedo86 requested a review from Copilot November 3, 2025 10:10

Copilot AI reviewed Nov 3, 2025

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

perdasilva reviewed Nov 3, 2025

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

camilamacedo86 force-pushed the fix-issue-conditions branch from 287bede to b2da76f Compare November 3, 2025 12:16

perdasilva reviewed Nov 3, 2025

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

perdasilva reviewed Nov 3, 2025

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

perdasilva reviewed Nov 3, 2025

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

Copilot AI reviewed Nov 4, 2025

View reviewed changes

camilamacedo86 force-pushed the fix-issue-conditions branch from e140939 to 90a53c0 Compare November 4, 2025 12:20

camilamacedo86 requested a review from pedjak November 4, 2025 12:22

Copilot AI review requested due to automatic review settings November 4, 2025 13:25

camilamacedo86 force-pushed the fix-issue-conditions branch 2 times, most recently from 5838d32 to 748e63f Compare November 4, 2025 13:30

camilamacedo86 commented Nov 4, 2025

View reviewed changes

api/v1/clusterextension_types.go Show resolved Hide resolved

Copilot AI reviewed Nov 4, 2025

View reviewed changes

internal/operator-controller/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

internal/operator-controller/controllers/clusterextension_controller.go Show resolved Hide resolved

pedjak approved these changes Nov 4, 2025

View reviewed changes

openshift-ci bot assigned pedjak Nov 4, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2025

camilamacedo86 changed the title ~~🐛 (fix):Fix deprecation conditions leaking install errors~~ 🐛 Fix deprecation conditions leaking install errors Nov 4, 2025

camilamacedo86 force-pushed the fix-issue-conditions branch from 748e63f to 442ef2b Compare November 4, 2025 14:40

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2025

Copilot AI review requested due to automatic review settings November 4, 2025 15:04

camilamacedo86 force-pushed the fix-issue-conditions branch from 442ef2b to 5b36d85 Compare November 4, 2025 15:04

Copilot AI reviewed Nov 4, 2025

View reviewed changes

camilamacedo86 force-pushed the fix-issue-conditions branch from 5b36d85 to 4e1486d Compare November 4, 2025 15:16

Copilot AI review requested due to automatic review settings November 4, 2025 15:20

camilamacedo86 force-pushed the fix-issue-conditions branch from 4e1486d to 52a2b72 Compare November 4, 2025 15:20

Copilot AI reviewed Nov 4, 2025

View reviewed changes

camilamacedo86 force-pushed the fix-issue-conditions branch from 52a2b72 to c15272d Compare November 5, 2025 09:30

pedjak reviewed Nov 5, 2025

View reviewed changes

🐛 Fix deprecation conditions leaking install errors #2296

Are you sure you want to change the base?

🐛 Fix deprecation conditions leaking install errors #2296

Uh oh!

Conversation

camilamacedo86 commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TL'DR:

Motivation

Uh oh!

netlify bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

codecov bot commented Nov 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

pedjak commented Nov 4, 2025

Uh oh!

camilamacedo86 commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 4, 2025

Uh oh!

openshift-ci bot commented Nov 4, 2025

Uh oh!

Copilot AI left a comment

camilamacedo86 commented Nov 1, 2025 •

edited

Loading

netlify bot commented Nov 1, 2025 •

edited

Loading

codecov bot commented Nov 1, 2025 •

edited

Loading

camilamacedo86 commented Nov 4, 2025 •

edited

Loading

camilamacedo86 commented Nov 4, 2025 •

edited

Loading