-
Notifications
You must be signed in to change notification settings - Fork 53
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
⚠️ (Partially) fix upgrade behaviour in Extension
#759
⚠️ (Partially) fix upgrade behaviour in Extension
#759
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -204,7 +204,7 @@ func TestExtensionResolve(t *testing.T) { | |||
extNN := types.NamespacedName{Name: ext.GetName(), Namespace: ext.GetNamespace()} | |||
|
|||
res, reconcileErr := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extNN}) | |||
assert.Equal(t, reconcileErr, tc.wantErr) | |||
assert.Equal(t, tc.wantErr, reconcileErr) |
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.
Should be expected and then actual.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #759 +/- ##
==========================================
- Coverage 68.15% 67.90% -0.26%
==========================================
Files 22 22
Lines 1429 1424 -5
==========================================
- Hits 974 967 -7
- Misses 390 391 +1
- Partials 65 66 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
HigherBundleVersion
predicateExtension
* `HigherBundleVersion` allows upgrades which are not allowed in `ClusterExtension`. For example, it is possible to ugprade `Extension` from one major version to another. * Existing `InMastermindsSemverRange` can be used instead reducing the amount of code to maintain a bit. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
00afa62
to
d73541f
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.
/lgtm
e079129
Description
With the current code there are a few issues:
catalogfilter.HigherBundleVersion
is not really required as we already have more genericcatalogfilter.InMastermindsSemverRange
.catalogfilter.HigherBundleVersion
allows upgrades from one major version to another major version, etc. This is not the same behaviour as inClusterExtension
replaces
here.This PR solves the first two, but the third still needs to be addressed.
How it currently works in
ClusterExtension
: https://github.com/operator-framework/operator-controller/blob/52cb7eb1a650731f2e16253a7f0f0ba25ea3af70/docs/drafts/upgrade-support.mdReviewer Checklist