Replace use of ProviderBuilder in controlplane #2841

dmjb · 2024-03-28T09:54:26Z

Relates to #2845

Create a new TraitInstantiator interface. This is essentially a dedicated factory interface with a similar interface to ProviderBuilder. This change allows us to stub out the process of creating instances of the Provider interfaces/traits from a db.Provider. Furthermore, because many of the dependencies which previously had to be passed to ProviderBuilder are now passed to TraitInstantiator during application wireup, this means that fewer dependencies have to be passed around in the code, and less boilerplate is required to create an instance of a Provider interface.

I changed the name since A) we are moving towards a model where Providers have "traits", and thus the responsibility is to instantiate instances of those traits and B) it always seemed strange to me that ProviderBuilder needed a Provider as an input - it creates the impression that there are two (or more) things called "Provider" in the codebase.

Note that I have not replaced all uses of ProviderBuilder yet. I'd like to get this change merged before converting the engine code to use this new interface.

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:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

dmjb · 2024-03-28T10:02:41Z

internal/providers/factory.go

@@ -0,0 +1,243 @@
+// Copyright 2024 Stacklok, Inc.


In a future PR, I'll write unit tests for these methods. Right now - their behaviour is validated through pre-existing tests which exercise these methods.

JAORMX · 2024-03-28T10:02:31Z

internal/controlplane/handlers_repositories.go

@@ -322,21 +322,24 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
 	}

 	for _, provider := range provs {
+		// keep linter happy
+		// TODO: this should not be necessary post Go 1.22 - update linter
+		p := provider


Don't we already use and enforce go 1.22?

See the comment I left in this code in my most recent commit - the gosec authors have decided not to get rid of this warning until all versions prior to 1.22 are unsupported.

JAORMX · 2024-03-28T10:03:37Z

internal/providers/factory.go

+// GitHubOptions contains options used when creating a GitHub instance
+// If omitted, these will be looked up in the database
+type GitHubOptions struct {
+	Credential  provinfv1.GitHubCredential


We'll use credentials in all providers, why is this specific to GitHub? Did you check the OCI Providers POC?

In most cases, the credentials are loaded from the database. However, there are a few places where the Github trait is instantiated with credentials which are not in the DB - one is in internal/providers/service.go (which is in this PR), and there are two in the CLI (not in this PR). In those cases, it is necessary to provide some Github specific details, namely the owner filter and the credentials used.

Your comment did make me realize that there was a bug in my change where I assumed Github credentials in more places than I should have. That has been fixed.

dmjb · 2024-03-28T10:04:26Z

internal/controlplane/handlers_githubwebhooks_test.go

@@ -239,27 +236,6 @@ func (s *UnitTestSuite) TestHandleWebHookRepository() {
 	repositoryID := uuid.New()
 	projectID := uuid.New()

-	// create a test oauth2 token


Previously, these operations were called to create a ProviderBuilder. In the TraitInstantiator, they are only called when the provider traits get instantiated. Since these test cases do not need a GitHub instance to be instantiated, they are unused.

coveralls · 2024-03-28T10:11:58Z

coverage: 47.203% (-0.2%) from 47.361%
when pulling d613445 on trait-instantiator
into a9d3540 on main.

go.mod

JAORMX · 2024-03-28T11:04:27Z

This LGTM and I dig the direction. I'll let @eleftherias and @evankanderson chime in

evankanderson · 2024-03-28T12:35:09Z

internal/db/domain.go

+// CanImplement returns true if the provider implements the given type.
+func (p *Provider) CanImplement(impl ProviderType) bool {
+	return slices.Contains(p.Implements, impl)
+}


I was just thinking about this last night. I think this is a good first start, but I'm wondering whether it would make sense to rename db.Provider to db.ProviderData to emphasize that it's the data-at-rest representation rather than the active object representing a provider. Either that, or we just accept that a db.Provider and a provider.Provider are unrelated to each other, and maybe use the same name more places.

sqlc renaming feature: https://docs.sqlc.dev/en/stable/howto/rename.html#tables

I had a similar thought while working on this PR. I was thinking ProviderConfig or similar.

I can investigate implementing this using the sqlc renaming functionality, but I feel like that deserves its own PR.

Agreed; I'd hold off on the rename until after #2841, #2842, and the follow-on that mostly hides db.Provider from controlplane before doing the rename.

evankanderson · 2024-03-28T12:35:42Z

internal/providers/instantiator.go

+// interfaces defined in `github.com/stacklok/minder/pkg/providers/v1` from
+// the providers stored in the database.
+type TraitInstantiator interface {
+	GetGit(ctx context.Context, provider *db.Provider) (provinfv1.Git, error)


Should this be part of the Provider interface?

evankanderson · 2024-03-28T12:36:13Z

internal/providers/instantiator.go

+// interfaces defined in `github.com/stacklok/minder/pkg/providers/v1` from
+// the providers stored in the database.
+type TraitInstantiator interface {
+	GetGit(ctx context.Context, provider *db.Provider) (provinfv1.Git, error)


What do you think of AsGit rather than GetGit, to make it more clear that this is effectively a down-casting operation?

Good call, will make that change.

evankanderson · 2024-03-28T12:40:11Z

internal/providers/instantiator.go

+// GitHubOptions contains options used when creating a GitHub instance
+// If omitted, these will be looked up in the database
+type GitHubOptions struct {
+	Credential  provinfv1.GitHubCredential
+	OwnerFilter *string
+}


I'm not a fan of having these on GetGitHub, rather than having them be part of the TraitInstantiator object state. Do you have a use case beyond verifyProviderTokenIdentity?

Yeah, the more I think about the approach I took, the less I like it. I might try another approach to avoid this hack.

Looking more at the code, I wonder if verifyProviderTokenIdentity should really be verifyGitHubTokenIdentity, and it should simply instantiate a github.Client itself.

In both cases, that call is being made before the provider is created, so it's a bit of an odd case that might be be addressed by not trying to re-use the rest of the framework.

evankanderson · 2024-03-28T12:54:32Z

internal/controlplane/handlers_githubwebhooks.go

-	provBuilder, err := providers.GetProviderBuilder(ctx, prov, s.store, s.cryptoEngine, &s.cfg.Provider, pbOpts...)
-	if err != nil {
-		return fmt.Errorf("error building client: %w", err)
-	}


I like removing this factory from all the calls, but I feel like I want two layers of objects:

A ProviderFactory which takes all the current arguments except ctx and prov, and has Build(ctx, currentProject, providerName) (Provider, error). This combines lines 313-328 into a function, and doesn't directly expose db.Provider elsewhere. As we discover other cases where we need to find a provider, the ProviderFactory would get new methods, such as BuildForEntity(ctx, currentProject, entityType)

fooProvider objects which wrap the db.Provider type with the selected implementation behavior, e.g. ghAppProvider and ghOAuthProvider. Both of these would implement v1.Provider, which would have As$Interface (AsGit, AsGitHub, AsREST, etc) methods for down-casting as well as Implements if we need the checks.

I'm a little concerned with passing db row objects everywhere in that we end up making it hard to separate the schema from the business logic; ideally the db queries and token / crypto pieces would live in ghAppProvider and ghOAuthProvider and we have a central place to implement the mapping from user-provider context -> provider, rather than going directly to the database layer.

I completely agree with you, but I also feel like it's a good idea to incrementally move towards that sort of model. Right now, too much knowledge of the creation of profiles exist in too many parts of the codebase. I want to reduce that coupling/lack of cohesion before introducing a new abstraction.

I'm fine with incremental progress, as long as we agree on the destination. I've just had bad experiences with incremental progress hiding disagreement on the final destination.

Create a new `TraitInstantiator` interface. This is essentially a dedicated factory interface which has a similar interface to ProviderBuilder. This change allows us to stub out the process of creating instances of the Provider interfaces/traits from a `db.Provider`. I changed the name since A) we are moving towards a model where Providers have "traits", and thus the responsibility is to instantiate instances of those traits and B) it always seemed strange to me that `ProviderBuilder` needed a `Provider` as an input.

dmjb · 2024-04-17T09:30:57Z

Decided to take a different approach

dmjb requested a review from a team as a code owner March 28, 2024 09:54

dmjb force-pushed the trait-instantiator branch from 4183f6b to 92c83a5 Compare March 28, 2024 10:00

dmjb commented Mar 28, 2024

View reviewed changes

JAORMX reviewed Mar 28, 2024

View reviewed changes

dmjb commented Mar 28, 2024

View reviewed changes

dmjb force-pushed the trait-instantiator branch from 92c83a5 to 8ab0533 Compare March 28, 2024 10:05

dmjb commented Mar 28, 2024

View reviewed changes

go.mod Show resolved Hide resolved

dmjb force-pushed the trait-instantiator branch from d2cc14e to 9fd1b3e Compare March 28, 2024 11:13

evankanderson reviewed Mar 28, 2024

View reviewed changes

dmjb force-pushed the trait-instantiator branch 3 times, most recently from 30c1e65 to 58dc44a Compare April 2, 2024 18:45

dmjb added 6 commits April 3, 2024 11:31

ensure that we use correct type of credentials

e8c9823

update linter, exclude irrelevant gosec issue

48ea6f1

rollback tooling upgrades for now

496f95e

rename file to reflect contents

343e4f2

rename methods

d613445

dmjb force-pushed the trait-instantiator branch from 58dc44a to d613445 Compare April 3, 2024 10:32

dmjb marked this pull request as draft April 4, 2024 12:14

dmjb closed this Apr 17, 2024

dmjb deleted the trait-instantiator branch April 17, 2024 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace use of ProviderBuilder in controlplane #2841

Replace use of ProviderBuilder in controlplane #2841

dmjb commented Mar 28, 2024 •

edited

Loading

dmjb Mar 28, 2024

JAORMX Mar 28, 2024

dmjb Mar 28, 2024

JAORMX Mar 28, 2024

dmjb Mar 28, 2024 •

edited

Loading

dmjb Mar 28, 2024 •

edited

Loading

coveralls commented Mar 28, 2024 •

edited

Loading

JAORMX commented Mar 28, 2024

evankanderson Mar 28, 2024

dmjb Mar 28, 2024

evankanderson Mar 28, 2024

evankanderson Mar 28, 2024

evankanderson Mar 28, 2024

dmjb Mar 28, 2024

evankanderson Mar 28, 2024

dmjb Mar 28, 2024

evankanderson Mar 28, 2024

evankanderson Mar 28, 2024

dmjb Mar 28, 2024

evankanderson Mar 28, 2024

dmjb commented Apr 17, 2024

Replace use of ProviderBuilder in controlplane #2841

Replace use of ProviderBuilder in controlplane #2841

Conversation

dmjb commented Mar 28, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmjb Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

dmjb Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

coveralls commented Mar 28, 2024 • edited Loading

JAORMX commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmjb commented Apr 17, 2024

dmjb commented Mar 28, 2024 •

edited

Loading

dmjb Mar 28, 2024 •

edited

Loading

dmjb Mar 28, 2024 •

edited

Loading

coveralls commented Mar 28, 2024 •

edited

Loading