-
Notifications
You must be signed in to change notification settings - Fork 47
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
Adds new catalogmetadata
package
#393
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
==========================================
+ Coverage 81.64% 83.65% +2.00%
==========================================
Files 21 27 +6
Lines 937 1101 +164
==========================================
+ Hits 765 921 +156
- Misses 119 123 +4
- Partials 53 57 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
9534267
to
a39d3fc
Compare
Is there a quick summary of why this change is being proposed? |
@stevekuznetsov we are looking into ways to remove entities and entity sources from operator-controller. This is just me playing with how we might interact with the catalog data. See #375 for actual removal (still WIP). |
482542a
to
b2174a4
Compare
74a2192
to
b5f00f5
Compare
|
||
// ByVersion is a sort "less" function that orders bundles | ||
// in inverse version order (higher versions on top). | ||
func ByVersion(b1, b2 *catalogmetadata.Bundle) bool { |
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 we not also sort by package name as we did before with entities? If we don't, what I'm imagining is our list of:
- packageName: a-package
version: 1.0.0
- packageName: b-package
version: 2.0.0
- packageName: a-package
version: 3.0.0
would get sorted to:
- packageName: a-package
version: 3.0.0
- packageName: b-package
version: 2.0.0
- packageName: a-package
version: 1.0.0
instead of getting grouped up by name followed by version:
- packageName: a-package
version: 3.0.0
- packageName: a-package
version: 1.0.0
- packageName: b-package
version: 2.0.0
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 think ordering by package is not important. We have the following use cases for old bundle entity based sort.ByChannelAndVersion
at the moment:
RequiredPackageVariableSource
- here we always filter by pacakge name so all bundles in sorting will have the same package name. For package name filtering we set a predicate here.- Twice in
InstalledPackageVariableSource
- on the first occasion we call it on a slice filtered by bundle image (so most likely same package) and on the second occasion we call it on a slice filtered byreplaces
which should also be within the same package BundlesAndDepsVariableSource
- here is the use case where we might sort different packages. A bundle might require multiple GVKs from different pacakge. But, I think, as long as dependencies are sorted by version from newest to oldest we should be ok.
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 that makes perfect sense, thanks for the explanation!
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'm not 100% that this is correct, but let's go with it and fix it if it doesn't work. Trying to not overcomplicate it :)
At the moment it contains: * Types which embed `declcfg.Package`, `declcfg.Channel` and `declcfg.Bundle`. Later we will be adding more methods to them (for unmarshalling properties, for example) * A function to fetch metadata by scheme & catalog name * `Filter` function which can filter all these 3 types using a given predicate * `And`, `Or` and `Not` composite predicates to be used with `Filter` Later we might expand/change this package to be more convinient in use with variable sources. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com> Signed-off-by: dtfranz <dfranz@redhat.com>
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
b5f00f5
to
d555222
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!
Description
Adding a new package to deal with catalog metadata.
We will be using it to replace existing enttiy soruces. At the moment
catalogmetadata
package contains:declcfg.Package
,declcfg.Channel
anddeclcfg.Bundle
.Later we will be adding more methods to them (for unmarshalling properties, for example). This PR already extends
Bundle
type.Package
,Channel
orBundle
structs.catalogmetadata/filter
subpackage:Filter
function which can filter all these 3 types using a given predicateAnd
,Or
andNot
composite predicates to be used withFilter
catalogmetadata/client
subpackage:catalogmetadata/sort
subpackage:catalogmetadata
structs (e.g. byndles by version).This will eventually replace the following:
internal/resolution/entities
internal/resolution/entitysources
internal/resolution/util/predicates
internal/resolution/util/sort
(most likely)cmd/resolutioncli/entity_source.go
Reviewer Checklist