diff --git a/internal/controlplane/handlers_githubwebhooks.go b/internal/controlplane/handlers_githubwebhooks.go index 7659f19f6b..359add114c 100644 --- a/internal/controlplane/handlers_githubwebhooks.go +++ b/internal/controlplane/handlers_githubwebhooks.go @@ -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, diff --git a/internal/controlplane/handlers_repositories.go b/internal/controlplane/handlers_repositories.go index 75845813e4..d636d36ea7 100644 --- a/internal/controlplane/handlers_repositories.go +++ b/internal/controlplane/handlers_repositories.go @@ -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" @@ -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" @@ -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 @@ -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 { @@ -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 @@ -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{ @@ -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 { diff --git a/internal/controlplane/handlers_repositories_test.go b/internal/controlplane/handlers_repositories_test.go index 3fdd850747..3dcba49690 100644 --- a/internal/controlplane/handlers_repositories_test.go +++ b/internal/controlplane/handlers_repositories_test.go @@ -17,23 +17,18 @@ package controlplane import ( "context" "database/sql" - "encoding/json" "errors" "net/http" - "reflect" "testing" - "github.com/ThreeDotsLabs/watermill/message" "github.com/go-git/go-git/v5" "github.com/google/go-github/v56/github" "github.com/google/uuid" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "golang.org/x/oauth2" - "google.golang.org/protobuf/proto" 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" @@ -41,8 +36,6 @@ import ( "github.com/stacklok/minder/internal/providers/ratecache" ghrepo "github.com/stacklok/minder/internal/repositories/github" mockghrepo "github.com/stacklok/minder/internal/repositories/github/mock" - mockghhook "github.com/stacklok/minder/internal/repositories/github/webhooks/mock" - "github.com/stacklok/minder/internal/util/ptr" pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" provinfv1 "github.com/stacklok/minder/pkg/providers/v1" ) @@ -50,190 +43,91 @@ import ( func TestServer_RegisterRepository(t *testing.T) { t.Parallel() - accessToken := "AUTH" - - tests := []struct { - name string - req *pb.RegisterRepositoryRequest - repo github.Repository - repoErr error - want *pb.RegisterRepositoryResponse - events []message.Message - wantErr bool - }{{ - name: "register repository, invalid upstream ID", - req: &pb.RegisterRepositoryRequest{ - Provider: "github", - Repository: &pb.UpstreamRepositoryRef{ - Owner: "test", - Name: "a-test", - RepoId: 31337, - }, - Context: &pb.Context{ - Provider: proto.String("github"), - Project: proto.String(uuid.NewString()), - }, + scenarios := []struct { + Name string + RepoOwner string + RepoName string + RepoServiceSetup repoMockBuilder + ProviderFails bool + ExpectedError string + }{ + { + Name: "Repo creation fails when provider cannot be found", + RepoOwner: repoOwner, + RepoName: repoName, + ProviderFails: true, + ExpectedError: "cannot retrieve providers", }, - repo: github.Repository{ - Owner: &github.User{ - Login: github.String("test"), - }, - Name: github.String("a-test"), - ID: github.Int64(1234), // NOTE: does not match RPC! + { + Name: "Repo creation fails when repo name is missing", + RepoOwner: repoOwner, + RepoName: "", + ExpectedError: "missing repository name", }, - want: &pb.RegisterRepositoryResponse{ - Result: &pb.RegisterRepoResult{ - Repository: &pb.Repository{ - Id: proto.String(uuid.NewString()), - Owner: "test", - Name: "a-test", - RepoId: 1234, - HookId: 1, - HookUuid: uuid.New().String(), - }, - Status: &pb.RegisterRepoResult_Status{ - Success: true, - }, - }, + { + Name: "Repo creation fails when repo does not exist in Github", + RepoOwner: repoOwner, + RepoName: repoName, + RepoServiceSetup: newRepoService(withFailedCreate(errDefault)), + ExpectedError: errDefault.Error(), }, - events: []message.Message{{ - Metadata: map[string]string{ - "provider": "github", - }, - Payload: []byte(`{"repository":1234}`), - }}, - }, { - name: "repo not found", - req: &pb.RegisterRepositoryRequest{ - Provider: "github", - Repository: &pb.UpstreamRepositoryRef{ - Owner: "test", - Name: "b-test", - }, - Context: &pb.Context{ - Provider: proto.String("github"), - Project: proto.String(uuid.NewString()), - }, - }, - repoErr: errors.New("Repo not found"), - want: &pb.RegisterRepositoryResponse{ - Result: &pb.RegisterRepoResult{ - // NOTE: the client as of v0.0.31 expects that the Repository - // field is always non-null, even if the repo doesn't exist. - Repository: &pb.Repository{ - Owner: "test", - Name: "b-test", - }, - Status: &pb.RegisterRepoResult_Status{ - Error: proto.String("Repo not found"), - }, - }, - }, - }} - for _, tc := range tests { - tt := tc - t.Run(tt.name, func(t *testing.T) { + Name: "Repo creation fails repo is private, and private repos are not allowed", + RepoOwner: repoOwner, + RepoName: repoName, + RepoServiceSetup: newRepoService(withFailedCreate(ghrepo.ErrPrivateRepoForbidden)), + ExpectedError: "private repos cannot be registered in this project", + }, + { + Name: "Repo creation on unexpected error", + RepoOwner: repoOwner, + RepoName: repoName, + RepoServiceSetup: newRepoService(withFailedCreate(errDefault)), + ExpectedError: errDefault.Error(), + }, + { + Name: "Repo creation is successful", + RepoOwner: repoOwner, + RepoName: repoName, + RepoServiceSetup: newRepoService(withSuccessfulCreate), + }, + } + + for i := range scenarios { + scenario := scenarios[i] + t.Run(scenario.Name, func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) defer ctrl.Finish() - mockStore := mockdb.NewMockStore(ctrl) - - projectUUID := uuid.MustParse(tt.req.GetContext().GetProject()) - - stubClient := StubGitHub{ - ExpectedOwner: tt.req.Repository.GetOwner(), - ExpectedRepo: tt.req.Repository.GetName(), - T: t, - Repo: &tt.repo, - RepoErr: tt.repoErr, - } - - mockStore.EXPECT(). - GetParentProjects(gomock.Any(), projectUUID). - Return([]uuid.UUID{projectUUID}, nil) - mockStore.EXPECT(). - ListProvidersByProjectID(gomock.Any(), []uuid.UUID{projectUUID}). - Return([]db.Provider{{ - ID: uuid.New(), - Name: "github", - Implements: []db.ProviderType{db.ProviderTypeGithub}, - Version: provinfv1.V1, - }}, nil) - mockStore.EXPECT(). - GetAccessTokenByProjectID(gomock.Any(), gomock.Any()). - Return(db.ProviderAccessToken{ - EncryptedToken: "encryptedToken", - }, nil).Times(2) - if tt.repoErr == nil { - mockStore.EXPECT(). - CreateRepository(gomock.Any(), gomock.Any()). - Return(db.Repository{ - ID: uuid.MustParse(tt.want.Result.Repository.GetId()), - }, nil) - } - - cancelable, cancel := context.WithCancel(context.Background()) - clientCache := ratecache.NewRestClientCache(cancelable) - defer cancel() - clientCache.Set("", accessToken, db.ProviderTypeGithub, &stubClient) - - stubEventer := StubEventer{} - stubWebhookManager := mockghhook.NewMockWebhookManager(ctrl) - if tt.repoErr == nil { - stubbedHook := &github.Hook{ - ID: ptr.Ptr[int64](1), - } - stubWebhookManager.EXPECT(). - CreateWebhook(gomock.Any(), gomock.Any(), gomock.Eq(tt.req.Repository.Owner), gomock.Eq(tt.req.Repository.Name)). - Return(tt.want.Result.Repository.HookUuid, stubbedHook, nil) - } - mockCryptoEngine := mockcrypto.NewMockEngine(ctrl) - mockCryptoEngine.EXPECT().DecryptOAuthToken(gomock.Any()).Return(oauth2.Token{AccessToken: accessToken}, nil).AnyTimes() - - s := &Server{ - store: mockStore, - cryptoEngine: mockCryptoEngine, - restClientCache: clientCache, - cfg: &server.Config{}, - evt: &stubEventer, - webhookManager: stubWebhookManager, - } ctx := engine.WithEntityContext(context.Background(), &engine.EntityContext{ - Provider: engine.Provider{Name: tt.req.Context.GetProvider()}, - Project: engine.Project{ID: projectUUID}, + Provider: engine.Provider{Name: ghprovider.Github}, + Project: engine.Project{ID: projectID}, }) - got, err := s.RegisterRepository(ctx, tt.req) - if (err != nil) != tt.wantErr { - t.Errorf("Server.RegisterRepository() error = %v, wantErr %v", err, tt.wantErr) - return - } + server := createServer(ctrl, scenario.RepoServiceSetup, scenario.ProviderFails) - if len(tt.events) != len(stubEventer.Sent) { - t.Fatalf("expected %d events, got %d", len(tt.events), len(stubEventer.Sent)) + req := &pb.RegisterRepositoryRequest{ + Repository: &pb.UpstreamRepositoryRef{ + Owner: scenario.RepoOwner, + Name: scenario.RepoName, + }, } - for i := range tt.events { - got := stubEventer.Sent[i] - want := &tt.events[i] - if !reflect.DeepEqual(got.Metadata, want.Metadata) { - t.Errorf("event %d.Metadata = %+v, want %+v", i, got.Metadata, want.Metadata) - } - var gotPayload, wantPayload EventPayload - if err := json.Unmarshal(got.Payload, &gotPayload); err != nil { - t.Fatalf("failed to unmarshal event %d.Payload: %v", i, err) - } - if err := json.Unmarshal(want.Payload, &wantPayload); err != nil { - t.Fatalf("failed to unmarshal event %d.Payload: %v", i, err) - } - if !reflect.DeepEqual(gotPayload.Repository, wantPayload.Repository) { - t.Errorf("event %d.Payload = %q, want %q", i, string(got.Payload), string(want.Payload)) + res, err := server.RegisterRepository(ctx, req) + if scenario.ExpectedError == "" { + expectation := &pb.RegisterRepositoryResponse{ + Result: &pb.RegisterRepoResult{ + Repository: creationResult, + Status: &pb.RegisterRepoResult_Status{ + Success: true, + }, + }, } - } - - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("Server.RegisterRepository() = %v, want %v", got, tt.want) + require.NoError(t, err) + require.Equal(t, res, expectation) + } else { + require.Nil(t, res) + require.Contains(t, err.Error(), scenario.ExpectedError) } }) } @@ -352,6 +246,8 @@ type ( ) const ( + repoOwner = "acme-corp" + repoName = "api-gateway" repoOwnerAndName = "acme-corp/api-gateway" repoID = "3eb6d254-4163-460f-89f7-44e2ae916e71" accessToken = "TOKEN" @@ -360,6 +256,16 @@ const ( var ( projectID = uuid.New() errDefault = errors.New("oh no") + provider = db.Provider{ + ID: uuid.UUID{}, + Name: ghprovider.Github, + Implements: []db.ProviderType{db.ProviderTypeGithub}, + Version: provinfv1.V1, + } + creationResult = &pb.Repository{ + Owner: repoOwner, + Name: repoName, + } ) func newRepoService(opts ...func(repoServiceMock)) repoMockBuilder { @@ -372,6 +278,20 @@ func newRepoService(opts ...func(repoServiceMock)) repoMockBuilder { } } +func withSuccessfulCreate(mock repoServiceMock) { + mock.EXPECT(). + CreateRepository(gomock.Any(), 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). + Return(nil, err) + } +} + func withSuccessfulDeleteByName(mock repoServiceMock) { withFailedDeleteByName(nil)(mock) } @@ -379,7 +299,7 @@ func withSuccessfulDeleteByName(mock repoServiceMock) { func withFailedDeleteByName(err error) func(repoServiceMock) { return func(mock repoServiceMock) { mock.EXPECT(). - DeleteRepositoryByName(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DeleteRepositoryByName(gomock.Any(), gomock.Any(), projectID, gomock.Any(), repoOwner, repoName). Return(err) } } @@ -391,7 +311,7 @@ func withSuccessfulDeleteByID(mock repoServiceMock) { func withFailedDeleteByID(err error) func(repoServiceMock) { return func(mock repoServiceMock) { mock.EXPECT(). - DeleteRepositoryByID(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DeleteRepositoryByID(gomock.Any(), gomock.Any(), projectID, gomock.Any()). Return(err) } } @@ -431,12 +351,7 @@ func createServer( } else { store.EXPECT(). ListProvidersByProjectID(gomock.Any(), []uuid.UUID{projectID}). - Return([]db.Provider{{ - ID: uuid.New(), - Name: "github", - Implements: []db.ProviderType{db.ProviderTypeGithub}, - Version: provinfv1.V1, - }}, nil).AnyTimes() + Return([]db.Provider{provider}, nil).AnyTimes() store.EXPECT(). GetAccessTokenByProjectID(gomock.Any(), gomock.Any()). Return(db.ProviderAccessToken{ @@ -565,17 +480,8 @@ func (*StubGitHub) UpdateReview(context.Context, string, string, int, int64, str } // GetRepository implements v1.GitHub. -func (s *StubGitHub) GetRepository(_ context.Context, owner string, repo string) (*github.Repository, error) { - if owner != s.ExpectedOwner { - s.T.Errorf("expected owner %q, got %q", s.ExpectedOwner, owner) - } - if repo != s.ExpectedRepo { - s.T.Errorf("expected repo %q, got %q", s.ExpectedRepo, repo) - } - if s.RepoErr != nil { - return nil, s.RepoErr - } - return s.Repo, nil +func (*StubGitHub) GetRepository(_ context.Context, _ string, _ string) (*github.Repository, error) { + panic("unimplemented") } // GetCredential implements v1.GitHub. diff --git a/internal/controlplane/server.go b/internal/controlplane/server.go index 27bc395eea..0ff7a1bf30 100644 --- a/internal/controlplane/server.go +++ b/internal/controlplane/server.go @@ -92,9 +92,6 @@ type Server struct { ruleTypes ruletypes.RuleTypeService repos github.RepositoryService profiles profiles.ProfileService - // TODO: this will be removed from server when the create repo - // flow is refactored - webhookManager webhooks.WebhookManager // Implementations for service registration pb.UnimplementedHealthServiceServer @@ -166,7 +163,6 @@ func NewServer( profiles: profileSvc, ruleTypes: ruletypes.NewRuleTypeService(store), repos: github.NewRepositoryService(whManager, store, evt), - webhookManager: whManager, // TODO: this currently always returns authorized as a transitionary measure. // When OpenFGA is fully rolled out, we may want to make this a hard error or set to false. authzClient: &mock.NoopClient{Authorized: true}, diff --git a/internal/repositories/github/mock/service.go b/internal/repositories/github/mock/service.go index eaae5a55a3..345e52082a 100644 --- a/internal/repositories/github/mock/service.go +++ b/internal/repositories/github/mock/service.go @@ -14,7 +14,9 @@ import ( reflect "reflect" uuid "github.com/google/uuid" + db "github.com/stacklok/minder/internal/db" clients "github.com/stacklok/minder/internal/repositories/github/clients" + v1 "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" gomock "go.uber.org/mock/gomock" ) @@ -41,6 +43,21 @@ func (m *MockRepositoryService) EXPECT() *MockRepositoryServiceMockRecorder { return m.recorder } +// CreateRepository mocks base method. +func (m *MockRepositoryService) CreateRepository(arg0 context.Context, arg1 clients.GitHubRepoClient, arg2 *db.Provider, arg3 uuid.UUID, arg4, arg5 string) (*v1.Repository, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreateRepository", arg0, arg1, arg2, arg3, arg4, arg5) + ret0, _ := ret[0].(*v1.Repository) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CreateRepository indicates an expected call of CreateRepository. +func (mr *MockRepositoryServiceMockRecorder) CreateRepository(arg0, arg1, arg2, arg3, arg4, arg5 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateRepository", reflect.TypeOf((*MockRepositoryService)(nil).CreateRepository), arg0, arg1, arg2, arg3, arg4, arg5) +} + // DeleteRepositoryByID mocks base method. func (m *MockRepositoryService) DeleteRepositoryByID(arg0 context.Context, arg1 clients.GitHubRepoClient, arg2, arg3 uuid.UUID) error { m.ctrl.T.Helper() @@ -56,15 +73,15 @@ func (mr *MockRepositoryServiceMockRecorder) DeleteRepositoryByID(arg0, arg1, ar } // DeleteRepositoryByName mocks base method. -func (m *MockRepositoryService) DeleteRepositoryByName(arg0 context.Context, arg1 clients.GitHubRepoClient, arg2 uuid.UUID, arg3, arg4 string) error { +func (m *MockRepositoryService) DeleteRepositoryByName(arg0 context.Context, arg1 clients.GitHubRepoClient, arg2 uuid.UUID, arg3, arg4, arg5 string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteRepositoryByName", arg0, arg1, arg2, arg3, arg4) + ret := m.ctrl.Call(m, "DeleteRepositoryByName", arg0, arg1, arg2, arg3, arg4, arg5) ret0, _ := ret[0].(error) return ret0 } // DeleteRepositoryByName indicates an expected call of DeleteRepositoryByName. -func (mr *MockRepositoryServiceMockRecorder) DeleteRepositoryByName(arg0, arg1, arg2, arg3, arg4 any) *gomock.Call { +func (mr *MockRepositoryServiceMockRecorder) DeleteRepositoryByName(arg0, arg1, arg2, arg3, arg4, arg5 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteRepositoryByName", reflect.TypeOf((*MockRepositoryService)(nil).DeleteRepositoryByName), arg0, arg1, arg2, arg3, arg4) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteRepositoryByName", reflect.TypeOf((*MockRepositoryService)(nil).DeleteRepositoryByName), arg0, arg1, arg2, arg3, arg4, arg5) } diff --git a/internal/repositories/github/service.go b/internal/repositories/github/service.go index 6628856f32..8104fab981 100644 --- a/internal/repositories/github/service.go +++ b/internal/repositories/github/service.go @@ -17,27 +17,45 @@ package github import ( "context" + "database/sql" + "errors" "fmt" + "github.com/google/go-github/v56/github" "github.com/google/uuid" "github.com/rs/zerolog/log" "github.com/stacklok/minder/internal/db" "github.com/stacklok/minder/internal/events" "github.com/stacklok/minder/internal/logger" - ghprovider "github.com/stacklok/minder/internal/providers/github/oauth" + "github.com/stacklok/minder/internal/projects/features" + "github.com/stacklok/minder/internal/reconcilers" ghclient "github.com/stacklok/minder/internal/repositories/github/clients" "github.com/stacklok/minder/internal/repositories/github/webhooks" + "github.com/stacklok/minder/internal/util/ptr" + pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" ) // RepositoryService encapsulates logic related to registering and deleting repos +// TODO: get rid of the github client from this interface type RepositoryService interface { + // CreateRepository registers a GitHub repository, including creating + // a webhook in the repo in GitHub. + CreateRepository( + ctx context.Context, + client ghclient.GitHubRepoClient, + provider *db.Provider, + projectID uuid.UUID, + repoName string, + repoOwner string, + ) (*pb.Repository, error) // DeleteRepositoryByName removes the webhook and deletes the repo from the // database. The repo is identified by its name and project. DeleteRepositoryByName( ctx context.Context, client ghclient.GitHubRepoClient, projectID uuid.UUID, + providerName string, repoOwner string, repoName string, ) error @@ -51,6 +69,13 @@ type RepositoryService interface { ) error } +var ( + // ErrPrivateRepoForbidden is returned when creation fails due to an + // attempt to register a private repo in a project which does not allow + // private repos + ErrPrivateRepoForbidden = errors.New("private repos cannot be registered in this project") +) + type repositoryService struct { webhookManager webhooks.WebhookManager store db.Store @@ -70,16 +95,76 @@ func NewRepositoryService( } } +func (r *repositoryService) CreateRepository( + ctx context.Context, + client ghclient.GitHubRepoClient, + provider *db.Provider, + projectID uuid.UUID, + repoOwner string, + repoName string, +) (*pb.Repository, error) { + // get information about the repo from GitHub, and ensure it exists + githubRepo, err := client.GetRepository(ctx, repoOwner, repoName) + if err != nil { + return nil, fmt.Errorf("error retrieving repo from github: %w", err) + } + + // skip if this is a private repo, and private repos are not enabled + if githubRepo.GetPrivate() && !features.ProjectAllowsPrivateRepos(ctx, r.store, projectID) { + return nil, ErrPrivateRepoForbidden + } + + // create a webhook to capture events from the repository + hookUUID, githubHook, err := r.webhookManager.CreateWebhook(ctx, client, repoOwner, repoName) + if err != nil { + return nil, fmt.Errorf("error creating webhook in repo: %w", err) + } + + // insert the repository into the DB + dbID, pbRepo, err := r.persistRepository( + ctx, + githubRepo, + githubHook, + hookUUID, + projectID, + provider, + ) + if err != nil { + log.Printf("error creating repository '%s/%s' in database: %v", repoOwner, repoName, err) + // Attempt to clean up the webhook we created earlier. This is a + // best-effort attempt: If it fails, the customer either has to delete + // the hook manually, or it will be deleted the next time the customer + // attempts to register a repo. + cleanupErr := r.webhookManager.DeleteWebhook(ctx, client, repoOwner, repoName, *githubHook.ID) + if cleanupErr != nil { + log.Printf("error deleting new webhook: %v", cleanupErr) + } + return nil, fmt.Errorf("error creating repository in database: %w", err) + } + + // publish a reconciling event for the registered repositories + if err = r.pushReconcilerEvent(pbRepo, projectID, provider.Name); err != nil { + return nil, err + } + + // Telemetry logging + logger.BusinessRecord(ctx).Provider = provider.Name + logger.BusinessRecord(ctx).Project = projectID + logger.BusinessRecord(ctx).Repository = dbID + + return pbRepo, nil +} + func (r *repositoryService) DeleteRepositoryByName( ctx context.Context, client ghclient.GitHubRepoClient, projectID uuid.UUID, + providerName string, repoOwner string, repoName string, ) error { - // assumption: provider name should always be `ghprovider.Github` for this Github-specific code params := db.GetRepositoryByRepoNameParams{ - Provider: ghprovider.Github, + Provider: providerName, RepoOwner: repoOwner, RepoName: repoName, ProjectID: projectID, @@ -134,3 +219,75 @@ func (r *repositoryService) deleteRepository(ctx context.Context, client ghclien return nil } + +func (r *repositoryService) pushReconcilerEvent(pbRepo *pb.Repository, projectID uuid.UUID, providerName string) error { + log.Printf("publishing register event for repository: %s/%s", pbRepo.Owner, pbRepo.Name) + + msg, err := reconcilers.NewRepoReconcilerMessage(providerName, pbRepo.RepoId, projectID) + if err != nil { + return fmt.Errorf("error creating reconciler event: %v", err) + } + + // This is a non-fatal error, so we'll just log it and continue with the next ones + if err = r.eventProducer.Publish(reconcilers.InternalReconcilerEventTopic, msg); err != nil { + log.Printf("error publishing reconciler event: %v", err) + } + + return nil +} + +// returns DB PK along with protobuf representation of a repo +func (r *repositoryService) persistRepository( + ctx context.Context, + githubRepo *github.Repository, + githubHook *github.Hook, + hookUUID string, + projectID uuid.UUID, + provider *db.Provider, +) (uuid.UUID, *pb.Repository, error) { + // instantiate the response object + pbRepo := &pb.Repository{ + Name: githubRepo.GetName(), + Owner: githubRepo.GetOwner().GetLogin(), + RepoId: githubRepo.GetID(), + HookId: githubHook.GetID(), + HookUrl: githubHook.GetURL(), + DeployUrl: githubRepo.GetDeploymentsURL(), + CloneUrl: githubRepo.GetCloneURL(), + HookType: githubHook.GetType(), + HookName: githubHook.GetName(), + HookUuid: hookUUID, + IsPrivate: githubRepo.GetPrivate(), + IsFork: githubRepo.GetFork(), + DefaultBranch: githubRepo.GetDefaultBranch(), + } + + // update the database + dbRepo, err := r.store.CreateRepository(ctx, db.CreateRepositoryParams{ + Provider: provider.Name, + ProviderID: provider.ID, + ProjectID: projectID, + RepoOwner: pbRepo.Owner, + RepoName: pbRepo.Name, + RepoID: pbRepo.RepoId, + IsPrivate: pbRepo.IsPrivate, + IsFork: pbRepo.IsFork, + WebhookID: sql.NullInt64{ + Int64: pbRepo.HookId, + Valid: true, + }, + CloneUrl: pbRepo.CloneUrl, + WebhookUrl: pbRepo.HookUrl, + DeployUrl: pbRepo.DeployUrl, + DefaultBranch: sql.NullString{ + String: pbRepo.DefaultBranch, + Valid: true, + }, + }) + if err != nil { + return uuid.Nil, nil, err + } + + pbRepo.Id = ptr.Ptr(dbRepo.ID.String()) + return dbRepo.ID, pbRepo, nil +} diff --git a/internal/repositories/github/service_test.go b/internal/repositories/github/service_test.go index a1b0af6e83..63bfb343b2 100644 --- a/internal/repositories/github/service_test.go +++ b/internal/repositories/github/service_test.go @@ -17,9 +17,11 @@ package github_test import ( "context" "database/sql" + "encoding/json" "errors" "testing" + gh "github.com/google/go-github/v56/github" "github.com/google/uuid" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -32,8 +34,103 @@ import ( cf "github.com/stacklok/minder/internal/repositories/github/fixtures" "github.com/stacklok/minder/internal/repositories/github/webhooks" mockghhook "github.com/stacklok/minder/internal/repositories/github/webhooks/mock" + "github.com/stacklok/minder/internal/util/ptr" + pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" + provinfv1 "github.com/stacklok/minder/pkg/providers/v1" ) +func TestRepositoryService_CreateRepository(t *testing.T) { + t.Parallel() + + scenarios := []struct { + Name string + ClientSetup cf.ClientMockBuilder + DBSetup dbMockBuilder + WebhookSetup whMockBuilder + EventsSetup eventMockBuilder + EventSendFails bool + ExpectedError string + }{ + { + Name: "CreateRepository fails when repo cannot be found in GitHub", + ClientSetup: cf.NewClientMock(cf.WithFailedGet), + ExpectedError: "error retrieving repo from github", + }, + { + Name: "CreateRepository fails for private repo in project which disallows private repos", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(privateRepo)), + DBSetup: newDBMock(withPrivateReposDisabled), + ExpectedError: "private repos cannot be registered in this project", + }, + { + Name: "CreateRepository fails when webhook creation fails", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + WebhookSetup: newWebhookMock(withFailedWebhookCreate), + ExpectedError: "error creating webhook in repo", + }, + { + Name: "CreateRepository fails when repo cannot be inserted into database", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + DBSetup: newDBMock(withFailedCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate, withSuccessfulWebhookDelete), + ExpectedError: "error creating repository", + }, + { + Name: "CreateRepository fails when repo cannot be inserted into database (cleanup fails)", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + DBSetup: newDBMock(withFailedCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate, withFailedWebhookDelete), + ExpectedError: "error creating repository", + }, + { + Name: "CreateRepository succeeds", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + DBSetup: newDBMock(withSuccessfulCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate), + }, + { + Name: "CreateRepository succeeds (private repos enabled)", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(privateRepo)), + DBSetup: newDBMock(withPrivateReposEnabled, withSuccessfulCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate), + }, + { + Name: "CreateRepository succeeds (skips failed event send)", + ClientSetup: cf.NewClientMock(cf.WithSuccessfulGet(publicRepo)), + DBSetup: newDBMock(withSuccessfulCreate), + WebhookSetup: newWebhookMock(withSuccessfulWebhookCreate), + EventSendFails: true, + }, + } + + for i := range scenarios { + scenario := scenarios[i] + t.Run(scenario.Name, func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + ctx := context.Background() + + var ghClient clients.GitHubRepoClient + if scenario.ClientSetup != nil { + ghClient = scenario.ClientSetup(ctrl) + } + + svc := createService(ctrl, scenario.WebhookSetup, scenario.DBSetup, scenario.EventSendFails) + res, err := svc.CreateRepository(ctx, ghClient, &provider, projectID, repoOwner, repoName) + if scenario.ExpectedError == "" { + require.NoError(t, err) + // cheat here a little... + expectation := newExpectation(res.IsPrivate) + require.Equal(t, expectation, res) + } else { + require.Nil(t, res) + require.ErrorContains(t, err, scenario.ExpectedError) + } + }) + } +} + func TestRepositoryService_DeleteRepository(t *testing.T) { t.Parallel() @@ -112,7 +209,7 @@ func TestRepositoryService_DeleteRepository(t *testing.T) { case ByID: err = svc.DeleteRepositoryByID(ctx, ghClient, projectID, repoID) case ByName: - err = svc.DeleteRepositoryByName(ctx, ghClient, projectID, repoOwner, repoName) + err = svc.DeleteRepositoryByName(ctx, ghClient, projectID, providerName, repoOwner, repoName) default: t.Fatalf("Unknown deletion type: %d", scenario.DeleteType) } @@ -155,8 +252,9 @@ func createService( } const ( - repoOwner = "acme-corp" - repoName = "api-gateway" + repoOwner = "acme-corp" + repoName = "api-gateway" + providerName = "github" ) type DeleteCallType int @@ -167,7 +265,9 @@ const ( ) var ( + hookUUID = uuid.New().String() repoID = uuid.New() + ghRepoID = ptr.Ptr[int64](0xE1E10) projectID = uuid.New() errDefault = errors.New("uh oh") dbRepo = db.Repository{ @@ -180,13 +280,26 @@ var ( Int64: cf.HookID, }, } + webhook = &gh.Hook{ + ID: ptr.Ptr[int64](cf.HookID), + } + publicRepo = newGithubRepo(false) + privateRepo = newGithubRepo(true) + provider = db.Provider{ + ID: uuid.UUID{}, + Name: providerName, + Implements: []db.ProviderType{db.ProviderTypeGithub}, + Version: provinfv1.V1, + } ) type ( - dbMock = *mockdb.MockStore - dbMockBuilder = func(controller *gomock.Controller) dbMock - whMock = *mockghhook.MockWebhookManager - whMockBuilder = func(controller *gomock.Controller) whMock + dbMock = *mockdb.MockStore + dbMockBuilder = func(controller *gomock.Controller) dbMock + whMock = *mockghhook.MockWebhookManager + whMockBuilder = func(controller *gomock.Controller) whMock + eventMock = *mockevents.MockInterface + eventMockBuilder = func(controller *gomock.Controller) eventMock ) func newDBMock(opts ...func(dbMock)) dbMockBuilder { @@ -209,15 +322,27 @@ func newWebhookMock(opts ...func(mock whMock)) whMockBuilder { } } +func withSuccessfulWebhookCreate(mock whMock) { + mock.EXPECT(). + CreateWebhook(gomock.Any(), gomock.Any(), repoOwner, repoName). + Return(hookUUID, webhook, nil) +} + +func withFailedWebhookCreate(mock whMock) { + mock.EXPECT(). + CreateWebhook(gomock.Any(), gomock.Any(), repoOwner, repoName). + Return("", nil, errDefault) +} + func withSuccessfulWebhookDelete(mock whMock) { mock.EXPECT(). - DeleteWebhook(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DeleteWebhook(gomock.Any(), gomock.Any(), repoOwner, repoName, cf.HookID). Return(nil) } func withFailedWebhookDelete(mock whMock) { mock.EXPECT(). - DeleteWebhook(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + DeleteWebhook(gomock.Any(), gomock.Any(), repoOwner, repoName, cf.HookID). Return(errDefault) } @@ -257,3 +382,61 @@ func withSuccessfulDelete(mock dbMock) { DeleteRepository(gomock.Any(), gomock.Eq(repoID)). Return(nil) } + +func withFailedCreate(mock dbMock) { + mock.EXPECT(). + CreateRepository(gomock.Any(), gomock.Any()). + Return(db.Repository{}, errDefault) +} + +func withSuccessfulCreate(mock dbMock) { + mock.EXPECT(). + CreateRepository(gomock.Any(), gomock.Any()). + Return(dbRepo, nil) +} + +func withPrivateReposEnabled(mock dbMock) { + mock.EXPECT(). + GetFeatureInProject(gomock.Any(), gomock.Any()). + Return(json.RawMessage{}, nil) +} + +func withPrivateReposDisabled(mock dbMock) { + mock.EXPECT(). + GetFeatureInProject(gomock.Any(), gomock.Any()). + Return(json.RawMessage{}, sql.ErrNoRows) +} + +func newGithubRepo(isPrivate bool) *gh.Repository { + return &gh.Repository{ + ID: ghRepoID, + Name: ptr.Ptr(repoName), + Owner: &gh.User{ + Login: ptr.Ptr(repoOwner), + }, + Private: ptr.Ptr(isPrivate), + DeploymentsURL: ptr.Ptr("https://foo.com"), + CloneURL: ptr.Ptr("http://cloneurl.com"), + Fork: ptr.Ptr(false), + DefaultBranch: ptr.Ptr("main"), + } +} + +func newExpectation(isPrivate bool) *pb.Repository { + return &pb.Repository{ + Id: ptr.Ptr(dbRepo.ID.String()), + Name: publicRepo.GetName(), + Owner: publicRepo.GetOwner().GetLogin(), + RepoId: publicRepo.GetID(), + HookId: webhook.GetID(), + HookUrl: webhook.GetURL(), + DeployUrl: publicRepo.GetDeploymentsURL(), + CloneUrl: publicRepo.GetCloneURL(), + HookType: webhook.GetType(), + HookName: webhook.GetName(), + HookUuid: hookUUID, + IsPrivate: isPrivate, + IsFork: publicRepo.GetFork(), + DefaultBranch: publicRepo.GetDefaultBranch(), + } +} diff --git a/internal/ruletypes/service.go b/internal/ruletypes/service.go index a94691f7a4..f698b2d47a 100644 --- a/internal/ruletypes/service.go +++ b/internal/ruletypes/service.go @@ -85,6 +85,10 @@ func (r *ruleTypeService) CreateRuleType( provider db.Provider, ruleType *pb.RuleType, ) (*pb.RuleType, error) { + // Telemetry logging + logger.BusinessRecord(ctx).Provider = provider.Name + logger.BusinessRecord(ctx).Project = projectID + if err := ruleType.Validate(); err != nil { return nil, errors.Join(ErrRuleTypeInvalid, err) } @@ -128,16 +132,13 @@ func (r *ruleTypeService) CreateRuleType( return nil, fmt.Errorf("failed to create rule type: %w", err) } + logger.BusinessRecord(ctx).RuleType = logger.RuleType{Name: newDBRecord.Name, ID: newDBRecord.ID} + rt, err := engine.RuleTypePBFromDB(&newDBRecord) if err != nil { return nil, fmt.Errorf("cannot convert rule type %s to pb: %w", newDBRecord.Name, err) } - // Telemetry logging - logger.BusinessRecord(ctx).Provider = newDBRecord.Provider - logger.BusinessRecord(ctx).Project = newDBRecord.ProjectID - logger.BusinessRecord(ctx).RuleType = logger.RuleType{Name: newDBRecord.Name, ID: newDBRecord.ID} - return rt, nil } @@ -147,6 +148,10 @@ func (r *ruleTypeService) UpdateRuleType( provider db.Provider, ruleType *pb.RuleType, ) (*pb.RuleType, error) { + // Telemetry logging + logger.BusinessRecord(ctx).Provider = provider.Name + logger.BusinessRecord(ctx).Project = projectID + if err := ruleType.Validate(); err != nil { return nil, errors.Join(ErrRuleTypeInvalid, err) } @@ -193,16 +198,13 @@ func (r *ruleTypeService) UpdateRuleType( return nil, fmt.Errorf("failed to update rule type: %w", err) } + logger.BusinessRecord(ctx).RuleType = logger.RuleType{Name: existingRuleType.Name, ID: existingRuleType.ID} + result, err := engine.RuleTypePBFromDB(&updatedRuleType) if err != nil { return nil, fmt.Errorf("cannot convert rule type %s to pb: %w", existingRuleType.Name, err) } - // Telemetry logging - logger.BusinessRecord(ctx).Provider = existingRuleType.Provider - logger.BusinessRecord(ctx).Project = existingRuleType.ProjectID - logger.BusinessRecord(ctx).RuleType = logger.RuleType{Name: existingRuleType.Name, ID: existingRuleType.ID} - return result, nil }