Implement ProviderFactory and ProviderClassFactory #3131
Conversation
| @@ -0,0 +1,223 @@ | |||
| // Copyright 2024 Stacklok, Inc. | |||
There was a problem hiding this comment.
The code here is mostly copy-paste from ProviderBuilder. The main difference is that the process of figuring out which type of credential is determined dynamically here, whereas ProviderBuilder requires it to be figured out when the ProviderBuilder struct is instantiated.
There was a problem hiding this comment.
Another note: I have decided to rely on existing test code to verify this logic once it is wired up in the application.
| } | ||
|
|
||
| func (p *providerFactory) BuildFromID(ctx context.Context, providerID uuid.UUID) (v1.Provider, error) { | ||
| config, err := p.store.GetByID(ctx, providerID) |
There was a problem hiding this comment.
I've taken to referring to db.Provider as the "provider configuration" which I feel is an accurate description of its intended purpose. In future, I may introduce a PR to rename the struct type (but not change the table, the rename can be done using sqlc's configuration). This avoids the confusion from having two types called "Provider" in the system which are used for very different things.
| @@ -133,3 +134,12 @@ func ParseAndValidate(rawConfig json.RawMessage, to any) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| // As is a type-cast function for Providers | |||
| func As[T Provider](provider Provider) (T, error) { | |||
There was a problem hiding this comment.
It will be used in the next PR. It will be used as a helper function for casting the Provider to a specific trait
Relates to #2845 Introduce two new constructs for creating providers. The top-level ProviderFactory takes the name/project or ID of a provider and instantiates it. In order to instantiate it, it checks the class of the provider, and delegates it to a ProviderClassFactory, which is responsible for creating it. This split exists so that we can register new types of provider while minimizing changes to existing code. This PR does not make use of the new interfaces. Instead, it just adds the implementations. This is to simplify review - the process of getting rid of the ProviderBuilder will involve a large number of small changes to many parts of code, and that will happen in another PR. Some points worth noting: 1) This does not provide a ProviderClassFactory for REST or Git since we do not use standalone instances of those providers at this point in time. It should be trivial to write factories for those at the point in time when we need them. 2) Based on discussion with Ozz and Jakub, I have decided to assume that the provider class column of the providers table will not be null for valid providers. This approach uses the class to figure out which concrete type is used to instantiate the provider, and is orthogonal to the traits, which describe the set of interfaces which that provider can support. In a future PR, I will make the class column non-nullable.
dmjb
force-pushed
the
provider-factory-iii
branch
from
4fc12cb
to
b62306e
Compare
| return nil, fmt.Errorf("provider version not supported") | ||
| } | ||
|
|
||
| creds, err := g.getProviderCredentials(ctx, config) |
There was a problem hiding this comment.
There are some places where we don't use the DB credential and pass a different credential into the provider constructor.
I think it's valid to not use a provider in those cases, or pass the credential into the function call.
I'm only calling it out here to be aware of it when we refactor that piece of code later.
Relates to #2845
Introduce two new constructs for creating providers. The top-level
ProviderFactorytakes the name/project or ID of a provider and instantiates it. In order to instantiate it, it checks the class of the provider, and delegates it to aProviderClassFactory, which is responsible for creating the specific implementation of the provider. This split exists so that we can register new types of provider while minimizing changes to existing code.It is intended that this will replace all uses of
ProviderBuilder.ProviderBuilderrequires the caller to know various details about provider creation, and is therefore a leaky abstraction.ProviderFactoryis intended to provide a "black box" interface for creating providers which does not require various provider-creation dependencies to be passed around in the code.This PR does not make use of the new interfaces. Instead, it just adds the implementations. This is to simplify review - the process of getting rid of the ProviderBuilder will involve a large number of small changes to many parts of code, and that will happen in another PR.
Some points worth noting:
do not use standalone instances of those providers at this point in
time. It should be trivial to write factories for those at the point
in time when we need them.
the provider class column of the providers table will not be null for
valid providers. This approach uses the class to figure out which
concrete type is used to instantiate the provider, and is orthogonal
to the traits, which describe the set of interfaces which that
provider can support. In a future PR, I will make the class column
non-nullable.
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: