From 705ec00d421ca9c05123a61e5c1158e0c4cdd89f Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Thu, 12 Dec 2024 15:17:48 +0200 Subject: [PATCH 1/7] Add PullRequestCommenter provider trait This allows for providers to comment on pull requests Signed-off-by: Juan Antonio Osorio --- internal/engine/actions/alert/alert.go | 2 +- .../pull_request_comment.go | 144 ++++----- .../pull_request_comment_test.go | 26 +- internal/providers/github/clients/app.go | 14 + internal/providers/github/clients/oauth.go | 9 + internal/providers/github/commenter.go | 289 ++++++++++++++++++ internal/providers/github/common.go | 29 ++ internal/providers/github/mock/github.go | 170 +++++++++++ pkg/providers/v1/mock/providers.go | 170 +++++++++++ pkg/providers/v1/providers.go | 50 +++ 10 files changed, 796 insertions(+), 107 deletions(-) create mode 100644 internal/providers/github/commenter.go diff --git a/internal/engine/actions/alert/alert.go b/internal/engine/actions/alert/alert.go index 5da2a06ab3..c8ac0ddbd5 100644 --- a/internal/engine/actions/alert/alert.go +++ b/internal/engine/actions/alert/alert.go @@ -53,7 +53,7 @@ func NewRuleAlert( if alertCfg.GetPullRequestComment() == nil { return nil, fmt.Errorf("alert engine missing pull_request_review configuration") } - client, err := provinfv1.As[provinfv1.GitHub](provider) + client, err := provinfv1.As[provinfv1.PullRequestCommenter](provider) if err != nil { zerolog.Ctx(ctx).Debug().Str("rule-type", ruletype.GetName()). Msg("provider is not a GitHub provider. Silently skipping alerts.") diff --git a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go index 3bb9d23c66..4b61f8fdf5 100644 --- a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go +++ b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go @@ -8,11 +8,7 @@ package pull_request_comment import ( "context" "encoding/json" - "errors" "fmt" - "math" - "strconv" - "time" "github.com/google/go-github/v63/github" "github.com/rs/zerolog" @@ -21,6 +17,7 @@ import ( "github.com/mindersec/minder/internal/db" enginerr "github.com/mindersec/minder/internal/engine/errors" "github.com/mindersec/minder/internal/engine/interfaces" + "github.com/mindersec/minder/internal/entities/properties" pbinternal "github.com/mindersec/minder/internal/proto" "github.com/mindersec/minder/internal/util" pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1" @@ -39,7 +36,7 @@ const ( // Alert is the structure backing the noop alert type Alert struct { actionType interfaces.ActionType - gh provifv1.GitHub + commenter provifv1.PullRequestCommenter reviewCfg *pb.RuleType_Definition_Alert_AlertTypePRComment setting models.ActionOpt } @@ -54,26 +51,17 @@ type PrCommentTemplateParams struct { } type paramsPR struct { - Owner string - Repo string - CommitSha string - Number int Comment string - Metadata *alertMetadata + props *properties.Properties + Metadata *provifv1.CommentResultMeta prevStatus *db.ListRuleEvaluationsByProfileIdRow } -type alertMetadata struct { - ReviewID string `json:"review_id,omitempty"` - SubmittedAt *time.Time `json:"submitted_at,omitempty"` - PullRequestUrl *string `json:"pull_request_url,omitempty"` -} - // NewPullRequestCommentAlert creates a new pull request comment alert action func NewPullRequestCommentAlert( actionType interfaces.ActionType, reviewCfg *pb.RuleType_Definition_Alert_AlertTypePRComment, - gh provifv1.GitHub, + gh provifv1.PullRequestCommenter, setting models.ActionOpt, ) (*Alert, error) { if actionType == "" { @@ -82,7 +70,7 @@ func NewPullRequestCommentAlert( return &Alert{ actionType: actionType, - gh: gh, + commenter: gh, reviewCfg: reviewCfg, setting: setting, }, nil @@ -134,70 +122,20 @@ func (alert *Alert) Do( } func (alert *Alert) run(ctx context.Context, params *paramsPR, cmd interfaces.ActionCmd) (json.RawMessage, error) { - logger := zerolog.Ctx(ctx) - // Process the command switch cmd { - // Create a review case interfaces.ActionCmdOn: - review := &github.PullRequestReviewRequest{ - CommitID: github.String(params.CommitSha), - Event: github.String("COMMENT"), - Body: github.String(params.Comment), - } - - r, err := alert.gh.CreateReview( - ctx, - params.Owner, - params.Repo, - params.Number, - review, - ) - if err != nil { - return nil, fmt.Errorf("error creating PR review: %w, %w", err, enginerr.ErrActionFailed) - } - - newMeta, err := json.Marshal(alertMetadata{ - ReviewID: strconv.FormatInt(r.GetID(), 10), - SubmittedAt: r.SubmittedAt.GetTime(), - PullRequestUrl: r.PullRequestURL, - }) - if err != nil { - return nil, fmt.Errorf("error marshalling alert metadata json: %w", err) - } - - logger.Info().Int64("review_id", *r.ID).Msg("PR review created") - return newMeta, nil - // Dismiss the review + // Create a review + return alert.runDoReview(ctx, params) case interfaces.ActionCmdOff: - if params.Metadata == nil { - // We cannot do anything without the PR review ID, so we assume that turning the alert off is a success - return nil, fmt.Errorf("no PR comment ID provided: %w", enginerr.ErrActionTurnedOff) - } - - reviewID, err := strconv.ParseInt(params.Metadata.ReviewID, 10, 64) - if err != nil { - zerolog.Ctx(ctx).Error().Err(err).Str("review_id", params.Metadata.ReviewID).Msg("failed to parse review_id") - return nil, fmt.Errorf("no PR comment ID provided: %w, %w", err, enginerr.ErrActionTurnedOff) - } - - _, err = alert.gh.DismissReview(ctx, params.Owner, params.Repo, params.Number, reviewID, - &github.PullRequestReviewDismissalRequest{ - Message: github.String("Dismissed due to alert being turned off"), - }) - if err != nil { - if errors.Is(err, enginerr.ErrNotFound) { - // There's no PR review with that ID anymore. - // We exit by stating that the action was turned off. - return nil, fmt.Errorf("PR comment already dismissed: %w, %w", err, enginerr.ErrActionTurnedOff) - } - return nil, fmt.Errorf("error dismissing PR comment: %w, %w", err, enginerr.ErrActionFailed) - } - logger.Info().Str("review_id", params.Metadata.ReviewID).Msg("PR comment dismissed") - // Success - return ErrActionTurnedOff to indicate the action was successful - return nil, fmt.Errorf("%s : %w", alert.Class(), enginerr.ErrActionTurnedOff) + return json.RawMessage(`{}`), nil case interfaces.ActionCmdDoNothing: - // Return the previous alert status. + // If the previous status didn't change (still a failure, for instance) we + // want to refresh the alert. + if alert.setting == models.ActionOptOn { + return alert.runDoReview(ctx, params) + } + // Else, we just do nothing. return alert.runDoNothing(ctx, params) } return nil, enginerr.ErrActionSkipped @@ -211,16 +149,16 @@ func (alert *Alert) runDry(ctx context.Context, params *paramsPR, cmd interfaces switch cmd { case interfaces.ActionCmdOn: body := github.String(params.Comment) - logger.Info().Msgf("dry run: create a PR comment on PR %d in repo %s/%s with the following body: %s", - params.Number, params.Owner, params.Repo, *body) + logger.Info().Dict("properties", params.props.ToLogDict()). + Msgf("dry run: create a PR comment on PR with body: %s", *body) return nil, nil case interfaces.ActionCmdOff: if params.Metadata == nil { // We cannot do anything without the PR review ID, so we assume that turning the alert off is a success return nil, fmt.Errorf("no PR comment ID provided: %w", enginerr.ErrActionTurnedOff) } - logger.Info().Msgf("dry run: dismiss PR comment %s on PR PR %d in repo %s/%s", params.Metadata.ReviewID, - params.Number, params.Owner, params.Repo) + logger.Info().Dict("properties", params.props.ToLogDict()). + Msgf("dry run: dismiss PR comment on PR") case interfaces.ActionCmdDoNothing: // Return the previous alert status. return alert.runDoNothing(ctx, params) @@ -231,7 +169,7 @@ func (alert *Alert) runDry(ctx context.Context, params *paramsPR, cmd interfaces // runDoNothing returns the previous alert status func (_ *Alert) runDoNothing(ctx context.Context, params *paramsPR) (json.RawMessage, error) { - logger := zerolog.Ctx(ctx).With().Str("repo", params.Repo).Logger() + logger := zerolog.Ctx(ctx).With().Dict("properties", params.props.ToLogDict()).Logger() logger.Debug().Msg("Running do nothing") @@ -245,6 +183,30 @@ func (_ *Alert) runDoNothing(ctx context.Context, params *paramsPR) (json.RawMes return nil, err } +func (alert *Alert) runDoReview(ctx context.Context, params *paramsPR) (json.RawMessage, error) { + logger := zerolog.Ctx(ctx) + + r, err := alert.commenter.CommentOnPullRequest(ctx, params.props, provifv1.PullRequestCommentInfo{ + // TODO: We should add the header to identify the alert. We could use the + // rule type name. + Commit: params.props.GetProperty(properties.PullRequestCommitSHA).GetString(), + Body: params.Comment, + // TODO: Determine the priority from the rule type severity + }) + if err != nil { + return nil, fmt.Errorf("error creating PR review: %w, %w", err, enginerr.ErrActionFailed) + } + logger.Info().Str("review_id", r.ID).Msg("PR review created") + + // serialize r to json + meta, err := json.Marshal(r) + if err != nil { + return nil, fmt.Errorf("error marshalling PR review metadata: %w", err) + } + + return meta, nil +} + // getParamsForSecurityAdvisory extracts the details from the entity func (alert *Alert) getParamsForPRComment( ctx context.Context, @@ -253,19 +215,15 @@ func (alert *Alert) getParamsForPRComment( metadata *json.RawMessage, ) (*paramsPR, error) { logger := zerolog.Ctx(ctx) - result := ¶msPR{ - prevStatus: params.GetEvalStatusFromDb(), - Owner: pr.GetRepoOwner(), - Repo: pr.GetRepoName(), - CommitSha: pr.GetCommitSha(), + props, err := properties.NewProperties(pr.GetProperties().AsMap()) + if err != nil { + return nil, fmt.Errorf("error creating properties: %w", err) } - // The GitHub Go API takes an int32, but our proto stores an int64; make sure we don't overflow - // The PR number is an int in GitHub and Gitlab; in practice overflow will never happen. - if pr.Number > math.MaxInt { - return nil, fmt.Errorf("pr number is too large") + result := ¶msPR{ + prevStatus: params.GetEvalStatusFromDb(), + props: props, } - result.Number = int(pr.Number) commentTmpl, err := util.NewSafeHTMLTemplate(&alert.reviewCfg.ReviewMessage, "message") if err != nil { @@ -289,7 +247,7 @@ func (alert *Alert) getParamsForPRComment( // Unmarshal the existing alert metadata, if any if metadata != nil { - meta := &alertMetadata{} + meta := &provifv1.CommentResultMeta{} err := json.Unmarshal(*metadata, meta) if err != nil { // There's nothing saved apparently, so no need to fail here, but do log the error diff --git a/internal/engine/actions/alert/pull_request_comment/pull_request_comment_test.go b/internal/engine/actions/alert/pull_request_comment/pull_request_comment_test.go index f02dae1cc9..bf3aed30d0 100644 --- a/internal/engine/actions/alert/pull_request_comment/pull_request_comment_test.go +++ b/internal/engine/actions/alert/pull_request_comment/pull_request_comment_test.go @@ -18,10 +18,10 @@ import ( enginerr "github.com/mindersec/minder/internal/engine/errors" engif "github.com/mindersec/minder/internal/engine/interfaces" pbinternal "github.com/mindersec/minder/internal/proto" - mockghclient "github.com/mindersec/minder/internal/providers/github/mock" pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1" "github.com/mindersec/minder/pkg/engine/v1/interfaces" "github.com/mindersec/minder/pkg/profiles/models" + mockcommenter "github.com/mindersec/minder/pkg/providers/v1/mock" ) var TestActionTypeValid engif.ActionType = "alert-test" @@ -44,7 +44,7 @@ func TestPullRequestCommentAlert(t *testing.T) { cmd engif.ActionCmd reviewMsg string inputMetadata *json.RawMessage - mockSetup func(*mockghclient.MockGitHub) + mockSetup func(commenter *mockcommenter.MockPullRequestCommenter) expectedErr error expectedMetadata json.RawMessage }{ @@ -53,9 +53,9 @@ func TestPullRequestCommentAlert(t *testing.T) { actionType: TestActionTypeValid, reviewMsg: "This is a constant review message", cmd: engif.ActionCmdOn, - mockSetup: func(mockGitHub *mockghclient.MockGitHub) { + mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) { mockGitHub.EXPECT(). - CreateReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()). Return(&github.PullRequestReview{ID: &reviewID}, nil) }, expectedMetadata: json.RawMessage(fmt.Sprintf(`{"review_id":"%s"}`, reviewIDStr)), @@ -65,9 +65,9 @@ func TestPullRequestCommentAlert(t *testing.T) { actionType: TestActionTypeValid, reviewMsg: "{{ .EvalErrorDetails }}", cmd: engif.ActionCmdOn, - mockSetup: func(mockGitHub *mockghclient.MockGitHub) { + mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) { mockGitHub.EXPECT(). - CreateReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.AssignableToTypeOf(&github.PullRequestReviewRequest{})). + CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()). DoAndReturn(validateReviewBodyAndReturn(evaluationFailureDetails, reviewID)) }, expectedMetadata: json.RawMessage(fmt.Sprintf(`{"review_id":"%s"}`, reviewIDStr)), @@ -77,9 +77,9 @@ func TestPullRequestCommentAlert(t *testing.T) { actionType: TestActionTypeValid, reviewMsg: "{{ .EvalResultOutput.ViolationMsg }}", cmd: engif.ActionCmdOn, - mockSetup: func(mockGitHub *mockghclient.MockGitHub) { + mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) { mockGitHub.EXPECT(). - CreateReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.AssignableToTypeOf(&github.PullRequestReviewRequest{})). + CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()). DoAndReturn(validateReviewBodyAndReturn(violationMsg, reviewID)) }, expectedMetadata: json.RawMessage(fmt.Sprintf(`{"review_id":"%s"}`, reviewIDStr)), @@ -89,9 +89,9 @@ func TestPullRequestCommentAlert(t *testing.T) { actionType: TestActionTypeValid, reviewMsg: "This is a constant review message", cmd: engif.ActionCmdOn, - mockSetup: func(mockGitHub *mockghclient.MockGitHub) { + mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) { mockGitHub.EXPECT(). - CreateReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()). Return(nil, fmt.Errorf("failed to create PR comment")) }, expectedErr: enginerr.ErrActionFailed, @@ -102,9 +102,9 @@ func TestPullRequestCommentAlert(t *testing.T) { reviewMsg: "This is a constant review message", cmd: engif.ActionCmdOff, inputMetadata: &successfulRunMetadata, - mockSetup: func(mockGitHub *mockghclient.MockGitHub) { + mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) { mockGitHub.EXPECT(). - DismissReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()). Return(&github.PullRequestReview{}, nil) }, expectedErr: enginerr.ErrActionTurnedOff, @@ -126,7 +126,7 @@ func TestPullRequestCommentAlert(t *testing.T) { ReviewMessage: tt.reviewMsg, } - mockClient := mockghclient.NewMockGitHub(ctrl) + mockClient := mockcommenter.NewMockPullRequestCommenter(ctrl) tt.mockSetup(mockClient) prCommentAlert, err := NewPullRequestCommentAlert( diff --git a/internal/providers/github/clients/app.go b/internal/providers/github/clients/app.go index b65d3a1e15..da555d701c 100644 --- a/internal/providers/github/clients/app.go +++ b/internal/providers/github/clients/app.go @@ -292,6 +292,20 @@ func (g *GitHubAppDelegate) GetUserId(ctx context.Context) (int64, error) { return user.GetID(), nil } +// GetMinderUserId returns the user id for the GitHub App user +func (g *GitHubAppDelegate) GetMinderUserId(ctx context.Context) (int64, error) { + // Try to get this user ID from the GitHub API + //nolint:errcheck // this will never error + appUserName, _ := g.GetName(ctx) + user, _, err := g.client.Users.Get(ctx, appUserName) + if err != nil { + // Fallback to the configured user ID + // note: this is different from the App ID + return g.defaultUserId, nil + } + return user.GetID(), nil +} + // GetName returns the username for the GitHub App user func (g *GitHubAppDelegate) GetName(_ context.Context) (string, error) { return fmt.Sprintf("%s[bot]", g.appName), nil diff --git a/internal/providers/github/clients/oauth.go b/internal/providers/github/clients/oauth.go index a1ff847825..4149a280ed 100644 --- a/internal/providers/github/clients/oauth.go +++ b/internal/providers/github/clients/oauth.go @@ -213,6 +213,15 @@ func (o *GitHubOAuthDelegate) GetUserId(ctx context.Context) (int64, error) { return user.GetID(), nil } +// GetMinderUserId returns the user id for the authenticated user +func (o *GitHubOAuthDelegate) GetMinderUserId(ctx context.Context) (int64, error) { + user, _, err := o.client.Users.Get(ctx, "") + if err != nil { + return 0, err + } + return user.GetID(), nil +} + // GetName returns the username for the authenticated user func (o *GitHubOAuthDelegate) GetName(ctx context.Context) (string, error) { user, _, err := o.client.Users.Get(ctx, "") diff --git a/internal/providers/github/commenter.go b/internal/providers/github/commenter.go new file mode 100644 index 0000000000..e9f9c847da --- /dev/null +++ b/internal/providers/github/commenter.go @@ -0,0 +1,289 @@ +// SPDX-FileCopyrightText: Copyright 2023 The Minder Authors +// SPDX-License-Identifier: Apache-2.0 + +package github + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "regexp" + "strconv" + "strings" + "time" + + "github.com/google/go-github/v63/github" + "github.com/rs/zerolog" + + "github.com/mindersec/minder/internal/util" + "github.com/mindersec/minder/internal/util/ptr" + provifv1 "github.com/mindersec/minder/pkg/providers/v1" +) + +const ( + // MagicCommentLimit is the maximum length of the magic comment + MagicCommentLimit = 1024 + // CommentLimit is the maximum length of the comment + CommentLimit = 65536 + minderTemplateMagicCommentName = "minderCommentBody" + //nolint:lll + statusBodyMagicComment = `` + statusBodyMagicCommentPrefix = "", statusBodyMagicCommentPrefix)) + + matches := re.FindStringSubmatch(input) + if len(matches) != 2 { + return magicCommentInfo{}, errors.New("no match found") + } + + jsonPart := matches[1] + + var strMagicCommentInfo struct { + ContentSha string `json:"ContentSha"` + ReviewID string `json:"ReviewID"` // Assuming you're handling ReviewID as a string + } + err := json.Unmarshal([]byte(jsonPart), &strMagicCommentInfo) + if err != nil { + return magicCommentInfo{}, fmt.Errorf("error unmarshalling JSON: %w", err) + } + + var contentInfo magicCommentInfo + contentInfo.ContentSha = strMagicCommentInfo.ContentSha + contentInfo.ReviewID, err = strconv.ParseInt(strMagicCommentInfo.ReviewID, 10, 64) + if err != nil { + return magicCommentInfo{}, fmt.Errorf("error parsing ReviewID: %w", err) + } + + return contentInfo, nil +} diff --git a/internal/providers/github/common.go b/internal/providers/github/common.go index c512810d28..46c67f3ac1 100644 --- a/internal/providers/github/common.go +++ b/internal/providers/github/common.go @@ -25,6 +25,7 @@ import ( "github.com/mindersec/minder/internal/db" engerrors "github.com/mindersec/minder/internal/engine/errors" + entprops "github.com/mindersec/minder/internal/entities/properties" gitclient "github.com/mindersec/minder/internal/providers/git" "github.com/mindersec/minder/internal/providers/github/ghcr" "github.com/mindersec/minder/internal/providers/github/properties" @@ -145,6 +146,7 @@ type Delegate interface { GetCredential() provifv1.GitHubCredential ListAllRepositories(context.Context) ([]*minderv1.Repository, error) GetUserId(ctx context.Context) (int64, error) + GetMinderUserId(ctx context.Context) (int64, error) GetName(ctx context.Context) (string, error) GetLogin(ctx context.Context) (string, error) GetPrimaryEmail(ctx context.Context) (string, error) @@ -419,6 +421,33 @@ func (c *GitHub) ListFiles( return resp.files, resp.resp, err } +// CommentOnPullRequest implements the CommentOnPullRequest method of the GitHub interface +func (c *GitHub) CommentOnPullRequest( + ctx context.Context, getByProps *entprops.Properties, comment provifv1.PullRequestCommentInfo, +) (*provifv1.CommentResultMeta, error) { + owner := getByProps.GetProperty(properties.PullPropertyRepoOwner).GetString() + name := getByProps.GetProperty(properties.PullPropertyRepoName).GetString() + prNum := getByProps.GetProperty(properties.PullPropertyNumber).GetInt64() + authorID := getByProps.GetProperty(properties.PullPropertyAuthorID).GetInt64() + + authorizedUser, err := c.delegate.GetMinderUserId(ctx) + if err != nil { + return nil, fmt.Errorf("could not get authenticated user: %w", err) + } + mci, err := c.findPreviousStatusComment(ctx, owner, name, prNum, authorizedUser) + if err != nil { + return nil, fmt.Errorf("could not find previous status comment: %w", err) + } + + mci, err = c.updateOrSubmitComment( + ctx, mci, comment, owner, name, prNum, comment.Commit, authorID, authorizedUser) + if err != nil { + // this should be fatal. In case we can't submit the review, we can't proceed + return nil, fmt.Errorf("could not submit review: %w", err) + } + return mci.ToCommentResultMeta(), nil +} + // CreateReview is a wrapper for the GitHub API to create a review func (c *GitHub) CreateReview( ctx context.Context, owner, repo string, number int, reviewRequest *github.PullRequestReviewRequest, diff --git a/internal/providers/github/mock/github.go b/internal/providers/github/mock/github.go index 6e4de76da3..53760b6e5b 100644 --- a/internal/providers/github/mock/github.go +++ b/internal/providers/github/mock/github.go @@ -2016,3 +2016,173 @@ func (mr *MockOCIMockRecorder) SupportsEntity(entType any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SupportsEntity", reflect.TypeOf((*MockOCI)(nil).SupportsEntity), entType) } + +// MockPullRequestCommenter is a mock of PullRequestCommenter interface. +type MockPullRequestCommenter struct { + ctrl *gomock.Controller + recorder *MockPullRequestCommenterMockRecorder + isgomock struct{} +} + +// MockPullRequestCommenterMockRecorder is the mock recorder for MockPullRequestCommenter. +type MockPullRequestCommenterMockRecorder struct { + mock *MockPullRequestCommenter +} + +// NewMockPullRequestCommenter creates a new mock instance. +func NewMockPullRequestCommenter(ctrl *gomock.Controller) *MockPullRequestCommenter { + mock := &MockPullRequestCommenter{ctrl: ctrl} + mock.recorder = &MockPullRequestCommenterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockPullRequestCommenter) EXPECT() *MockPullRequestCommenterMockRecorder { + return m.recorder +} + +// CanImplement mocks base method. +func (m *MockPullRequestCommenter) CanImplement(trait v10.ProviderType) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CanImplement", trait) + ret0, _ := ret[0].(bool) + return ret0 +} + +// CanImplement indicates an expected call of CanImplement. +func (mr *MockPullRequestCommenterMockRecorder) CanImplement(trait any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CanImplement", reflect.TypeOf((*MockPullRequestCommenter)(nil).CanImplement), trait) +} + +// CommentOnPullRequest mocks base method. +func (m *MockPullRequestCommenter) CommentOnPullRequest(ctx context.Context, getByProps *properties.Properties, comment v11.PullRequestCommentInfo) (*v11.CommentResultMeta, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CommentOnPullRequest", ctx, getByProps, comment) + ret0, _ := ret[0].(*v11.CommentResultMeta) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CommentOnPullRequest indicates an expected call of CommentOnPullRequest. +func (mr *MockPullRequestCommenterMockRecorder) CommentOnPullRequest(ctx, getByProps, comment any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CommentOnPullRequest", reflect.TypeOf((*MockPullRequestCommenter)(nil).CommentOnPullRequest), ctx, getByProps, comment) +} + +// DeregisterEntity mocks base method. +func (m *MockPullRequestCommenter) DeregisterEntity(ctx context.Context, entType v10.Entity, props *properties.Properties) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeregisterEntity", ctx, entType, props) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeregisterEntity indicates an expected call of DeregisterEntity. +func (mr *MockPullRequestCommenterMockRecorder) DeregisterEntity(ctx, entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeregisterEntity", reflect.TypeOf((*MockPullRequestCommenter)(nil).DeregisterEntity), ctx, entType, props) +} + +// FetchAllProperties mocks base method. +func (m *MockPullRequestCommenter) FetchAllProperties(ctx context.Context, getByProps *properties.Properties, entType v10.Entity, cachedProps *properties.Properties) (*properties.Properties, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FetchAllProperties", ctx, getByProps, entType, cachedProps) + ret0, _ := ret[0].(*properties.Properties) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FetchAllProperties indicates an expected call of FetchAllProperties. +func (mr *MockPullRequestCommenterMockRecorder) FetchAllProperties(ctx, getByProps, entType, cachedProps any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchAllProperties", reflect.TypeOf((*MockPullRequestCommenter)(nil).FetchAllProperties), ctx, getByProps, entType, cachedProps) +} + +// FetchProperty mocks base method. +func (m *MockPullRequestCommenter) FetchProperty(ctx context.Context, getByProps *properties.Properties, entType v10.Entity, key string) (*properties.Property, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FetchProperty", ctx, getByProps, entType, key) + ret0, _ := ret[0].(*properties.Property) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FetchProperty indicates an expected call of FetchProperty. +func (mr *MockPullRequestCommenterMockRecorder) FetchProperty(ctx, getByProps, entType, key any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchProperty", reflect.TypeOf((*MockPullRequestCommenter)(nil).FetchProperty), ctx, getByProps, entType, key) +} + +// GetEntityName mocks base method. +func (m *MockPullRequestCommenter) GetEntityName(entType v10.Entity, props *properties.Properties) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetEntityName", entType, props) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetEntityName indicates an expected call of GetEntityName. +func (mr *MockPullRequestCommenterMockRecorder) GetEntityName(entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetEntityName", reflect.TypeOf((*MockPullRequestCommenter)(nil).GetEntityName), entType, props) +} + +// PropertiesToProtoMessage mocks base method. +func (m *MockPullRequestCommenter) PropertiesToProtoMessage(entType v10.Entity, props *properties.Properties) (protoreflect.ProtoMessage, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "PropertiesToProtoMessage", entType, props) + ret0, _ := ret[0].(protoreflect.ProtoMessage) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// PropertiesToProtoMessage indicates an expected call of PropertiesToProtoMessage. +func (mr *MockPullRequestCommenterMockRecorder) PropertiesToProtoMessage(entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PropertiesToProtoMessage", reflect.TypeOf((*MockPullRequestCommenter)(nil).PropertiesToProtoMessage), entType, props) +} + +// RegisterEntity mocks base method. +func (m *MockPullRequestCommenter) RegisterEntity(ctx context.Context, entType v10.Entity, props *properties.Properties) (*properties.Properties, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RegisterEntity", ctx, entType, props) + ret0, _ := ret[0].(*properties.Properties) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// RegisterEntity indicates an expected call of RegisterEntity. +func (mr *MockPullRequestCommenterMockRecorder) RegisterEntity(ctx, entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RegisterEntity", reflect.TypeOf((*MockPullRequestCommenter)(nil).RegisterEntity), ctx, entType, props) +} + +// ReregisterEntity mocks base method. +func (m *MockPullRequestCommenter) ReregisterEntity(ctx context.Context, entType v10.Entity, props *properties.Properties) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ReregisterEntity", ctx, entType, props) + ret0, _ := ret[0].(error) + return ret0 +} + +// ReregisterEntity indicates an expected call of ReregisterEntity. +func (mr *MockPullRequestCommenterMockRecorder) ReregisterEntity(ctx, entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReregisterEntity", reflect.TypeOf((*MockPullRequestCommenter)(nil).ReregisterEntity), ctx, entType, props) +} + +// SupportsEntity mocks base method. +func (m *MockPullRequestCommenter) SupportsEntity(entType v10.Entity) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SupportsEntity", entType) + ret0, _ := ret[0].(bool) + return ret0 +} + +// SupportsEntity indicates an expected call of SupportsEntity. +func (mr *MockPullRequestCommenterMockRecorder) SupportsEntity(entType any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SupportsEntity", reflect.TypeOf((*MockPullRequestCommenter)(nil).SupportsEntity), entType) +} diff --git a/pkg/providers/v1/mock/providers.go b/pkg/providers/v1/mock/providers.go index 1501431d3d..c6a9925855 100644 --- a/pkg/providers/v1/mock/providers.go +++ b/pkg/providers/v1/mock/providers.go @@ -2016,3 +2016,173 @@ func (mr *MockOCIMockRecorder) SupportsEntity(entType any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SupportsEntity", reflect.TypeOf((*MockOCI)(nil).SupportsEntity), entType) } + +// MockPullRequestCommenter is a mock of PullRequestCommenter interface. +type MockPullRequestCommenter struct { + ctrl *gomock.Controller + recorder *MockPullRequestCommenterMockRecorder + isgomock struct{} +} + +// MockPullRequestCommenterMockRecorder is the mock recorder for MockPullRequestCommenter. +type MockPullRequestCommenterMockRecorder struct { + mock *MockPullRequestCommenter +} + +// NewMockPullRequestCommenter creates a new mock instance. +func NewMockPullRequestCommenter(ctrl *gomock.Controller) *MockPullRequestCommenter { + mock := &MockPullRequestCommenter{ctrl: ctrl} + mock.recorder = &MockPullRequestCommenterMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockPullRequestCommenter) EXPECT() *MockPullRequestCommenterMockRecorder { + return m.recorder +} + +// CanImplement mocks base method. +func (m *MockPullRequestCommenter) CanImplement(trait v10.ProviderType) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CanImplement", trait) + ret0, _ := ret[0].(bool) + return ret0 +} + +// CanImplement indicates an expected call of CanImplement. +func (mr *MockPullRequestCommenterMockRecorder) CanImplement(trait any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CanImplement", reflect.TypeOf((*MockPullRequestCommenter)(nil).CanImplement), trait) +} + +// CommentOnPullRequest mocks base method. +func (m *MockPullRequestCommenter) CommentOnPullRequest(ctx context.Context, getByProps *properties.Properties, comment v11.PullRequestCommentInfo) (*v11.CommentResultMeta, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CommentOnPullRequest", ctx, getByProps, comment) + ret0, _ := ret[0].(*v11.CommentResultMeta) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CommentOnPullRequest indicates an expected call of CommentOnPullRequest. +func (mr *MockPullRequestCommenterMockRecorder) CommentOnPullRequest(ctx, getByProps, comment any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CommentOnPullRequest", reflect.TypeOf((*MockPullRequestCommenter)(nil).CommentOnPullRequest), ctx, getByProps, comment) +} + +// DeregisterEntity mocks base method. +func (m *MockPullRequestCommenter) DeregisterEntity(ctx context.Context, entType v10.Entity, props *properties.Properties) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeregisterEntity", ctx, entType, props) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeregisterEntity indicates an expected call of DeregisterEntity. +func (mr *MockPullRequestCommenterMockRecorder) DeregisterEntity(ctx, entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeregisterEntity", reflect.TypeOf((*MockPullRequestCommenter)(nil).DeregisterEntity), ctx, entType, props) +} + +// FetchAllProperties mocks base method. +func (m *MockPullRequestCommenter) FetchAllProperties(ctx context.Context, getByProps *properties.Properties, entType v10.Entity, cachedProps *properties.Properties) (*properties.Properties, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FetchAllProperties", ctx, getByProps, entType, cachedProps) + ret0, _ := ret[0].(*properties.Properties) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FetchAllProperties indicates an expected call of FetchAllProperties. +func (mr *MockPullRequestCommenterMockRecorder) FetchAllProperties(ctx, getByProps, entType, cachedProps any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchAllProperties", reflect.TypeOf((*MockPullRequestCommenter)(nil).FetchAllProperties), ctx, getByProps, entType, cachedProps) +} + +// FetchProperty mocks base method. +func (m *MockPullRequestCommenter) FetchProperty(ctx context.Context, getByProps *properties.Properties, entType v10.Entity, key string) (*properties.Property, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FetchProperty", ctx, getByProps, entType, key) + ret0, _ := ret[0].(*properties.Property) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FetchProperty indicates an expected call of FetchProperty. +func (mr *MockPullRequestCommenterMockRecorder) FetchProperty(ctx, getByProps, entType, key any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchProperty", reflect.TypeOf((*MockPullRequestCommenter)(nil).FetchProperty), ctx, getByProps, entType, key) +} + +// GetEntityName mocks base method. +func (m *MockPullRequestCommenter) GetEntityName(entType v10.Entity, props *properties.Properties) (string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetEntityName", entType, props) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetEntityName indicates an expected call of GetEntityName. +func (mr *MockPullRequestCommenterMockRecorder) GetEntityName(entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetEntityName", reflect.TypeOf((*MockPullRequestCommenter)(nil).GetEntityName), entType, props) +} + +// PropertiesToProtoMessage mocks base method. +func (m *MockPullRequestCommenter) PropertiesToProtoMessage(entType v10.Entity, props *properties.Properties) (protoreflect.ProtoMessage, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "PropertiesToProtoMessage", entType, props) + ret0, _ := ret[0].(protoreflect.ProtoMessage) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// PropertiesToProtoMessage indicates an expected call of PropertiesToProtoMessage. +func (mr *MockPullRequestCommenterMockRecorder) PropertiesToProtoMessage(entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PropertiesToProtoMessage", reflect.TypeOf((*MockPullRequestCommenter)(nil).PropertiesToProtoMessage), entType, props) +} + +// RegisterEntity mocks base method. +func (m *MockPullRequestCommenter) RegisterEntity(ctx context.Context, entType v10.Entity, props *properties.Properties) (*properties.Properties, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RegisterEntity", ctx, entType, props) + ret0, _ := ret[0].(*properties.Properties) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// RegisterEntity indicates an expected call of RegisterEntity. +func (mr *MockPullRequestCommenterMockRecorder) RegisterEntity(ctx, entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RegisterEntity", reflect.TypeOf((*MockPullRequestCommenter)(nil).RegisterEntity), ctx, entType, props) +} + +// ReregisterEntity mocks base method. +func (m *MockPullRequestCommenter) ReregisterEntity(ctx context.Context, entType v10.Entity, props *properties.Properties) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ReregisterEntity", ctx, entType, props) + ret0, _ := ret[0].(error) + return ret0 +} + +// ReregisterEntity indicates an expected call of ReregisterEntity. +func (mr *MockPullRequestCommenterMockRecorder) ReregisterEntity(ctx, entType, props any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReregisterEntity", reflect.TypeOf((*MockPullRequestCommenter)(nil).ReregisterEntity), ctx, entType, props) +} + +// SupportsEntity mocks base method. +func (m *MockPullRequestCommenter) SupportsEntity(entType v10.Entity) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SupportsEntity", entType) + ret0, _ := ret[0].(bool) + return ret0 +} + +// SupportsEntity indicates an expected call of SupportsEntity. +func (mr *MockPullRequestCommenterMockRecorder) SupportsEntity(entType any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SupportsEntity", reflect.TypeOf((*MockPullRequestCommenter)(nil).SupportsEntity), entType) +} diff --git a/pkg/providers/v1/providers.go b/pkg/providers/v1/providers.go index 8d084f5b6e..f9a5af0503 100644 --- a/pkg/providers/v1/providers.go +++ b/pkg/providers/v1/providers.go @@ -225,6 +225,56 @@ type OCI interface { GetAuthenticator() (authn.Authenticator, error) } +// PullRequestCommentType is the type of the pull request comment +type PullRequestCommentType string + +const ( + // PullRequestCommentTypeApprove is the type for an approval + PullRequestCommentTypeApprove PullRequestCommentType = "approve" + // PullRequestCommentTypeRequestChanges is the type for a request for changes + PullRequestCommentTypeRequestChanges PullRequestCommentType = "request_changes" + // PullRequestCommentTypeComment is the type for a regular comment + PullRequestCommentTypeComment PullRequestCommentType = "comment" +) + +// PullRequestCommentInfo is the information for a pull request comment to +// be issued by the provider +type PullRequestCommentInfo struct { + // The commit sha for the pull request + Commit string `json:"commit,omitempty"` + // An optional header for the comment. If aggregating multiple comments, this + // could be used as a header. + Header string `json:"header,omitempty"` + // The comment body + Body string `json:"body,omitempty"` + // The priority of the comment. This is used to determine the order of the comments + // when aggregating multiple comments. Lower values are higher priority. + Priority int `json:"priority,omitempty"` + // The type of the comment + Type PullRequestCommentType `json:"type,omitempty"` + // +} + +// CommentResultMeta is the metadata for the comment result +type CommentResultMeta struct { + ID string `json:"review_id,omitempty"` + SubmittedAt time.Time `json:"submitted_at,omitempty"` + URL string `json:"pull_request_url,omitempty"` +} + +// PullRequestCommenter is the interface for commenting on pull requests +// The provider must implement this interface if it supports commenting on pull requests. +// Providers are assumed to support discovering the pull request by the properties +// as well as discovering the *one* comment they're supposed to work on. +// That is, the provider may issue one comment and aggregate multiple comments into one. +type PullRequestCommenter interface { + Provider + + // CommentOnPullRequest issues comments on a pull request + CommentOnPullRequest( + ctx context.Context, getByProps *properties.Properties, comment PullRequestCommentInfo) (*CommentResultMeta, error) +} + // ParseAndValidate parses the given provider configuration and validates it. func ParseAndValidate(rawConfig json.RawMessage, to any) error { if err := json.Unmarshal(rawConfig, to); err != nil { From 0462f4692104e5cd036b1d09161861579ab5d7d6 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Wed, 11 Dec 2024 15:09:14 +0200 Subject: [PATCH 2/7] Add a shared actions context structure This shared space will allow actions to gather structures or results that will allow it to aggregate and flush. In more solid terms, we'd be able to aggregate PR comments, iterate them, and issue just one big comment. Signed-off-by: Juan Antonio Osorio --- go.mod | 1 + go.sum | 2 + internal/engine/actions/context.go | 89 +++++++++++++++++++++++++ internal/engine/executor.go | 8 ++- internal/engine/interfaces/interface.go | 7 ++ 5 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 internal/engine/actions/context.go diff --git a/go.mod b/go.mod index 85a8b04be8..19158cd9a1 100644 --- a/go.mod +++ b/go.mod @@ -23,6 +23,7 @@ require ( github.com/erikgeiser/promptkit v0.9.0 github.com/evanphx/json-patch/v5 v5.9.0 github.com/fergusstrange/embedded-postgres v1.30.0 + github.com/gammazero/deque v0.2.1 github.com/go-git/go-billy/v5 v5.6.0 github.com/go-git/go-git/v5 v5.12.0 github.com/go-playground/validator/v10 v10.23.0 diff --git a/go.sum b/go.sum index a37d47a0ec..5e1808ddad 100644 --- a/go.sum +++ b/go.sum @@ -382,6 +382,8 @@ github.com/fsnotify/fsnotify v1.8.0 h1:dAwr6QBTBZIkG8roQaJjGof0pp0EeF+tNV7YBP3F/ github.com/fsnotify/fsnotify v1.8.0/go.mod h1:8jBTzvmWwFyi3Pb8djgCCO5IBqzKJ/Jwo8TRcHyHii0= github.com/gabriel-vasile/mimetype v1.4.6 h1:3+PzJTKLkvgjeTbts6msPJt4DixhT4YtFNf1gtGe3zc= github.com/gabriel-vasile/mimetype v1.4.6/go.mod h1:JX1qVKqZd40hUPpAfiNTe0Sne7hdfKSbOqqmkq8GCXc= +github.com/gammazero/deque v0.2.1 h1:qSdsbG6pgp6nL7A0+K/B7s12mcCY/5l5SIUpMOl+dC0= +github.com/gammazero/deque v0.2.1/go.mod h1:LFroj8x4cMYCukHJDbxFCkT+r9AndaJnFMuZDV34tuU= github.com/gkampitakis/ciinfo v0.3.0 h1:gWZlOC2+RYYttL0hBqcoQhM7h1qNkVqvRCV1fOvpAv8= github.com/gkampitakis/ciinfo v0.3.0/go.mod h1:1NIwaOcFChN4fa/B0hEBdAb6npDlFL8Bwx4dfRLRqAo= github.com/gkampitakis/go-diff v1.3.2 h1:Qyn0J9XJSDTgnsgHRdz9Zp24RaJeKMUHg2+PDZZdC4M= diff --git a/internal/engine/actions/context.go b/internal/engine/actions/context.go new file mode 100644 index 0000000000..6cda76678f --- /dev/null +++ b/internal/engine/actions/context.go @@ -0,0 +1,89 @@ +// SPDX-FileCopyrightText: Copyright 2023 The Minder Authors +// SPDX-License-Identifier: Apache-2.0 + +package actions + +import ( + "context" + "errors" + "sync" + + engif "github.com/mindersec/minder/internal/engine/interfaces" +) + +// SharedActionsContextKey is the key used to store the shared actions context +// in the context.Context. +type SharedActionsContextKey struct{} + +// SharedFlusherKey is the key used to store the shared flusher +type SharedFlusherKey string + +type sharedFlusher struct { + flusher engif.AggregatingAction + items []any +} + +// SharedActionsContext is the shared actions context. +type SharedActionsContext struct { + shared map[SharedFlusherKey]*sharedFlusher + mux sync.Mutex +} + +// WithSharedActionsContext returns a new context.Context with the shared actions +// context set. +func WithSharedActionsContext(ctx context.Context) (context.Context, *SharedActionsContext) { + sac := &SharedActionsContext{} + return context.WithValue(ctx, SharedActionsContextKey{}, sac), sac +} + +// GetSharedActionsContext returns the shared actions context from the context.Context. +func GetSharedActionsContext(ctx context.Context) *SharedActionsContext { + ctxVal := ctx.Value(SharedActionsContextKey{}) + if ctxVal == nil { + return nil + } + + v, ok := ctxVal.(*SharedActionsContext) + if !ok { + return nil + } + + return v +} + +// ShareAndRegister adds a shared value to the shared actions context. It may +// also register a flusher if it does not exist. +func (sac *SharedActionsContext) ShareAndRegister(key SharedFlusherKey, flusher engif.AggregatingAction, item any) { + sac.mux.Lock() + defer sac.mux.Unlock() + + f, ok := sac.shared[key] + if !ok { + f = &sharedFlusher{ + flusher: flusher, + items: []any{item}, + } + sac.shared[key] = f + return + } + + f.items = append(f.items, item) +} + +// Flush returns all the shared values and clears the shared actions context. +func (sac *SharedActionsContext) Flush() error { + sac.mux.Lock() + defer sac.mux.Unlock() + var errs []error + + for key, f := range sac.shared { + err := f.flusher.Flush(f.items...) + if err != nil { + errs = append(errs, err) + } + + delete(sac.shared, key) + } + + return errors.Join(errs...) +} diff --git a/internal/engine/executor.go b/internal/engine/executor.go index 52f58e710b..ca2f3d7c64 100644 --- a/internal/engine/executor.go +++ b/internal/engine/executor.go @@ -139,21 +139,23 @@ func (e *executor) EvalEntityEvent(ctx context.Context, inf *entities.EntityInfo return fmt.Errorf("error while retrieving profiles and rule instances: %w", err) } + sacctx, sac := actions.WithSharedActionsContext(ctx) + // For each profile, get the profileEvalStatus first. Then, if the profileEvalStatus is nil // evaluate each rule and store the outcome in the database. If profileEvalStatus is non-nil, // just store it for all rules without evaluation. for _, profile := range profileAggregates { - profileEvalStatus := e.profileEvalStatus(ctx, inf, profile) + profileEvalStatus := e.profileEvalStatus(sacctx, inf, profile) for _, rule := range profile.Rules { - if err := e.evaluateRule(ctx, inf, provider, &profile, &rule, ruleEngineCache, profileEvalStatus); err != nil { + if err := e.evaluateRule(sacctx, inf, provider, &profile, &rule, ruleEngineCache, profileEvalStatus); err != nil { return fmt.Errorf("error evaluating entity event: %w", err) } } } - return nil + return sac.Flush() } func (e *executor) evaluateRule( diff --git a/internal/engine/interfaces/interface.go b/internal/engine/interfaces/interface.go index 77c4c12f53..d2365bd585 100644 --- a/internal/engine/interfaces/interface.go +++ b/internal/engine/interfaces/interface.go @@ -31,6 +31,13 @@ type Action interface { params ActionsParams, metadata *json.RawMessage) (json.RawMessage, error) } +// AggregatingAction is the interface for an action that aggregates multiple +// pieces to form a final action. Normally this will come from the result of a +// `Do` call on an action. +type AggregatingAction interface { + Flush(item ...any) error +} + // ActionCmd is the type that defines what effect an action should have type ActionCmd string From 37b9a317ce29ad69ee3e7cc39f89c2e9e6e384fa Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 17 Dec 2024 19:01:53 +0200 Subject: [PATCH 3/7] Fixes and hooking up new shared actions Signed-off-by: Juan Antonio Osorio --- .../pull_request_comment.go | 74 ++++++++++++++----- .../engine/actions/{ => context}/context.go | 10 ++- internal/engine/executor.go | 5 +- internal/engine/interfaces/interface.go | 2 +- 4 files changed, 65 insertions(+), 26 deletions(-) rename internal/engine/actions/{ => context}/context.go (90%) diff --git a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go index 4b61f8fdf5..c4d1dfd4e4 100644 --- a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go +++ b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go @@ -15,6 +15,7 @@ import ( "google.golang.org/protobuf/reflect/protoreflect" "github.com/mindersec/minder/internal/db" + actionContext "github.com/mindersec/minder/internal/engine/actions/context" enginerr "github.com/mindersec/minder/internal/engine/errors" "github.com/mindersec/minder/internal/engine/interfaces" "github.com/mindersec/minder/internal/entities/properties" @@ -184,27 +185,21 @@ func (_ *Alert) runDoNothing(ctx context.Context, params *paramsPR) (json.RawMes } func (alert *Alert) runDoReview(ctx context.Context, params *paramsPR) (json.RawMessage, error) { - logger := zerolog.Ctx(ctx) - - r, err := alert.commenter.CommentOnPullRequest(ctx, params.props, provifv1.PullRequestCommentInfo{ - // TODO: We should add the header to identify the alert. We could use the - // rule type name. - Commit: params.props.GetProperty(properties.PullRequestCommitSHA).GetString(), - Body: params.Comment, - // TODO: Determine the priority from the rule type severity - }) - if err != nil { - return nil, fmt.Errorf("error creating PR review: %w, %w", err, enginerr.ErrActionFailed) - } - logger.Info().Str("review_id", r.ID).Msg("PR review created") - - // serialize r to json - meta, err := json.Marshal(r) - if err != nil { - return nil, fmt.Errorf("error marshalling PR review metadata: %w", err) + sac := actionContext.GetSharedActionsContext(ctx) + if sac == nil { + return nil, fmt.Errorf("shared actions context not found") } - return meta, nil + sac.ShareAndRegister("pull_request_comment", + newAlertFlusher(params.props, params.props.GetProperty(properties.PullRequestCommitSHA).GetString(), alert.commenter), + &provifv1.PullRequestCommentInfo{ + // TODO: We should add the header to identify the alert. We could use the + // rule type name. + Commit: params.props.GetProperty(properties.PullRequestCommitSHA).GetString(), + Body: params.Comment, + // TODO: Determine the priority from the rule type severity + }) + return json.RawMessage("{}"), nil } // getParamsForSecurityAdvisory extracts the details from the entity @@ -259,3 +254,44 @@ func (alert *Alert) getParamsForPRComment( return result, nil } + +type alertFlusher struct { + props *properties.Properties + commitSha string + commenter provifv1.PullRequestCommenter +} + +func newAlertFlusher(props *properties.Properties, commitSha string, commenter provifv1.PullRequestCommenter) *alertFlusher { + return &alertFlusher{ + props: props, + commitSha: commitSha, + commenter: commenter, + } +} + +func (a *alertFlusher) Flush(ctx context.Context, items ...any) error { + logger := zerolog.Ctx(ctx) + + var aggregatedCommentBody string + + // iterate and aggregate + for _, item := range items { + fp, ok := item.(*provifv1.PullRequestCommentInfo) + if !ok { + logger.Error().Msgf("expected PullRequestCommentInfo, got %T", item) + continue + } + + aggregatedCommentBody += fmt.Sprintf("\n\n%s", fp.Body) + } + + _, err := a.commenter.CommentOnPullRequest(ctx, a.props, provifv1.PullRequestCommentInfo{ + Commit: a.commitSha, + Body: aggregatedCommentBody, + }) + if err != nil { + return fmt.Errorf("error creating PR review: %w", err) + } + + return nil +} diff --git a/internal/engine/actions/context.go b/internal/engine/actions/context/context.go similarity index 90% rename from internal/engine/actions/context.go rename to internal/engine/actions/context/context.go index 6cda76678f..6800a40857 100644 --- a/internal/engine/actions/context.go +++ b/internal/engine/actions/context/context.go @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: Copyright 2023 The Minder Authors // SPDX-License-Identifier: Apache-2.0 -package actions +package context import ( "context" @@ -32,7 +32,9 @@ type SharedActionsContext struct { // WithSharedActionsContext returns a new context.Context with the shared actions // context set. func WithSharedActionsContext(ctx context.Context) (context.Context, *SharedActionsContext) { - sac := &SharedActionsContext{} + sac := &SharedActionsContext{ + shared: make(map[SharedFlusherKey]*sharedFlusher), + } return context.WithValue(ctx, SharedActionsContextKey{}, sac), sac } @@ -71,13 +73,13 @@ func (sac *SharedActionsContext) ShareAndRegister(key SharedFlusherKey, flusher } // Flush returns all the shared values and clears the shared actions context. -func (sac *SharedActionsContext) Flush() error { +func (sac *SharedActionsContext) Flush(ctx context.Context) error { sac.mux.Lock() defer sac.mux.Unlock() var errs []error for key, f := range sac.shared { - err := f.flusher.Flush(f.items...) + err := f.flusher.Flush(ctx, f.items...) if err != nil { errs = append(errs, err) } diff --git a/internal/engine/executor.go b/internal/engine/executor.go index ca2f3d7c64..674717da8a 100644 --- a/internal/engine/executor.go +++ b/internal/engine/executor.go @@ -16,6 +16,7 @@ import ( "github.com/mindersec/minder/internal/db" "github.com/mindersec/minder/internal/engine/actions" "github.com/mindersec/minder/internal/engine/actions/alert" + actionContext "github.com/mindersec/minder/internal/engine/actions/context" "github.com/mindersec/minder/internal/engine/actions/remediate" "github.com/mindersec/minder/internal/engine/entities" evalerrors "github.com/mindersec/minder/internal/engine/errors" @@ -139,7 +140,7 @@ func (e *executor) EvalEntityEvent(ctx context.Context, inf *entities.EntityInfo return fmt.Errorf("error while retrieving profiles and rule instances: %w", err) } - sacctx, sac := actions.WithSharedActionsContext(ctx) + sacctx, sac := actionContext.WithSharedActionsContext(ctx) // For each profile, get the profileEvalStatus first. Then, if the profileEvalStatus is nil // evaluate each rule and store the outcome in the database. If profileEvalStatus is non-nil, @@ -155,7 +156,7 @@ func (e *executor) EvalEntityEvent(ctx context.Context, inf *entities.EntityInfo } } - return sac.Flush() + return sac.Flush(sacctx) } func (e *executor) evaluateRule( diff --git a/internal/engine/interfaces/interface.go b/internal/engine/interfaces/interface.go index d2365bd585..4e94955275 100644 --- a/internal/engine/interfaces/interface.go +++ b/internal/engine/interfaces/interface.go @@ -35,7 +35,7 @@ type Action interface { // pieces to form a final action. Normally this will come from the result of a // `Do` call on an action. type AggregatingAction interface { - Flush(item ...any) error + Flush(ctx context.Context, item ...any) error } // ActionCmd is the type that defines what effect an action should have From 3191dbe568559d36e78df58dff54fd36188d44b2 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Wed, 18 Dec 2024 10:17:09 +0200 Subject: [PATCH 4/7] Add rule type info and formatting Signed-off-by: Juan Antonio Osorio --- internal/engine/actions/alert/alert.go | 10 ++- .../alert/pull_request_comment/flush.go | 60 +++++++++++++++++ .../alert/pull_request_comment/format.go | 31 +++++++++ .../pull_request_comment.go | 66 +++++-------------- .../pull_request_comment_test.go | 2 +- internal/engine/actions/context/context.go | 2 + 6 files changed, 118 insertions(+), 53 deletions(-) create mode 100644 internal/engine/actions/alert/pull_request_comment/flush.go create mode 100644 internal/engine/actions/alert/pull_request_comment/format.go diff --git a/internal/engine/actions/alert/alert.go b/internal/engine/actions/alert/alert.go index c8ac0ddbd5..8bb68643ef 100644 --- a/internal/engine/actions/alert/alert.go +++ b/internal/engine/actions/alert/alert.go @@ -60,8 +60,16 @@ func NewRuleAlert( return noop.NewNoopAlert(ActionType) } return pull_request_comment.NewPullRequestCommentAlert( - ActionType, alertCfg.GetPullRequestComment(), client, setting) + ActionType, alertCfg.GetPullRequestComment(), client, setting, + defaultName(ruletype)) } return nil, fmt.Errorf("unknown alert type: %s", alertCfg.GetType()) } + +func defaultName(ruletype *pb.RuleType) string { + if ruletype.GetDisplayName() != "" { + return ruletype.GetDisplayName() + } + return ruletype.GetName() +} diff --git a/internal/engine/actions/alert/pull_request_comment/flush.go b/internal/engine/actions/alert/pull_request_comment/flush.go new file mode 100644 index 0000000000..6ff7983bec --- /dev/null +++ b/internal/engine/actions/alert/pull_request_comment/flush.go @@ -0,0 +1,60 @@ +// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors +// SPDX-License-Identifier: Apache-2.0 + +package pull_request_comment + +import ( + "context" + "fmt" + + "github.com/rs/zerolog" + + "github.com/mindersec/minder/internal/entities/properties" + provifv1 "github.com/mindersec/minder/pkg/providers/v1" +) + +// alertFlusher aggregates a list of comments and flushes them to the PR +// as a single comment. The idea is that we can aggregate multiple alerts +// into a single comment without needing to flood the PR with multiple comments. +// This is only instantiated once; the first creation is the only one that will +// be used. +type alertFlusher struct { + props *properties.Properties + commitSha string + commenter provifv1.PullRequestCommenter +} + +func newAlertFlusher(props *properties.Properties, commitSha string, commenter provifv1.PullRequestCommenter) *alertFlusher { + return &alertFlusher{ + props: props, + commitSha: commitSha, + commenter: commenter, + } +} + +func (a *alertFlusher) Flush(ctx context.Context, items ...any) error { + logger := zerolog.Ctx(ctx) + + aggregatedCommentBody := paragraph(title1("Minder Alerts")) + + // iterate and aggregate + for _, item := range items { + fp, ok := item.(*provifv1.PullRequestCommentInfo) + if !ok { + logger.Error().Msgf("expected PullRequestCommentInfo, got %T", item) + continue + } + + aggregatedCommentBody += paragraph(alert(fp.Header, fp.Body)) + } + + _, err := a.commenter.CommentOnPullRequest(ctx, a.props, provifv1.PullRequestCommentInfo{ + Commit: a.commitSha, + Body: aggregatedCommentBody, + }) + if err != nil { + return fmt.Errorf("error creating PR review: %w", err) + } + + return nil +} diff --git a/internal/engine/actions/alert/pull_request_comment/format.go b/internal/engine/actions/alert/pull_request_comment/format.go new file mode 100644 index 0000000000..8c3ed9c519 --- /dev/null +++ b/internal/engine/actions/alert/pull_request_comment/format.go @@ -0,0 +1,31 @@ +// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors +// SPDX-License-Identifier: Apache-2.0 + +package pull_request_comment + +import "fmt" + +func formatTitle(displayName string) string { + return fmt.Sprintf("Rule '%s' Alert", displayName) +} + +// Formats the comment for a single alert as markdown +func alert(title, body string) string { + return fmt.Sprintf("%s\n\n%s", title2(title), body) +} + +func paragraph(text string) string { + return fmt.Sprintf("%s\n\n", text) +} + +func title1(title string) string { + return fmt.Sprintf("# %s", title) +} + +func title2(title string) string { + return fmt.Sprintf("## %s", title) +} + +func separator() string { + return "---" +} diff --git a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go index c4d1dfd4e4..cf59d699aa 100644 --- a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go +++ b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go @@ -36,10 +36,11 @@ const ( // Alert is the structure backing the noop alert type Alert struct { - actionType interfaces.ActionType - commenter provifv1.PullRequestCommenter - reviewCfg *pb.RuleType_Definition_Alert_AlertTypePRComment - setting models.ActionOpt + actionType interfaces.ActionType + commenter provifv1.PullRequestCommenter + reviewCfg *pb.RuleType_Definition_Alert_AlertTypePRComment + setting models.ActionOpt + displayName string } // PrCommentTemplateParams is the parameters for the PR comment templates @@ -52,6 +53,7 @@ type PrCommentTemplateParams struct { } type paramsPR struct { + Title string Comment string props *properties.Properties Metadata *provifv1.CommentResultMeta @@ -64,16 +66,19 @@ func NewPullRequestCommentAlert( reviewCfg *pb.RuleType_Definition_Alert_AlertTypePRComment, gh provifv1.PullRequestCommenter, setting models.ActionOpt, +// The display name for the alert + displayName string, ) (*Alert, error) { if actionType == "" { return nil, fmt.Errorf("action type cannot be empty") } return &Alert{ - actionType: actionType, - commenter: gh, - reviewCfg: reviewCfg, - setting: setting, + actionType: actionType, + commenter: gh, + reviewCfg: reviewCfg, + setting: setting, + displayName: displayName, }, nil } @@ -193,8 +198,7 @@ func (alert *Alert) runDoReview(ctx context.Context, params *paramsPR) (json.Raw sac.ShareAndRegister("pull_request_comment", newAlertFlusher(params.props, params.props.GetProperty(properties.PullRequestCommitSHA).GetString(), alert.commenter), &provifv1.PullRequestCommentInfo{ - // TODO: We should add the header to identify the alert. We could use the - // rule type name. + Header: params.Title, Commit: params.props.GetProperty(properties.PullRequestCommitSHA).GetString(), Body: params.Comment, // TODO: Determine the priority from the rule type severity @@ -238,6 +242,7 @@ func (alert *Alert) getParamsForPRComment( return nil, fmt.Errorf("cannot execute title template: %w", err) } + result.Title = formatTitle(alert.displayName) result.Comment = comment // Unmarshal the existing alert metadata, if any @@ -254,44 +259,3 @@ func (alert *Alert) getParamsForPRComment( return result, nil } - -type alertFlusher struct { - props *properties.Properties - commitSha string - commenter provifv1.PullRequestCommenter -} - -func newAlertFlusher(props *properties.Properties, commitSha string, commenter provifv1.PullRequestCommenter) *alertFlusher { - return &alertFlusher{ - props: props, - commitSha: commitSha, - commenter: commenter, - } -} - -func (a *alertFlusher) Flush(ctx context.Context, items ...any) error { - logger := zerolog.Ctx(ctx) - - var aggregatedCommentBody string - - // iterate and aggregate - for _, item := range items { - fp, ok := item.(*provifv1.PullRequestCommentInfo) - if !ok { - logger.Error().Msgf("expected PullRequestCommentInfo, got %T", item) - continue - } - - aggregatedCommentBody += fmt.Sprintf("\n\n%s", fp.Body) - } - - _, err := a.commenter.CommentOnPullRequest(ctx, a.props, provifv1.PullRequestCommentInfo{ - Commit: a.commitSha, - Body: aggregatedCommentBody, - }) - if err != nil { - return fmt.Errorf("error creating PR review: %w", err) - } - - return nil -} diff --git a/internal/engine/actions/alert/pull_request_comment/pull_request_comment_test.go b/internal/engine/actions/alert/pull_request_comment/pull_request_comment_test.go index bf3aed30d0..a37fb13d7f 100644 --- a/internal/engine/actions/alert/pull_request_comment/pull_request_comment_test.go +++ b/internal/engine/actions/alert/pull_request_comment/pull_request_comment_test.go @@ -130,7 +130,7 @@ func TestPullRequestCommentAlert(t *testing.T) { tt.mockSetup(mockClient) prCommentAlert, err := NewPullRequestCommentAlert( - tt.actionType, &prCommentCfg, mockClient, models.ActionOptOn) + tt.actionType, &prCommentCfg, mockClient, models.ActionOptOn, "Title") require.NoError(t, err) require.NotNil(t, prCommentAlert) diff --git a/internal/engine/actions/context/context.go b/internal/engine/actions/context/context.go index 6800a40857..d9c9ad7da7 100644 --- a/internal/engine/actions/context/context.go +++ b/internal/engine/actions/context/context.go @@ -1,6 +1,8 @@ // SPDX-FileCopyrightText: Copyright 2023 The Minder Authors // SPDX-License-Identifier: Apache-2.0 +// Package context contains the shared actions context for sharing +// data between actions. package context import ( From 9bc375dc3479e70b6ba3c0b35ff776b036cac1e6 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Wed, 18 Dec 2024 10:32:11 +0200 Subject: [PATCH 5/7] Add todo comment Signed-off-by: Juan Antonio Osorio --- internal/providers/github/commenter.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/providers/github/commenter.go b/internal/providers/github/commenter.go index e9f9c847da..8ab6894251 100644 --- a/internal/providers/github/commenter.go +++ b/internal/providers/github/commenter.go @@ -180,6 +180,9 @@ func (c *GitHub) doReview( logger := zerolog.Ctx(ctx) // if the previous review was on the same commit, keep it + // TODO: This should only apply if the profile has not changed. That is, we need + // to detect if there was a change in the profile and if there was, we should + // dismiss the previous review. if mci.ContentSha == commitSha { logger.Debug(). Int64("review-id", mci.ReviewID). From 3d82226d1d874abd30b2e4df0bb274bcb845562b Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Wed, 18 Dec 2024 10:55:39 +0200 Subject: [PATCH 6/7] More formatting Signed-off-by: Juan Antonio Osorio --- .../alert/pull_request_comment/flush.go | 38 ++++++++++++++----- .../alert/pull_request_comment/format.go | 8 ++-- .../pull_request_comment.go | 5 +++ internal/providers/github/commenter.go | 13 ++----- 4 files changed, 41 insertions(+), 23 deletions(-) diff --git a/internal/engine/actions/alert/pull_request_comment/flush.go b/internal/engine/actions/alert/pull_request_comment/flush.go index 6ff7983bec..a8bee92018 100644 --- a/internal/engine/actions/alert/pull_request_comment/flush.go +++ b/internal/engine/actions/alert/pull_request_comment/flush.go @@ -6,6 +6,8 @@ package pull_request_comment import ( "context" "fmt" + "slices" + "strings" "github.com/rs/zerolog" @@ -33,9 +35,29 @@ func newAlertFlusher(props *properties.Properties, commitSha string, commenter p } func (a *alertFlusher) Flush(ctx context.Context, items ...any) error { + title := title1("Minder Alerts") + + aggregatedAlerts := getAlerts(ctx, items...) + + _, err := a.commenter.CommentOnPullRequest(ctx, a.props, provifv1.PullRequestCommentInfo{ + Commit: a.commitSha, + Body: fmt.Sprintf("%s\n\n%s", title, aggregatedAlerts), + }) + if err != nil { + return fmt.Errorf("error creating PR review: %w", err) + } + + return nil +} + +func getAlerts(ctx context.Context, items ...any) string { logger := zerolog.Ctx(ctx) - aggregatedCommentBody := paragraph(title1("Minder Alerts")) + if len(items) == 0 { + return "Minder found no issues." + } + + var alerts []string // iterate and aggregate for _, item := range items { @@ -45,16 +67,12 @@ func (a *alertFlusher) Flush(ctx context.Context, items ...any) error { continue } - aggregatedCommentBody += paragraph(alert(fp.Header, fp.Body)) + alerts = append(alerts, alert(fp.Header, fp.Body)) } - _, err := a.commenter.CommentOnPullRequest(ctx, a.props, provifv1.PullRequestCommentInfo{ - Commit: a.commitSha, - Body: aggregatedCommentBody, - }) - if err != nil { - return fmt.Errorf("error creating PR review: %w", err) - } + // Ensure predictable ordering + // TODO: This should be sorted by severity + slices.Sort(alerts) - return nil + return strings.Join(alerts, spacing()) } diff --git a/internal/engine/actions/alert/pull_request_comment/format.go b/internal/engine/actions/alert/pull_request_comment/format.go index 8c3ed9c519..ad738e6c0f 100644 --- a/internal/engine/actions/alert/pull_request_comment/format.go +++ b/internal/engine/actions/alert/pull_request_comment/format.go @@ -14,10 +14,6 @@ func alert(title, body string) string { return fmt.Sprintf("%s\n\n%s", title2(title), body) } -func paragraph(text string) string { - return fmt.Sprintf("%s\n\n", text) -} - func title1(title string) string { return fmt.Sprintf("# %s", title) } @@ -26,6 +22,10 @@ func title2(title string) string { return fmt.Sprintf("## %s", title) } +func spacing() string { + return "\n\n" +} + func separator() string { return "---" } diff --git a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go index cf59d699aa..ee657d6127 100644 --- a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go +++ b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go @@ -195,6 +195,11 @@ func (alert *Alert) runDoReview(ctx context.Context, params *paramsPR) (json.Raw return nil, fmt.Errorf("shared actions context not found") } + // This was a successful result, so we don't need to alert + if params.Comment == "" { + return json.RawMessage("{}"), nil + } + sac.ShareAndRegister("pull_request_comment", newAlertFlusher(params.props, params.props.GetProperty(properties.PullRequestCommitSHA).GetString(), alert.commenter), &provifv1.PullRequestCommentInfo{ diff --git a/internal/providers/github/commenter.go b/internal/providers/github/commenter.go index 8ab6894251..f70f56d894 100644 --- a/internal/providers/github/commenter.go +++ b/internal/providers/github/commenter.go @@ -159,17 +159,12 @@ func (c *GitHub) updateOrSubmitComment(ctx context.Context, mci magicCommentInfo return mci, fmt.Errorf("could not render comment: %w", err) } - // TODO: Should we keep this? - if mci.ContentSha != comment.Commit { - err := c.UpdateIssueComment(ctx, owner, repoName, mci.ExistingCommentID, body) - if err != nil { - return mci, fmt.Errorf("failed to update minder status report comment: %w", err) - } - - mci.SubmittedAt = time.Now() - return mci, nil + err = c.UpdateIssueComment(ctx, owner, repoName, mci.ExistingCommentID, body) + if err != nil { + return mci, fmt.Errorf("failed to update minder status report comment: %w", err) } + mci.SubmittedAt = time.Now() return mci, nil } From fb5d46d0609575a9302249553eb65a7d3e197280 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Wed, 18 Dec 2024 13:11:53 +0200 Subject: [PATCH 7/7] Don't alert on successful rules. Signed-off-by: Juan Antonio Osorio --- .../pull_request_comment.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go index ee657d6127..8ab609dd1b 100644 --- a/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go +++ b/internal/engine/actions/alert/pull_request_comment/pull_request_comment.go @@ -53,11 +53,12 @@ type PrCommentTemplateParams struct { } type paramsPR struct { - Title string - Comment string - props *properties.Properties - Metadata *provifv1.CommentResultMeta - prevStatus *db.ListRuleEvaluationsByProfileIdRow + Title string + Comment string + props *properties.Properties + Metadata *provifv1.CommentResultMeta + prevStatus *db.ListRuleEvaluationsByProfileIdRow + shouldAlert bool } // NewPullRequestCommentAlert creates a new pull request comment alert action @@ -196,7 +197,7 @@ func (alert *Alert) runDoReview(ctx context.Context, params *paramsPR) (json.Raw } // This was a successful result, so we don't need to alert - if params.Comment == "" { + if !params.shouldAlert { return json.RawMessage("{}"), nil } @@ -225,8 +226,9 @@ func (alert *Alert) getParamsForPRComment( } result := ¶msPR{ - prevStatus: params.GetEvalStatusFromDb(), - props: props, + prevStatus: params.GetEvalStatusFromDb(), + props: props, + shouldAlert: params.GetEvalErr() != nil, } commentTmpl, err := util.NewSafeHTMLTemplate(&alert.reviewCfg.ReviewMessage, "message")