WIP: Remove Entities and EntitySources #375
Conversation
…actor catalogdsource into variable source, modify existing variablesources to use variables, move catalogd variablesource into nestedvariablesource Signed-off-by: dtfranz <dfranz@redhat.com>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Will fix #347 |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| func(inputVariableSource input.VariableSource) (input.VariableSource, error) { | ||
| return variablesources.NewCatalogdVariableSource(cl), nil | ||
| }, |
There was a problem hiding this comment.
I think this might have to go higher in the slice. Each variable source in this slice nests the one above and some of the variable sources depend on variables received from the nested sources.
E.g. I believe NewBundlesAndDepsVariableSource depends on variables produced by catalog variable source.
| type CatalogdVariable struct { | ||
| *input.SimpleVariable | ||
| dependencies []*BundleVariable | ||
| } |
There was a problem hiding this comment.
Do we use this one? Can't find a reference.
| @@ -44,12 +64,12 @@ func (es *CatalogdEntitySource) Filter(ctx context.Context, filter input.Predica | |||
| return resultSet, nil | |||
| } | |||
|
|
|||
| func (es *CatalogdEntitySource) GroupBy(ctx context.Context, fn input.GroupByFunction) (input.EntityListMap, error) { | |||
| func (es *CatalogdVariableSource) GroupBy(ctx context.Context, fn input.GroupByFunction) (map[string][]deppy.Variable, error) { | |||
There was a problem hiding this comment.
I believe we can remove Get, GroupBy and Iterate easily because we do not use them.
Filter has to be replaced somehow. It has a convenient API of accepting reusable predicate functions.
We might want to have some sort of filter package to help us work with variables. But I think it is reasonable to start without it and see whether we actually need it (e.g. do we have any repeating patterns?)
There was a problem hiding this comment.
I've been playing a bit with filtering & separate structures for catalog data. Here is what I came up with so far: #393
There was a problem hiding this comment.
After thinking about it more, I think #393 might not work well with filtering like "is this bundle replaces bundle X?" since in #393 we can filter either bundles or channels or packages at the time. And bundle itself doesn't contain information on channel membership.
Still thinking about how to structure data and/or code best to be able to do what we already doing in codebase.
| // these properties are lazy loaded as they are requested | ||
| bundlePackage *property.Package | ||
| providedGVKs []GVK | ||
| requiredGVKs []GVKRequired | ||
| requiredPackages []PackageRequired | ||
| channel *property.Channel | ||
| channelEntry *ChannelEntry | ||
| semVersion *bsemver.Version | ||
| bundlePath string | ||
| mediaType string | ||
| mu sync.RWMutex |
There was a problem hiding this comment.
Wondering if we should separate bundle variable (meant for deppy resolution) from bundle data struct holding data from catalog.
|
Let's close this. We have a new branch where we took a different approch. A PR will be created soon. |
Description
WIP PR for collaboration on the removal of Entities and EntitySources.
Reviewer Checklist