Skip to content

Conversation

pedjak
Copy link
Contributor

@pedjak pedjak commented Oct 2, 2025

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)

@openshift-ci 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 Oct 2, 2025
Copy link

netlify bot commented Oct 2, 2025

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...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

codecov bot commented Oct 2, 2025

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.

@pedjak pedjak force-pushed the remove-hash-usage branch from fe70c78 to edc06d8 Compare October 6, 2025 22:48
@pedjak pedjak changed the title wip remove phase hashing at CER applier ✨ Drop hash computation of ClusterExtensionRevision phases Oct 6, 2025
@pedjak pedjak marked this pull request as ready for review October 6, 2025 22:51
@pedjak pedjak requested a review from a team as a code owner October 6, 2025 22:51
@openshift-ci 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 Oct 6, 2025
@openshift-ci openshift-ci bot requested review from dtfranz and tmshort October 6, 2025 22:51
@pedjak pedjak requested a review from perdasilva October 6, 2025 22:52
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better ^^

}, pollDuration, pollInterval)
}

func TestClusterExtensionForceInstallNonSuccessorVersion(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@pedjak pedjak force-pushed the remove-hash-usage branch from edc06d8 to 9547776 Compare October 10, 2025 15:41
@pedjak pedjak changed the title ✨ Drop hash computation of ClusterExtensionRevision phases 🌱 Drop hash computation of ClusterExtensionRevision phases Oct 10, 2025
@pedjak pedjak force-pushed the remove-hash-usage branch from 9547776 to a756ba7 Compare October 10, 2025 16:03
Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thank you!!

@openshift-ci openshift-ci bot 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
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
@pedjak pedjak force-pushed the remove-hash-usage branch from a756ba7 to 9e94fd2 Compare October 13, 2025 11:20
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2025
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2025
Copy link

openshift-ci bot commented Oct 13, 2025

[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

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

@pedjak pedjak changed the title 🌱 Drop hash computation of ClusterExtensionRevision phases 🌱 OPRUN-4122 Drop hash computation of ClusterExtensionRevision phases Oct 13, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 94f2e67 into operator-framework:main Oct 13, 2025
26 checks 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