Skip to content

Commit

Permalink
Remove use of ProviderBuilder by repo handler (#3224)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@dmjb
dmjb authored May 2, 2024
1 parent b9daa61 commit 3e08779
Show file tree
Hide file tree
Showing 14 changed files with 263 additions and 141 deletions.
70 changes: 14 additions & 56 deletions internal/controlplane/handlers_repositories.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,7 @@ func (s *Server) RegisterRepository(
return nil, status.Errorf(codes.Internal, "cannot get provider: %v", err)
}

client, err := s.getClientForProvider(ctx, *provider)
if err != nil {
return nil, err
}

newRepo, err := s.repos.CreateRepository(ctx, client, provider, projectID, githubRepo.GetOwner(), githubRepo.GetName())
newRepo, err := s.repos.CreateRepository(ctx, provider, projectID, githubRepo.GetOwner(), githubRepo.GetName())
if err != nil {
if errors.Is(err, ghrepo.ErrPrivateRepoForbidden) || errors.Is(err, ghrepo.ErrArchivedRepoForbidden) {
return nil, util.UserVisibleError(codes.InvalidArgument, err.Error())
Expand Down Expand Up @@ -324,7 +319,7 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
logger.BusinessRecord(ctx).Project = projectID

providerName := in.GetContext().GetProvider()
provs, err := s.providerStore.GetByNameAndTrait(ctx, projectID, providerName, db.ProviderTypeRepoLister)
provs, errorProvs, err := s.providerManager.BulkInstantiateByTrait(ctx, projectID, db.ProviderTypeRepoLister, providerName)
if err != nil {
pErr := providers.ErrProviderNotFoundBy{}
if errors.As(err, &pErr) {
Expand All @@ -337,32 +332,23 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
Results: []*pb.UpstreamRepositoryRef{},
}

var erroringProviders []string

for _, provider := range provs {
for providerName, provider := range provs {
zerolog.Ctx(ctx).Trace().
Str("provider", provider.Name).
Str("provider", providerName).
Str("project_id", projectID.String()).
Msg("listing repositories")

pbOpts := []providers.ProviderBuilderOption{
providers.WithProviderMetrics(s.provMt),
providers.WithRestClientCache(s.restClientCache),
}
p, err := providers.GetProviderBuilder(ctx, provider, s.store, s.cryptoEngine, &s.cfg.Provider,
s.fallbackTokenClient, pbOpts...)
repoLister, err := v1.As[v1.RepoLister](provider)
if err != nil {
zerolog.Ctx(ctx).Error().Err(err).Msg("cannot get provider builder")
erroringProviders = append(erroringProviders, provider.Name)

// skip over this provider
zerolog.Ctx(ctx).Error().Err(err).Msg("error instantiating repo lister")
errorProvs = append(errorProvs, providerName)
continue
}

results, err := s.listRemoteRepositoriesForProvider(ctx, provider.Name, p, projectID)
results, err := s.listRemoteRepositoriesForProvider(ctx, providerName, repoLister, projectID)
if err != nil {
zerolog.Ctx(ctx).Error().Err(err).Msg("cannot list repositories for provider")
erroringProviders = append(erroringProviders, provider.Name)
errorProvs = append(errorProvs, providerName)

// skip over this provider
continue
Expand All @@ -372,8 +358,8 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
}

// If all providers failed, return an error
if len(erroringProviders) > 0 && len(out.Results) == 0 {
return nil, util.UserVisibleError(codes.Internal, "cannot list repositories for providers: %v", erroringProviders)
if len(errorProvs) > 0 && len(out.Results) == 0 {
return nil, util.UserVisibleError(codes.Internal, "cannot list repositories for providers: %v", errorProvs)
}

return out, nil
Expand All @@ -382,19 +368,13 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
func (s *Server) listRemoteRepositoriesForProvider(
ctx context.Context,
provName string,
p *providers.ProviderBuilder,
repoLister v1.RepoLister,
projectID uuid.UUID,
) ([]*pb.UpstreamRepositoryRef, error) {
// by now we've already checked that repo listed is implemented
client, err := p.GetRepoLister()
if err != nil {
return nil, fmt.Errorf("cannot create github client: %v", err)
}

tmoutCtx, cancel := context.WithTimeout(ctx, github.ExpensiveRestCallTimeout)
defer cancel()

remoteRepos, err := client.ListAllRepositories(tmoutCtx)
remoteRepos, err := repoLister.ListAllRepositories(tmoutCtx)
if err != nil {
return nil, fmt.Errorf("cannot list repositories: %v", err)
}
Expand Down Expand Up @@ -436,7 +416,7 @@ func (s *Server) inferProviderByOwner(ctx context.Context, owner string, project
if providerName != "" {
return s.providerStore.GetByName(ctx, projectID, providerName)
}
opts, err := s.providerStore.GetByNameAndTrait(ctx, projectID, providerName, db.ProviderTypeGithub)
opts, err := s.providerStore.GetByTraitInHierarchy(ctx, projectID, providerName, db.ProviderTypeGithub)
if err != nil {
return nil, fmt.Errorf("error getting providers: %v", err)
}
Expand All @@ -461,28 +441,6 @@ func (s *Server) inferProviderByOwner(ctx context.Context, owner string, project
return nil, fmt.Errorf("no providers can handle repo owned by %s", owner)
}

func (s *Server) getClientForProvider(
ctx context.Context,
provider db.Provider,
) (v1.GitHub, error) {
pbOpts := []providers.ProviderBuilderOption{
providers.WithProviderMetrics(s.provMt),
providers.WithRestClientCache(s.restClientCache),
}

p, err := providers.GetProviderBuilder(ctx, provider, s.store, s.cryptoEngine, &s.cfg.Provider, s.fallbackTokenClient, pbOpts...)
if err != nil {
return nil, status.Errorf(codes.Internal, "cannot get provider builder: %v", err)
}

client, err := p.GetGitHub()
if err != nil {
return nil, status.Errorf(codes.Internal, "error creating github provider: %v", err)
}

return client, nil
}

func getProjectID(ctx context.Context) uuid.UUID {
entityCtx := engine.EntityFromContext(ctx)
return entityCtx.Project.ID
Expand Down
53 changes: 23 additions & 30 deletions internal/controlplane/handlers_repositories_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,16 @@ import (
"github.com/google/uuid"
"github.com/stretchr/testify/require"
"go.uber.org/mock/gomock"
"golang.org/x/oauth2"

mockdb "github.com/stacklok/minder/database/mock"
"github.com/stacklok/minder/internal/config/server"
mockcrypto "github.com/stacklok/minder/internal/crypto/mock"
"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/engine"
"github.com/stacklok/minder/internal/providers"
mockgh "github.com/stacklok/minder/internal/providers/github/mock"
ghprovider "github.com/stacklok/minder/internal/providers/github/oauth"
"github.com/stacklok/minder/internal/providers/ratecache"
"github.com/stacklok/minder/internal/providers/manager"
mockmanager "github.com/stacklok/minder/internal/providers/manager/mock"
ghrepo "github.com/stacklok/minder/internal/repositories/github"
mockghrepo "github.com/stacklok/minder/internal/repositories/github/mock"
"github.com/stacklok/minder/internal/util/ptr"
Expand Down Expand Up @@ -113,7 +112,7 @@ func TestServer_RegisterRepository(t *testing.T) {
Project: engine.Project{ID: projectID},
})

server := createServer(ctrl, scenario.RepoServiceSetup, newGitHub(), scenario.ProviderFails)
server := createServer(ctrl, scenario.RepoServiceSetup, scenario.ProviderFails, nil)

req := &pb.RegisterRepositoryRequest{
Repository: &pb.UpstreamRepositoryRef{
Expand Down Expand Up @@ -172,7 +171,16 @@ func TestServer_ListRemoteRepositoriesFromProvider(t *testing.T) {
Project: engine.Project{ID: projectID},
})

server := createServer(ctrl, scenario.RepoServiceSetup, scenario.GitHubSetup, scenario.ProviderFails)
prov := scenario.GitHubSetup(ctrl)
manager := mockmanager.NewMockProviderManager(ctrl)
manager.EXPECT().BulkInstantiateByTrait(
gomock.Any(),
gomock.Eq(projectID),
gomock.Eq(db.ProviderTypeRepoLister),
gomock.Eq(""),
).Return(map[string]provinfv1.Provider{provider.Name: prov}, []string{}, nil)

server := createServer(ctrl, scenario.RepoServiceSetup, scenario.ProviderFails, manager)

projectIDStr := projectID.String()
req := &pb.ListRemoteRepositoriesFromProviderRequest{
Expand Down Expand Up @@ -273,7 +281,7 @@ func TestServer_DeleteRepository(t *testing.T) {
Project: engine.Project{ID: projectID},
})

server := createServer(ctrl, scenario.RepoServiceSetup, newGitHub(), scenario.ProviderFails)
server := createServer(ctrl, scenario.RepoServiceSetup, scenario.ProviderFails, nil)

var result string
var resultError error
Expand Down Expand Up @@ -324,7 +332,6 @@ const (
repoOwnerAndName = "acme-corp/api-gateway"
repoID = "3eb6d254-4163-460f-89f7-44e2ae916e71"
remoteRepoId = 123456
accessToken = "TOKEN"
)

var (
Expand Down Expand Up @@ -369,14 +376,14 @@ func newGitHub(opts ...func(mock githubMock)) githubMockBuilder {

func withSuccessfulCreate(mock repoServiceMock) {
mock.EXPECT().
CreateRepository(gomock.Any(), gomock.Any(), gomock.Any(), projectID, repoOwner, repoName).
CreateRepository(gomock.Any(), gomock.Any(), projectID, repoOwner, repoName).
Return(creationResult, nil)
}

func withFailedCreate(err error) func(repoServiceMock) {
return func(mock repoServiceMock) {
mock.EXPECT().
CreateRepository(gomock.Any(), gomock.Any(), gomock.Any(), projectID, repoOwner, repoName).
CreateRepository(gomock.Any(), gomock.Any(), projectID, repoOwner, repoName).
Return(nil, err)
}
}
Expand Down Expand Up @@ -419,30 +426,17 @@ func withFailedListAllRepositories(err error) func(githubMock) {
}
}

func createServer(ctrl *gomock.Controller, repoServiceSetup repoMockBuilder, githubSetup githubMockBuilder, providerFails bool) *Server {
func createServer(
ctrl *gomock.Controller,
repoServiceSetup repoMockBuilder,
providerFails bool,
providerManager manager.ProviderManager,
) *Server {
var svc ghrepo.RepositoryService
if repoServiceSetup != nil {
svc = repoServiceSetup(ctrl)
}

// stubs needed for providers to work
// TODO: this provider logic should be better encapsulated from the controlplane
mockCryptoEngine := mockcrypto.NewMockEngine(ctrl)
mockCryptoEngine.EXPECT().
DecryptOAuthToken(gomock.Any()).
Return(oauth2.Token{AccessToken: accessToken}, nil).
AnyTimes()
cancelable, cancel := context.WithCancel(context.Background())
clientCache := ratecache.NewRestClientCache(cancelable)
defer cancel()

var gh *mockgh.MockGitHub
if githubSetup != nil {
gh = githubSetup(ctrl)
}

clientCache.Set("", accessToken, db.ProviderTypeGithub, gh)

store := mockdb.NewMockStore(ctrl)
store.EXPECT().
GetParentProjects(gomock.Any(), projectID).
Expand Down Expand Up @@ -477,9 +471,8 @@ func createServer(ctrl *gomock.Controller, repoServiceSetup repoMockBuilder, git
return &Server{
store: store,
repos: svc,
cryptoEngine: mockCryptoEngine,
restClientCache: clientCache,
cfg: &server.Config{},
providerStore: providers.NewProviderStore(store),
providerManager: providerManager,
}
}
39 changes: 39 additions & 0 deletions internal/providers/manager/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"

"github.com/google/uuid"
"github.com/rs/zerolog"

"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/providers"
Expand All @@ -36,6 +37,18 @@ type ProviderManager interface {
// InstantiateFromNameProject creates the provider using the provider's name and
// project hierarchy.
InstantiateFromNameProject(ctx context.Context, name string, projectID uuid.UUID) (v1.Provider, error)
// BulkInstantiateByTrait instantiates multiple providers in the
// project hierarchy. Providers are filtered by trait, and optionally by
// name (empty name string means no filter by name).
// To preserve compatibility with behaviour expected by the API, if a
// provider cannot be instantiated, it will not cause the method to error
// out, instead a list of failed provider names will be returned.
BulkInstantiateByTrait(
ctx context.Context,
projectID uuid.UUID,
trait db.ProviderType,
name string,
) (map[string]v1.Provider, []string, error)
// DeleteByID deletes the specified instance of the Provider, and
// carries out any cleanup needed.
DeleteByID(ctx context.Context, providerID uuid.UUID, projectID uuid.UUID) error
Expand Down Expand Up @@ -112,6 +125,32 @@ func (p *providerManager) InstantiateFromNameProject(ctx context.Context, name s
return p.buildFromDBRecord(ctx, config)
}

func (p *providerManager) BulkInstantiateByTrait(
ctx context.Context,
projectID uuid.UUID,
trait db.ProviderType,
name string,
) (map[string]v1.Provider, []string, error) {
providerConfigs, err := p.store.GetByTraitInHierarchy(ctx, projectID, name, trait)
if err != nil {
return nil, nil, fmt.Errorf("error retrieving db records: %w", err)
}

result := make(map[string]v1.Provider, len(providerConfigs))
failedProviders := []string{}
for _, config := range providerConfigs {
provider, err := p.buildFromDBRecord(ctx, &config)
if err != nil {
zerolog.Ctx(ctx).Error().Err(err).Msgf("error while instantiating provider %s", config.ID)
failedProviders = append(failedProviders, config.Name)
continue
}
result[config.Name] = provider
}

return result, failedProviders, nil
}

func (p *providerManager) DeleteByID(ctx context.Context, providerID uuid.UUID, projectID uuid.UUID) error {
config, err := p.store.GetByIDProject(ctx, providerID, projectID)
if err != nil {
Expand Down
Loading

0 comments on commit 3e08779

Please sign in to comment.