-
Notifications
You must be signed in to change notification settings - Fork 50
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
Catalogd integration foundations #175
Catalogd integration foundations #175
Conversation
…ic interfaces This commit: * Replaces the RegistryClient interface with a EntitySourceConnector interface * Renames the grpcGrpcClient to grpcClientConnector, so that it can be indicated that the grpcClientConnector is an implentation of EntitySourceConnector(i.e lays the ground work for future connector implemenation) * Renames the catalogSource_controller to entitysource_reconciler, to lay the ground work for entities being built from sources other than olm v0 catalogsources. tldr of what components looks like after changes: entitysource_reconciler uses EntitySourceConnector to connect to an entity source, and build a cache. Signed-off-by: Anik <anikbhattacharya93@gmail.com>
…uilder This commit: * Pulls the cache into it's own package, that the entitysource_reconciler(entity_cache_builder) then imports as an attribute (previously, the EntitySourceReconciler was implementing the input.Entity interface that requires a Get implementation, which was overriding the Get method of the controller manager. This was an additional indication that the cache build and maintainace should be in it's own package) * Renames the entitysource_reconciler to be entity_cache_builder tldr of what componenets look like after changes: entity_cache_builder uses an EntitySourceConnector to build and maintain a cache of entities Signed-off-by: Anik <anikbhattacharya93@gmail.com>
a38275d
to
baf71dc
Compare
} | ||
|
||
// TODO: find better way to identify catalogSources unmanaged by olm | ||
// func isManagedCatalogSource(catalogSource v1alpha1.CatalogSource) 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.
I need more information about what this is for. Since an entity_cache_builder shouldn't make any assumptions about what the nature of the entity is (catalogsource or otherwise), I've commented it out now, but once I've learned the purpose of this I can find a better place for this (possibly the catalogsource entitySourceConnector).
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 this was a remnant of the idea that operator-controller would:
- not need OLMv0's catalog-operator to actually reconcile
CatalogSource
objects - (therefore) the only catalog sources that would be usable in this context are ones where the spec tells us the address to connect to.
In the end, we decided to just run OLMv0 so that we could use any catalog source.
) | ||
|
||
type EntitySourceConnector interface { | ||
ListEntities(ctx context.Context, catsrc *v1alpha1.CatalogSource) ([]*input.Entity, error) |
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 I'm missing something here. This still seems specific to OLMv0 catalog sources. Based on the PR description, it seemed like the goal was to remove the assumptions about the underlying API type.
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.
My understanding of this PR is that it is trying to generalize the old OLMv0 CatalogSource controller into one that can be used for a different entity source as long as that entity source meets the EntitySourceConnector
interface.
After reading #173 (comment) and looking through the code a bit more I think this isn't necessary and is probably introducing too much complexity in the beginning. As mentioned in the comment, it looks like the Operator
controller expects to have a resolver that satisfies the deppy input.EntitySource
interface so I think all we need for integrating catalogd is:
- Create a new entity source for catalogd
- For now lets not worry about caching anything. Lets try to keep it barebones and just make a request for the resources we need every time.
- Replace:
Line 108 in ca22669
Resolver: resolution.NewOperatorResolver(mgr.GetClient(), catsrcReconciler),
with something like:
Resolver: resolution.NewOperatorResolver(mgr.GetClient(), catalogdEntitySrc)
@anik120 should this be closed? |
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. |
Pretty sure this is obsolete now, so closing. |
This PR:
EntitySourceConnector
interface and refactors the current catalogsource entitysource to be an implementation ofEntitySourceConnector
catalogsource_controller
to be aentity_cache_builder
, and refactors it such that it connects to an entity source using anEntitySourceConnector
, and builds and maintains the cache using information fetch from the entity source.closes #173