Skip to content

Conversation

@camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Jan 22, 2026

Enhance the code for better readability and avoid flakes e.g. https://github.com/operator-framework/operator-lifecycle-manager/actions/runs/21225377668/job/61071090835

Follow up: #3737

@anik120
Copy link
Member

anik120 commented Jan 22, 2026

Copying my comment from #3746 (comment):

Oh looks like I had the same question on that PR too #3737 (review) and there was an answer #3737 (comment)

Curious to know, what motivated this test?

We will remove the marketplace index.
The motivation is to ensure that if any catalog is removed, everything continues to work correctly (except for upgrades that explicitly require that catalog).
Additionally, we should ensure that the installed solution works even if we temporarily cannot reach the catalog—for example, if the registry is unavailable for any reason.

but I didn't get a chance to get around to the discussion.

The "workload" you get when an operator is installed is handled by a different controller responsible for reconciling the ClusterServiceVersion CRs (olm-operator), than the one responsible for reconciling the catalogs (catalog-operator).

This doc outlines that info https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/architecture.md#architecture

ie I'm trying to make the case that "when a catalog is removed, the CSVs are untouched" is an axiom

@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Jan 22, 2026

Hi @anik120

When a catalog is removed — or when we can’t retrieve catalog data — we should still ensure OLM continues to properly manage and reconcile all resources it already manages. From the end-user perspective, the only impact should be that upgrades can’t proceed (because there’s no catalog data). Anything previously installed should continue running and being reconciled normally.

That’s exactly what we’re validating with these tests.

Example: As a user, I want to update the Solution configuration and have the changes applied, even if the referenced Catalog no longer exists, so that I can continue managing the installation and OLM can keep reconciling it.

(see the description of the PR: #3737 )
Delete the Catalog
....
Config Management Test:

  • Update subscription to add environment variable
  • Verify OLM propagates it to deployment
  • Proves: OLM actively manages deployment based on subscription config

Same added for OLMv1. See: operator-framework/operator-controller#2439 ( with bug fixes since there we have an issue. We were not reconciling the content prior that PR). I hope that clarifies.

I hope that clarifies your questions of #3747 (comment).

@tmshort
Copy link
Contributor

tmshort commented Jan 22, 2026

/lgtm
/approve

Because this is a flake, I'm re-running e2e / e2e (0) to be sure.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2026
@openshift-ci
Copy link

openshift-ci bot commented Jan 22, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tmshort

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2026
@openshift-merge-bot openshift-merge-bot bot merged commit d30e23d into operator-framework:master Jan 22, 2026
20 checks passed
@camilamacedo86 camilamacedo86 deleted the fix-flake-e2e branch January 22, 2026 19:46
@anik120
Copy link
Member

anik120 commented Jan 22, 2026

Yea I looked at the test, and I'm still of the opinion that we should discuss this before adding a test.

I am coming from the perspective of us having decided we won't be adding anything to v0 unless absolutely necessary and as I said, with this whole premise being an axiom, this test is not necessary

Having said that, it's an e2e test, and won't affect customers. It will only affect developers so I'll leave it up to you guys to figure it out.

@camilamacedo86
Copy link
Contributor Author

Hi @anik120

This isn’t a feature. It’s an end-to-end test to verify the existing behavior and ensure we don’t break the expected behavior going forward. It does not affect developers it just prevent we broke the expected behaviour and help us to ensure that it works as intent to work. PS. It was discussed and followed the process.

@tmshort
Copy link
Contributor

tmshort commented Jan 22, 2026

Ugh... this merged before e2e (0) finished... waiting for results.

@tmshort
Copy link
Contributor

tmshort commented Jan 22, 2026

And it passed...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants