Create ProviderStore interface, migrate controlplane to use it #2842
Conversation
| @@ -0,0 +1,147 @@ | |||
| // Copyright 2024 Stacklok, Inc | |||
There was a problem hiding this comment.
Similar to my other PR - this relies on the existing tests in the controlplane to validate it. It should get its own dedicated test suite in due course.
| ) | ||
|
|
||
| // ProviderStore provides methods for retrieving Providers from the database | ||
| type ProviderStore interface { |
There was a problem hiding this comment.
The term Repository is often used for these types of interface. However, given that that word already means something in the minder codebase, I am reusing Store
There was a problem hiding this comment.
We also have the term "Registry" used. I could also get behind Factory if we're retiring ProviderBuilder as an interface.
| ) | ||
|
|
||
| // ProviderStore provides methods for retrieving Providers from the database | ||
| type ProviderStore interface { |
There was a problem hiding this comment.
We also have the term "Registry" used. I could also get behind Factory if we're retiring ProviderBuilder as an interface.
| type ProviderStore interface { | ||
| // GetByName returns the provider instance in the database as identified | ||
| // by its project ID and name. | ||
| GetByName(ctx context.Context, projectID uuid.UUID, name string) (*db.Provider, error) |
There was a problem hiding this comment.
Future plan is to return an interface here that's not tied to the DB, correct?
| GetByNameAndTrait( | ||
| ctx context.Context, | ||
| projectID uuid.UUID, | ||
| name string, | ||
| trait db.ProviderType, | ||
| ) ([]db.Provider, error) |
| type providerStore struct { | ||
| store db.Store | ||
| } | ||
|
|
||
| // NewProviderStore returns a new instance of ProviderStore. | ||
| func NewProviderStore(store db.Store) ProviderStore { | ||
| return &providerStore{store: store} | ||
| } |
There was a problem hiding this comment.
FWIW, I'm fine with having an exported struct that matches the interface, rather than needing constructors for all of these. But not a big deal either way.
internal/providers/store.go
Outdated
| ) | ||
| } | ||
|
|
||
| return &providers[0], nil |
There was a problem hiding this comment.
Do we need to check that len(providers) > 0 first?
There was a problem hiding this comment.
It looks like we assume that the argument will always come from findProviders; since this is only used one place, I'd be inclined to just inline it.
| return inferProvider(providers, nameFilter) | ||
| } | ||
|
|
||
| func (p *providerStore) GetByNameAndTrait( |
There was a problem hiding this comment.
Is it worth a comment that GetByNameAndTrait will never return an empty list of Providers and a nil error?
This provides an interface for retrieving Providers out of the database, and encapsulates the logic for doing so. This makes it easier to stub out the provider retrieval logic for testing, and decouples the controlplane from the details of finding the right provider for a request.
|
Replaced by this PR: #2900 |
Relates to #2845
This provides an interface for retrieving Providers out of the database, and encapsulates the logic for doing so. This makes it easier to stub out the provider retrieval logic for testing, and decouples the controlplane from the details of finding the right provider for a request.
There are other places in the codebase where this should be used, but that is an exercise for a future PR.
Summary
Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.
Fixes #(related issue)
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: