:seedling: OPRUN-4122 Drop hash computation of `ClusterExtensionRevision` phases by pedjak · Pull Request #2245 · operator-framework/operator-controller

pedjak · 2025-10-02T13:38:56Z

Description

Applier can decide if a new ClusterExtensionRevision needs to be created
without computing the digest all objects in phases:

try to patch the current revision
if the operation fails due to invalid payload, it is a signal that we tried
to update an immutable field (phases included)
in that case, create a new revision

Benefits:

No need to keep the computed digest attached to ClusterExtensionRevision as annotation
Revisions are created using SSA, passing the right field owner
Simpler applier logic

Changes:

Unit tests updated, rephrasing their names to better reflect the use case scenario under test
Added test-operator 1.2.0 bundle to be able to assert creation of new revision in added e2e TestClusterExtensionForceInstallNonSuccessorVersion test
Helper function previously living in test/e2e/cluster_extension_install_test.go extracted into test/helpers/helpers.go so that it could be used in
test/experimental-e2e/experimental_e2e_test.go as well

Reviewer Checklist

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

netlify · 2025-10-02T13:39:05Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`9e94fd2`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/68ece06b87c63c0008e59fac
😎 Deploy Preview	https://deploy-preview-2245--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.

codecov · 2025-10-02T13:50:57Z

Codecov Report

❌ Patch coverage is 75.00000% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.91%. Comparing base (687401c) to head (9e94fd2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/operator-controller/applier/boxcutter.go	72.97%	6 Missing and 4 partials ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #2245 +/- ##
==========================================
+ Coverage 69.94% 72.91% +2.96% 
==========================================
Files 88 88 Lines 8733 8738 +5 ==========================================
+ Hits 6108 6371 +263 + Misses 2205 1953 -252 + Partials 420 414 -6

Flag	Coverage Δ
e2e	`39.40% <0.00%> (+0.11%)`	⬆️
experimental-e2e	`46.67% <75.00%> (?)`
unit	`58.01% <67.50%> (+0.01%)`	⬆️

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.

perdasilva · 2025-10-09T07:29:15Z

+	t.Log("By updating the ClusterExtension resource to a non-successor version")
+	// 1.2.0 does not replace/skip/skipRange 1.0.0.
+	clusterExtension.Spec.Source.Catalog.Version = "1.2.0"
+	clusterExtension.Spec.Source.Catalog.UpgradeConstraintPolicy = ocv1.UpgradeConstraintPolicySelfCertified


Suggested change
clusterExtension.Spec.Source.Catalog.UpgradeConstraintPolicy=ocv1.UpgradeConstraintPolicySelfCertified
t.Log("By setting the upgrade constraint policy to self-certified")
clusterExtension.Spec.Source.Catalog.UpgradeConstraintPolicy=ocv1.UpgradeConstraintPolicySelfCertified

I would say it should probably be then By forcing updating the ClusterExtension resource to a non-successor version - setting to self-certified policy is just the way to force such updates. wdyt?

Even better ^^

perdasilva · 2025-10-09T07:31:14Z

 	}, pollDuration, pollInterval)
 }

+func TestClusterExtensionForceInstallNonSuccessorVersion(t *testing.T) {


Would it make sense to have a negative test as well by either having a separate test where we don't update the upgrade policy, or having this test first try with the standard policy, watch it fail, then update to self-certified and watch it succeed?

The test is not about changing the upgrade policy - is is about asserting that a new revision is created if objects in the new version are different from the previous installed version. Such test did not exist previously.
The additional tests are not bound to the experimental e2e suite and could be added in a separate PR.

From the title and behavior it seems to be about ensuring that the user can force install to a version without an upgrade edge from the current one (which of course implies the creation of a new revision). Maybe a new title?

perdasilva

Awesome! Thank you!!

Applier can decide if a new `ClusterExtensionRevision` needs to be created without computing the digest all objects in phases: * try to patch the current revision * if the operation fails due to invalid payload, it is a signal that we tried to update an immutable field (phases included) * in that case, create a new revision Benefits: * No need to keep the computed digest attached to `ClusterExtensionRevision` as annotation * Revisions are created using SSA, passing the right field owner * Simpler applier logic Changes: * Unit tests updated, rephrasing their names to better reflect the use case scenario under test * Added test-operator 1.2.0 bundle to be able to assert creation of new revision in added e2e `TestClusterExtensionForceInstallNonSuccessorVersion` test * Helper function previously living in `test/e2e/cluster_extension_install_test.go` extracted into `test/helpers/helpers.go` so that it could be used in `test/experimental-e2e/experimental_e2e_test.go` as well

openshift-ci · 2025-10-13T11:22:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: perdasilva

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:

~~OWNERS~~ [perdasilva]

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

openshift-ciBot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2025

pedjak force-pushed the remove-hash-usage branch from fe70c78 to edc06d8Compare October 6, 2025 22:48

pedjak changed the title ~~wip remove phase hashing at CER applier~~✨ Drop hash computation of ClusterExtensionRevision phases Oct 6, 2025

pedjak marked this pull request as ready for review October 6, 2025 22:51

pedjak requested a review from a team as a code owner October 6, 2025 22:51

openshift-ciBot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2025

openshift-ciBot requested review from dtfranz and tmshort October 6, 2025 22:51

pedjak requested a review from perdasilva October 6, 2025 22:52

camilamacedo86 reviewed Oct 7, 2025
View reviewed changes

Comment threadinternal/operator-controller/applier/boxcutter.go Outdated

camilamacedo86 reviewed Oct 7, 2025
View reviewed changes

Comment threadinternal/operator-controller/applier/boxcutter.go Outdated

camilamacedo86 reviewed Oct 7, 2025
View reviewed changes

Comment threadcmd/operator-controller/main.go

perdasilva reviewed Oct 9, 2025
View reviewed changes

Comment threadtestdata/images/bundles/test-operator/v1.2.0/manifests/bundle.configmap.yaml

perdasilva reviewed Oct 9, 2025
View reviewed changes

Comment threadtestdata/images/bundles/test-operator/v1.2.0/manifests/testoperator.clusterserviceversion.yaml Outdated

perdasilva reviewed Oct 9, 2025
View reviewed changes

Comment threadinternal/operator-controller/applier/boxcutter_test.go

pedjak force-pushed the remove-hash-usage branch from edc06d8 to 9547776Compare October 10, 2025 15:41

pedjak requested review from camilamacedo86 and perdasilva October 10, 2025 15:41

pedjak changed the title ~~✨ Drop hash computation of ClusterExtensionRevision phases~~ 🌱 Drop hash computation of ClusterExtensionRevision phases Oct 10, 2025

pedjak force-pushed the remove-hash-usage branch from 9547776 to a756ba7Compare October 10, 2025 16:03

perdasilva approved these changes Oct 13, 2025
View reviewed changes

openshift-ciBot assigned perdasilva Oct 13, 2025

openshift-ciBot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 13, 2025

pedjak added 2 commits October 13, 2025 13:12

Address reviewer comments
9e94fd2

pedjak force-pushed the remove-hash-usage branch from a756ba7 to 9e94fd2Compare October 13, 2025 11:20

openshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2025

perdasilva approved these changes Oct 13, 2025
View reviewed changes

openshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2025

pedjak changed the title ~~🌱 Drop hash computation of ClusterExtensionRevision phases~~ 🌱 OPRUN-4122 Drop hash computation of ClusterExtensionRevision phases Oct 13, 2025

openshift-merge-botBot merged commit 94f2e67 into operator-framework:mainOct 13, 2025
26 checks passed

pedjak mentioned this pull request Nov 17, 2025
⚠️ (Boxcutter Runtime) Revision discovery now uses label-based cache lookups, removing Spec.Previous and preventing orphaned revisions on delete. #2315
Merged

	clusterExtension.Spec.Source.Catalog.UpgradeConstraintPolicy=ocv1.UpgradeConstraintPolicySelfCertified
	t.Log("By setting the upgrade constraint policy to self-certified")
	clusterExtension.Spec.Source.Catalog.UpgradeConstraintPolicy=ocv1.UpgradeConstraintPolicySelfCertified

Uh oh!

Conversation

pedjak commented Oct 2, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlifyBot commented Oct 2, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

codecovBot commented Oct 2, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perdasilvaOct 9, 2025

Choose a reason for hiding this comment

Uh oh!

pedjakOct 10, 2025

Choose a reason for hiding this comment

Uh oh!

perdasilvaOct 11, 2025

Choose a reason for hiding this comment

Uh oh!

perdasilvaOct 9, 2025

Choose a reason for hiding this comment

Uh oh!

pedjakOct 10, 2025

Choose a reason for hiding this comment

Uh oh!

perdasilvaOct 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perdasilva left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ciBot commented Oct 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pedjak commented Oct 2, 2025•
edited
Loading

netlifyBot commented Oct 2, 2025•
edited
Loading

codecovBot commented Oct 2, 2025•
edited
Loading