Create interfaces for Provider instantiation #2845
Assignees
Labels
Comments
This was referenced Mar 28, 2024
dmjb
added a commit
that referenced
this issue
Apr 2, 2024
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.
10 tasks
dmjb
added a commit
that referenced
this issue
Apr 2, 2024
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.
dmjb
added a commit
that referenced
this issue
Apr 2, 2024
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.
dmjb
added a commit
that referenced
this issue
Apr 2, 2024
Relates to #2845 Migrate more queries for providers to ProviderStore. This will make it easier to refactor away direct access to the provider table in subequent PRs.
10 tasks
dmjb
added a commit
that referenced
this issue
Apr 2, 2024
Relates to #2845 Migrate more queries for providers to ProviderStore. This will make it easier to refactor away direct access to the provider table in subequent PRs.
dmjb
added a commit
that referenced
this issue
Apr 2, 2024
Relates to #2845 Any time the engine needs a provider, go through ProviderStore. This will simplify future refactoring
dmjb
added a commit
that referenced
this issue
Apr 2, 2024
Relates to #2845 Any time the engine needs a provider, go through ProviderStore. This will simplify future refactoring
10 tasks
evankanderson
pushed a commit
that referenced
this issue
Apr 2, 2024
Relates to #2845 Any time the engine needs a provider, go through ProviderStore. This will simplify future refactoring
dmjb
added a commit
that referenced
this issue
Apr 3, 2024
Relates to #2845 Migrate more queries for providers to ProviderStore. This will make it easier to refactor away direct access to the provider table in subequent PRs.
dmjb
added a commit
that referenced
this issue
Apr 3, 2024
Relates to #2845 Migrate more queries for providers to ProviderStore. This will make it easier to refactor away direct access to the provider table in subequent PRs.
dmjb
added a commit
that referenced
this issue
Apr 16, 2024
Relates to #2845 Allows both the provider DB record as well as instances of the Provider interface to be queried to determine if they support a particular trait. Will be used more in future PRs.
10 tasks
dmjb
added a commit
that referenced
this issue
Apr 17, 2024
Fixes #2845 Final step in the process in migrating over to provider ID instead of provider name.
dmjb
added a commit
that referenced
this issue
Apr 18, 2024
Provide `CanImplement` method for providers Relates to #2845 Allows both the provider DB record as well as instances of the Provider interface to be queried to determine if they support a particular trait. Will be used more in future PRs.
dmjb
added a commit
that referenced
this issue
Apr 18, 2024
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.
10 tasks
dmjb
added a commit
that referenced
this issue
Apr 18, 2024
Relates to #2845 Undo migration #35 after discussion with Ozz and Jakub this morning. We want the provider class to store the concrete type of the provider, this will be used by the ProviderFactory. After studying the code, there does not appear to be any situation in which this field will be null in the staging/prod DB.
dmjb
added a commit
that referenced
this issue
Apr 18, 2024
Relates to #2845 Undo migration #35 after discussion with Ozz and Jakub this morning. We want the provider class to store the concrete type of the provider, this will be used by the ProviderFactory. After studying the code, there does not appear to be any situation in which this field will be null in the staging/prod DB.
dmjb
added a commit
that referenced
this issue
Apr 19, 2024
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
added a commit
that referenced
this issue
Apr 19, 2024
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
added a commit
that referenced
this issue
Apr 19, 2024
Fixes #2845 Final step in the process in migrating over to provider ID instead of provider name.
dmjb
added a commit
that referenced
this issue
May 1, 2024
Relates to #2845 When creating a provider, the code previously inserted the provider into the database and then instantiated it in order to validate that the user ID is correct. Alter the code so that it just instantiates the client and some supporting structures to validate the user ID. This client deliberately does not use the client cache in order to simplify the code. Since this is the first usage of the client for that provider, we will never find a suitable client in the cache anyway.
dmjb
added a commit
that referenced
this issue
May 1, 2024
Relates to #2845 When creating a provider, the code previously inserted the provider into the database and then instantiated it in order to validate that the user ID is correct. Alter the code so that it just instantiates the client and some supporting structures to validate the user ID. This client deliberately does not use the client cache in order to simplify the code. Since this is the first usage of the client for that provider, we will never find a suitable client in the cache anyway.
dmjb
added a commit
that referenced
this issue
May 1, 2024
Relates to #2845 1. Move the bulk provider instantiation for remote repo listing behind the provider manager interface. 2. Change the repo creation method in the repo service to instantiate its own provider. The handler still queries the table of providers to search for an appropriate provider. I have decided to not put this behind the ProviderManager at this time since I feel like the right place to do this sort of logic may change in subsequent PRs.
dmjb
added a commit
that referenced
this issue
May 1, 2024
Relates to #2845 1. Move the bulk provider instantiation for remote repo listing behind the provider manager interface. 2. Change the repo creation method in the repo service to instantiate its own provider. The handler still queries the table of providers to search for an appropriate provider. I have decided to not put this behind the ProviderManager at this time since I feel like the right place to do this sort of logic may change in subsequent PRs.
10 tasks
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 1. Move the bulk provider instantiation for remote repo listing behind the provider manager interface. 2. Change the repo creation method in the repo service to instantiate its own provider. The handler still queries the table of providers to search for an appropriate provider. I have decided to not put this behind the ProviderManager at this time since I feel like the right place to do this sort of logic may change in subsequent PRs.
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 This is part of a change which allows me to avoid some ugly hackery in my draft PR (#3221). Break Project creation/deletion out into a pair of dedicated structs/interfaces. Change the `ProjectFactory` function used in the GitHubProviderService from a method on the controlplane to a function which returns a function. This is needed for some additional cleanup introduced in a future PR.
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 This is part of a change which allows me to avoid some ugly hackery in my draft PR (#3221). Break Project creation/deletion out into a pair of dedicated structs/interfaces. Change the `ProjectFactory` function used in the GitHubProviderService from a method on the controlplane to a function which returns a function. This is needed for some additional cleanup introduced in a future PR.
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 With some of the changes I've made, I have started to run into issues with the way we currently wire up the server, event handler and reconcilers: 1. The way in which structs are shared across events, server and reconcilers is a bit hacky, and will get more hacky when I start sharing more structs. 2. The functional options pattern is starting to cause complexity now that we need to wire up dependencies which depend on some of the structs passed by the options. This moves most of the application wireup outside of `server.go` and into `service.go`. The optional parameters for the server are removed entirely, and are now passed in as explicit arguments. Various tests have been cleaned up to instantiate the Server directly with appropriate stubs for different dependencies. The use of optional parameters for the reconciler and event handler will be cleaned up in a future PR.
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 With some of the changes I've made, I have started to run into issues with the way we currently wire up the server, event handler and reconcilers: 1. The way in which structs are shared across events, server and reconcilers is a bit hacky, and will get more hacky when I start sharing more structs. 2. The functional options pattern is starting to cause complexity now that we need to wire up dependencies which depend on some of the structs passed by the options. This moves most of the application wireup outside of `server.go` and into `service.go`. The optional parameters for the server are removed entirely, and are now passed in as explicit arguments. Various tests have been cleaned up to instantiate the Server directly with appropriate stubs for different dependencies. The use of optional parameters for the reconciler and event handler will be cleaned up in a future PR.
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 With some of the changes I've made, I have started to run into issues with the way we currently wire up the server, event handler and reconcilers: 1. The way in which structs are shared across events, server and reconcilers is a bit hacky, and will get more hacky when I start sharing more structs. 2. The functional options pattern is starting to cause complexity now that we need to wire up dependencies which depend on some of the structs passed by the options. This moves most of the application wireup outside of `server.go` and into `service.go`. The optional parameters for the server are removed entirely, and are now passed in as explicit arguments. Various tests have been cleaned up to instantiate the Server directly with appropriate stubs for different dependencies. The use of optional parameters for the reconciler and event handler will be cleaned up in a future PR.
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 When creating a provider, the code previously inserted the provider into the database and then instantiated it in order to validate that the user ID is correct. Alter the code so that it just instantiates the client and some supporting structures to validate the user ID. This client deliberately does not use the client cache in order to simplify the code. Since this is the first usage of the client for that provider, we will never find a suitable client in the cache anyway.
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 When creating a provider, the code previously inserted the provider into the database and then instantiated it in order to validate that the user ID is correct. Alter the code so that it just instantiates the client and some supporting structures to validate the user ID. This client deliberately does not use the client cache in order to simplify the code. Since this is the first usage of the client for that provider, we will never find a suitable client in the cache anyway.
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 Part of the ongoing decoupling of Provider-specific details from various parts of Minder.
dmjb
added a commit
that referenced
this issue
May 2, 2024
Relates to #2845 When creating a provider, the code previously inserted the provider into the database and then instantiated it in order to validate that the user ID is correct. Alter the code so that it just instantiates the client and some supporting structures to validate the user ID. This client deliberately does not use the client cache in order to simplify the code. Since this is the first usage of the client for that provider, we will never find a suitable client in the cache anyway.
dmjb
added a commit
that referenced
this issue
May 7, 2024
Relates to #2845 When evaluating a policy, we instantiate a ProviderBuilder and then pass it around to various parts of the code. Once the specific trait is known, we get the specific trait we need from the ProviderBuilder. While replacing the ProviderBuilder, I notice that there are many parts of the code where we continue to pass around the ProviderBuilder even though we know the specific provider trait which will be needed. I changed various code to explicitly require the provider trait it needs instead of taking the ProviderBuilder and getting an instance of the desired type. As a result of this change, test setup is simplified, and this will make my PR to replace the ProviderBuilder smaller.
10 tasks
dmjb
added a commit
that referenced
this issue
May 7, 2024
Relates to #2845 When creating a provider, the code previously inserted the provider into the database and then instantiated it in order to validate that the user ID is correct. Alter the code so that it just instantiates the client and some supporting structures to validate the user ID. This client deliberately does not use the client cache in order to simplify the code. Since this is the first usage of the client for that provider, we will never find a suitable client in the cache anyway.
dmjb
added a commit
that referenced
this issue
May 7, 2024
Relates to #2845 When evaluating a policy, we instantiate a ProviderBuilder and then pass it around to various parts of the code. Once the specific trait is known, we get the specific trait we need from the ProviderBuilder. While replacing the ProviderBuilder, I notice that there are many parts of the code where we continue to pass around the ProviderBuilder even though we know the specific provider trait which will be needed. I changed various code to explicitly require the provider trait it needs instead of taking the ProviderBuilder and getting an instance of the desired type. As a result of this change, test setup is simplified, and this will make my PR to replace the ProviderBuilder smaller.
dmjb
added a commit
that referenced
this issue
May 7, 2024
…3262) Relates to #2845 When evaluating a policy, we instantiate a ProviderBuilder and then pass it around to various parts of the code. Once the specific trait is known, we get the specific trait we need from the ProviderBuilder. While replacing the ProviderBuilder, I notice that there are many parts of the code where we continue to pass around the ProviderBuilder even though we know the specific provider trait which will be needed. I changed various code to explicitly require the provider trait it needs instead of taking the ProviderBuilder and getting an instance of the desired type. As a result of this change, test setup is simplified, and this will make my PR to replace the ProviderBuilder smaller.
dmjb
added a commit
that referenced
this issue
May 7, 2024
Relates to #2845 Refactor engine to use ProviderManager. Various bits of refactoring along the way, including renaming some struct fields and variables for clarity.
dmjb
added a commit
that referenced
this issue
May 8, 2024
Relates to #2845 Refactor engine to use ProviderManager. Various bits of refactoring along the way, including renaming some struct fields and variables for clarity.
dmjb
added a commit
that referenced
this issue
May 8, 2024
Fixes #2845 Remove final usages of provider builder in CLI and delete code.
dmjb
added a commit
that referenced
this issue
May 8, 2024
Fixes #2845 Remove final usages of provider builder in CLI and delete code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Please describe the enhancement
Right now, any part of the code which needs to interact with Providers ends up repeating the same set of logic. This makes it difficult to stub out provider operations for testing, and creates coupling between the logic for creating providers, and the business logic which uses providers.
Define interfaces for creating/getting providers to reduce coupling and ease testing.
Solution Proposal
At minimum, make ProviderBuilder an interface, and create an interface for pulling
db.Providerout of the database. We may want some higher level abstractions to combine retrieving adb.Provider, and instantiating a particular Provider interface.Describe alternatives you've considered
No response
Additional context
No response
Acceptance Criteria
No response
The text was updated successfully, but these errors were encountered: