-
Notifications
You must be signed in to change notification settings - Fork 66
🌱 OPRUN-4122 Drop hash computation of ClusterExtensionRevision
phases
#2245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 OPRUN-4122 Drop hash computation of ClusterExtensionRevision
phases
#2245
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fe70c78
to
edc06d8
Compare
ClusterExtensionRevision
phases
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
testdata/images/bundles/test-operator/v1.2.0/manifests/bundle.configmap.yaml
Show resolved
Hide resolved
testdata/images/bundles/test-operator/v1.2.0/manifests/testoperator.clusterserviceversion.yaml
Outdated
Show resolved
Hide resolved
edc06d8
to
9547776
Compare
ClusterExtensionRevision
phases ClusterExtensionRevision
phases
9547776
to
a756ba7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
a756ba7
to
9e94fd2
Compare
[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 |
ClusterExtensionRevision
phases ClusterExtensionRevision
phases
94f2e67
into
operator-framework:main
Description
Applier can decide if a new
ClusterExtensionRevision
needs to be createdwithout computing the digest all objects in phases:
to update an immutable field (phases included)
Benefits:
ClusterExtensionRevision
as annotationChanges:
TestClusterExtensionForceInstallNonSuccessorVersion
testtest/e2e/cluster_extension_install_test.go
extracted intotest/helpers/helpers.go
so that it could be used intest/experimental-e2e/experimental_e2e_test.go
as wellReviewer Checklist