Skip to content

Commit

Permalink
Reapply "Move repository create to RepositoryService (#2632)" (#2645) (…
Browse files Browse the repository at this point in the history 
…#2648)

This reverts commit a0d83dc.

As part of reapplying this commit, two bug fixes have been introduced:

1) Ensure that provider ID is set when creating the repository in the
   database.
2) Change the API response structure: do not return partially-filled
   responses when the request has failed.
  • Loading branch information
@dmjb
dmjb authored Mar 19, 2024
1 parent a7d22bf commit 7b9a05a
Show file tree
Hide file tree
Showing 8 changed files with 535 additions and 420 deletions.
86 changes: 0 additions & 86 deletions internal/controlplane/handlers_githubwebhooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,92 +213,6 @@ func handleParseError(typ string, parseErr error) *metrics.WebhookEventState {
return state
}

// registerWebhookForRepository registers a set repository and sets up the webhook for each of them
// and returns the registration result for each repository.
// If an error occurs, the registration is aborted and the error is returned.
// https://docs.github.com/en/rest/reference/repos#create-a-repository-webhook

// The actual logic for webhook creation lives in the WebhookManager interface
// TODO: the remaining logic should be refactored into a repository
// registration interface
func (s *Server) registerWebhookForRepository(
ctx context.Context,
pbuild *providers.ProviderBuilder,
projectID uuid.UUID,
repo *pb.UpstreamRepositoryRef,
) (*pb.RegisterRepoResult, error) {
logger := zerolog.Ctx(ctx).With().
Str("repoName", repo.Name).
Str("repoOwner", repo.Owner).
Logger()
ctx = logger.WithContext(ctx)

if !pbuild.Implements(db.ProviderTypeGithub) {
return nil, fmt.Errorf("provider %s is not supported for github webhook", pbuild.GetName())
}

client, err := pbuild.GetGitHub()
if err != nil {
return nil, fmt.Errorf("error creating github provider: %w", err)
}

regResult := &pb.RegisterRepoResult{
// We will overwrite this later when we've looked it up from the provider,
// but existing clients expect a message here, so let's add one.
Repository: &pb.Repository{
Name: repo.Name, // Not normalized, from client
Owner: repo.Owner, // Not normalized, from client
},
Status: &pb.RegisterRepoResult_Status{
Success: false,
},
}

// let's verify that the repository actually exists.
repoGet, err := client.GetRepository(ctx, repo.Owner, repo.Name)
if err != nil {
errorStr := err.Error()
regResult.Status.Error = &errorStr
return regResult, nil
}

// skip if we try to register a private repository
if repoGet.GetPrivate() && !features.ProjectAllowsPrivateRepos(ctx, s.store, projectID) {
errorStr := "repository is private"
regResult.Status.Error = &errorStr
return regResult, nil
}

hookUUID, githubHook, err := s.webhookManager.CreateWebhook(ctx, client, repo.Owner, repo.Name)
if err != nil {
logger.Error().Msgf("error while creating webhook: %v", err)
errorStr := err.Error()
regResult.Status.Error = &errorStr
return regResult, nil
}

regResult.Status.Success = true

regResult.Repository = &pb.Repository{
Name: repoGet.GetName(),
Owner: repoGet.GetOwner().GetLogin(),
RepoId: repoGet.GetID(),
HookId: githubHook.GetID(),
HookUrl: githubHook.GetURL(),
DeployUrl: repoGet.GetDeploymentsURL(),
CloneUrl: repoGet.GetCloneURL(),
HookType: githubHook.GetType(),
HookName: githubHook.GetName(),
HookUuid: hookUUID,
IsPrivate: repoGet.GetPrivate(),
IsFork: repoGet.GetFork(),
DefaultBranch: repoGet.GetDefaultBranch(),
License: repoGet.GetLicense().GetSPDXID(),
}

return regResult, nil
}

func (s *Server) parseGithubEventForProcessing(
rawWHPayload []byte,
msg *message.Message,
Expand Down
156 changes: 48 additions & 108 deletions internal/controlplane/handlers_repositories.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (

"github.com/google/uuid"
"github.com/rs/zerolog"
"github.com/rs/zerolog/log"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

Expand All @@ -31,8 +30,8 @@ import (
"github.com/stacklok/minder/internal/logger"
"github.com/stacklok/minder/internal/projects/features"
"github.com/stacklok/minder/internal/providers"
github "github.com/stacklok/minder/internal/providers/github"
"github.com/stacklok/minder/internal/reconcilers"
"github.com/stacklok/minder/internal/providers/github"
ghrepo "github.com/stacklok/minder/internal/repositories/github"
"github.com/stacklok/minder/internal/util"
cursorutil "github.com/stacklok/minder/internal/util/cursor"
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
Expand All @@ -46,107 +45,48 @@ const maxFetchLimit = 100
// Once a user had enrolled in a project (they have a valid token), they can register
// repositories to be monitored by the minder by provisioning a webhook on the
// repository(ies).
func (s *Server) RegisterRepository(ctx context.Context,
in *pb.RegisterRepositoryRequest) (*pb.RegisterRepositoryResponse, error) {
entityCtx := engine.EntityFromContext(ctx)
projectID := entityCtx.Project.ID

provider, err := getProviderFromRequestOrDefault(ctx, s.store, in, projectID)
if err != nil {
return nil, providerError(err)
}

pbOpts := []providers.ProviderBuilderOption{
providers.WithProviderMetrics(s.provMt),
providers.WithRestClientCache(s.restClientCache),
}
p, err := providers.GetProviderBuilder(ctx, provider, s.store, s.cryptoEngine, pbOpts...)
func (s *Server) RegisterRepository(
ctx context.Context,
in *pb.RegisterRepositoryRequest,
) (*pb.RegisterRepositoryResponse, error) {
projectID := getProjectID(ctx)
provider, client, err := s.getProviderAndClient(ctx, projectID, in)
if err != nil {
return nil, status.Errorf(codes.Internal, "cannot get provider builder: %v", err)
return nil, err
}

// Unmarshal the in.GetRepositories() into a struct Repository
if in.GetRepository() == nil || in.GetRepository().Name == "" {
return nil, util.UserVisibleError(codes.InvalidArgument, "no repository provided")
// Validate that the Repository struct in the request
githubRepo := in.GetRepository()
// If the repo owner is missing, GitHub will assume a default value based
// on the user's credentials. An explicit check for owner is left out to
// avoid breaking backwards compatibility.
if githubRepo.GetName() == "" {
return nil, util.UserVisibleError(codes.InvalidArgument, "missing repository name")
}

repo := in.GetRepository()
l := zerolog.Ctx(ctx).With().
Str("repoName", githubRepo.GetName()).
Str("repoOwner", githubRepo.GetOwner()).
Str("projectID", projectID.String()).
Logger()
ctx = l.WithContext(ctx)

result, err := s.registerWebhookForRepository(ctx, p, projectID, repo)
newRepo, err := s.repos.CreateRepository(ctx, client, provider, projectID, githubRepo.GetOwner(), githubRepo.GetName())
if err != nil {
return nil, util.UserVisibleError(codes.Internal, "cannot register webhook: %v", err)
}

r := result.Repository

response := &pb.RegisterRepositoryResponse{
Result: result,
}

// Convert each result to a pb.Repository object
if result.Status.Error != nil {
return response, nil
if errors.Is(err, ghrepo.ErrPrivateRepoForbidden) {
return nil, util.UserVisibleError(codes.InvalidArgument, err.Error())
}
return nil, util.UserVisibleError(codes.Internal, "unable to register repository: %v", err)
}

// update the database
dbRepo, err := s.store.CreateRepository(ctx, db.CreateRepositoryParams{
Provider: provider.Name,
ProviderID: provider.ID,
ProjectID: projectID,
RepoOwner: r.Owner,
RepoName: r.Name,
RepoID: r.RepoId,
IsPrivate: r.IsPrivate,
IsFork: r.IsFork,
WebhookID: sql.NullInt64{
Int64: r.HookId,
Valid: true,
},
CloneUrl: r.CloneUrl,
WebhookUrl: r.HookUrl,
DeployUrl: r.DeployUrl,
DefaultBranch: sql.NullString{
String: r.DefaultBranch,
Valid: true,
},
License: sql.NullString{
String: r.License,
Valid: true,
return &pb.RegisterRepositoryResponse{
Result: &pb.RegisterRepoResult{
Status: &pb.RegisterRepoResult_Status{
Success: true,
},
Repository: newRepo,
},
})
// even if we set the webhook, if we couldn't create it in the database, we'll return an error
if err != nil {
log.Printf("error creating repository '%s/%s' in database: %v", r.Owner, r.Name, err)

result.Status.Success = false
errorStr := "error creating repository in database"
result.Status.Error = &errorStr
return response, nil
}

repoDBID := dbRepo.ID.String()
r.Id = &repoDBID

// publish a reconciling event for the registered repositories
log.Printf("publishing register event for repository: %s/%s", r.Owner, r.Name)

msg, err := reconcilers.NewRepoReconcilerMessage(provider.Name, r.RepoId, projectID)
if err != nil {
log.Printf("error creating reconciler event: %v", err)
return response, nil
}

// This is a non-fatal error, so we'll just log it and continue with the next ones
if err := s.evt.Publish(reconcilers.InternalReconcilerEventTopic, msg); err != nil {
log.Printf("error publishing reconciler event: %v", err)
}

// Telemetry logging
logger.BusinessRecord(ctx).Provider = provider.Name
logger.BusinessRecord(ctx).Project = projectID
logger.BusinessRecord(ctx).Repository = dbRepo.ID

return response, nil
}, nil
}

// ListRepositories returns a list of repositories for a given project
Expand Down Expand Up @@ -323,7 +263,7 @@ func (s *Server) DeleteRepositoryById(
return nil, util.UserVisibleError(codes.InvalidArgument, "invalid repository ID")
}

err = s.deleteRepository(ctx, in, func(client v1.GitHub, projectID uuid.UUID) error {
err = s.deleteRepository(ctx, in, func(client v1.GitHub, projectID uuid.UUID, _ string) error {
return s.repos.DeleteRepositoryByID(ctx, client, projectID, parsedRepositoryID)
})
if err != nil {
Expand All @@ -347,8 +287,8 @@ func (s *Server) DeleteRepositoryByName(
return nil, util.UserVisibleError(codes.InvalidArgument, "invalid repository name, needs to have the format: owner/name")
}

err := s.deleteRepository(ctx, in, func(client v1.GitHub, projectID uuid.UUID) error {
return s.repos.DeleteRepositoryByName(ctx, client, projectID, fragments[0], fragments[1])
err := s.deleteRepository(ctx, in, func(client v1.GitHub, projectID uuid.UUID, providerName string) error {
return s.repos.DeleteRepositoryByName(ctx, client, projectID, providerName, fragments[0], fragments[1])
})
if err != nil {
return nil, err
Expand Down Expand Up @@ -458,15 +398,14 @@ func (s *Server) ListRemoteRepositoriesFromProvider(
// TODO: this probably can probably be used elsewhere
// returns project ID and github client - this flow of code exists in multiple
// places
func (s *Server) getProjectIDAndClient(
func (s *Server) getProviderAndClient(
ctx context.Context,
projectID uuid.UUID,
request HasProtoContext,
) (uuid.UUID, v1.GitHub, error) {
projectID := getProjectID(ctx)

) (*db.Provider, v1.GitHub, error) {
provider, err := getProviderFromRequestOrDefault(ctx, s.store, request, projectID)
if err != nil {
return uuid.Nil, nil, providerError(err)
return nil, nil, providerError(err)
}

pbOpts := []providers.ProviderBuilderOption{
Expand All @@ -476,29 +415,30 @@ func (s *Server) getProjectIDAndClient(

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

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

return projectID, client, nil
return &provider, client, nil
}

// covers the common logic for the two varieties of repo deletion
func (s *Server) deleteRepository(
ctx context.Context,
request HasProtoContext,
deletionMethod func(v1.GitHub, uuid.UUID) error,
deletionMethod func(v1.GitHub, uuid.UUID, string) error,
) error {
projectID, client, err := s.getProjectIDAndClient(ctx, request)
projectID := getProjectID(ctx)
provider, client, err := s.getProviderAndClient(ctx, projectID, request)
if err != nil {
return err
}

err = deletionMethod(client, projectID)
err = deletionMethod(client, projectID, provider.Name)
if errors.Is(err, sql.ErrNoRows) {
return status.Errorf(codes.NotFound, "repository not found")
} else if err != nil {
Expand Down
Loading

0 comments on commit 7b9a05a

Please sign in to comment.