Uh oh!
There was an error while loading. Please reload this page.
✨ Filter out bundle versions lower than installed - #711
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
b4301b5 to
156420eCompareCodecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #711 +/- ##
==========================================
+ Coverage 63.50% 68.15% +4.65%
==========================================
Files 22 22 Lines 1403 1429 +26 ==========================================
+ Hits 891 974 +83 + Misses 459 390 -69 - Partials 53 65 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
5e80cd5 to
05db735Compare| } | ||
| } | ||
| func HigherBundleVersion(currentVersion *bsemver.Version) Predicate[catalogmetadata.Bundle] { |
There was a problem hiding this comment.
This is a good start, but the end state needs to be the same as the current behavior of the ClusterExtension, which is one of:
- Semver semantics (if the "force semver" feature gate is enabled)
- Replaces, skips, skipRange semantics (which finds other bundles that have skips, replaces, skipRange that include the currently installed bundle)
There was a problem hiding this comment.
Actually, I'm not sure ClusterExtension handles skips and skipRange for the second case. But it needs to there too.
There was a problem hiding this comment.
@joelanford Coming back to this after a v0 distraction and had a follow-up question here: for the second case, are we looking for bundles that do all three (replaces and skips and skipRange) or any one of the three?
There was a problem hiding this comment.
Any, and then of all the matches, choose the one with the highest semver.
There was a problem hiding this comment.
Thanks @joelanford ! I was just about finished adding the feature gating and replaces/skips/skipsRange then realized it was going to make the PR significantly larger (mostly due to tests). How do you feel about me doing that as a follow-up?
There was a problem hiding this comment.
+1 on wrapping this and capturing a follow-up issue.
I've implemented this for ClusterExtension here: #743
Given the recent change roadmap updates, I think we should try to wrap up work on Extension API for now, and capture a "current state of the world" of the Extension API and behavior.
| func hasKappApis(config *rest.Config) (bool, error) { | ||
| discoveryClient, err := discovery.NewDiscoveryClientForConfig(config) | ||
| if err != nil { | ||
| return false, fmt.Errorf("creating discovery client: %v", err) | ||
| } | ||
| apiResourceList, err := discoveryClient.ServerResourcesForGroupVersion(carvelv1alpha1.SchemeGroupVersion.String()) | ||
| if err != nil && !errors.IsNotFound(err) { | ||
| return false, fmt.Errorf("listing resource APIs: %v", err) | ||
| } | ||
| if apiResourceList == nil { | ||
| return false, nil | ||
| } | ||
| for _, resource := range apiResourceList.APIResources { | ||
| if resource.Kind == "App" { | ||
| return true, nil | ||
| } | ||
| } | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
This change was taken from #690 , in which Varsha explained that the check is not needed due to the feature gate.
There was a problem hiding this comment.
That PR is undergoing a refactor to combine the tests into a test loop, which is likely going to take some time given its relative priority. This particular change is valuable though since it made doing these tests much easier.
| if currentVersion == nil { | ||
| return false | ||
| } | ||
| bundleVersion, err := bundle.Version() |
There was a problem hiding this comment.
Not for this PR, just a though. I wonder if we should validate the bundle version on creation/ingress rather than on egress (i.e. bundle.Version() shouldn't return an error). So, a bundle object is always validated and correct.
There was a problem hiding this comment.
I've had this thought too. If it didn't break any assumptions, it would be nice to get something into the declcfg.Bundle type for this.
There was a problem hiding this comment.
Yeah that makes great sense and also would make it a lot easier to grab.
There was a problem hiding this comment.
I'll create a discussion upstream - see what ppl think - or would a ticket suffice?
| Properties: []property.Property{ | ||
| { | ||
| Type: property.TypePackage, | ||
| Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0"}`), |
There was a problem hiding this comment.
Oh, I see. I guess we'd need some sort of factory is we want to validate on creation...
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Adds an annotation to the App created for each extension indicating the bundle version, which can then be used on future reconciles to filter out older versions. This can be overridden by setting the UpgradeConstraintPolicy to 'Ignore'. Co-authored-by: Varsha Prasad Narsing <varshaprasad96@gmail.com> Signed-off-by: dtfranz <dfranz@redhat.com>
dtfranz
commented
Apr 8, 2024
Thank you for reviewing @perdasilva , I've updated per your comments. Please take another look when you can and let me know if you see anything else! |
perdasilva
left a comment
There was a problem hiding this comment.
chef's kiss! Thanks mate!!
Description
Adds an annotation to the App created for each extension indicating the bundle version, which can then be used on future reconciles to filter out older versions. This can be overridden by setting the Extension's
UpgradeConstraintPolicytoIgnore.Closes#654
This PR borrows a lot of stuff from @varshaprasad96 's PR #690 so I've added her as co-author :)
Reviewer Checklist