-
Notifications
You must be signed in to change notification settings - Fork 48
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
✨ Filter out bundle versions lower than installed #711
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,19 @@ func InMastermindsSemverRange(semverRange *mmsemver.Constraints) Predicate[catal | |
} | ||
} | ||
|
||
func HigherBundleVersion(currentVersion *bsemver.Version) Predicate[catalogmetadata.Bundle] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any, and then of all the matches, choose the one with the highest semver. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +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. |
||
return func(bundle *catalogmetadata.Bundle) bool { | ||
if currentVersion == nil { | ||
return false | ||
} | ||
bundleVersion, err := bundle.Version() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've had this thought too. If it didn't break any assumptions, it would be nice to get something into the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that makes great sense and also would make it a lot easier to grab. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll create a discussion upstream - see what ppl think - or would a ticket suffice? |
||
if err != nil { | ||
return false | ||
} | ||
return bundleVersion.GTE(*currentVersion) | ||
} | ||
} | ||
|
||
func InBlangSemverRange(semverRange bsemver.Range) Predicate[catalogmetadata.Bundle] { | ||
return func(bundle *catalogmetadata.Bundle) bool { | ||
bundleVersion, err := bundle.Version() | ||
|
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.
Why?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, why is it in yours? 😕
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.
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.