From 76edab8ef019e30c96432a235b839d334f519001 Mon Sep 17 00:00:00 2001 From: "jamirkulov (Jahon)" Date: Wed, 24 Sep 2025 17:59:38 -0700 Subject: [PATCH] Adding dismiss review type --- cmd/pr-bot/main.go | 2 +- github/api.go | 1 + github/github.go | 13 +++++ github/mock_api.go | 18 +++++++ go.mod | 2 +- opa/types/result.go | 3 ++ pullrequest/event_handler.go | 2 + pullrequest/review/dedup_reviewer.go | 4 ++ pullrequest/review/mock_reviewer.go | 18 +++++++ pullrequest/review/mutex_reviewer.go | 9 ++++ .../review/pre_cond_validation_reviewer.go | 4 ++ pullrequest/review/rate_limited_reviewer.go | 4 ++ pullrequest/review/reviewer.go | 48 +++++++++++++++++-- pullrequest/review/reviewer_test.go | 6 +-- 14 files changed, 125 insertions(+), 9 deletions(-) diff --git a/cmd/pr-bot/main.go b/cmd/pr-bot/main.go index 1d97d9fb..50ea333f 100644 --- a/cmd/pr-bot/main.go +++ b/cmd/pr-bot/main.go @@ -171,7 +171,7 @@ func downloadOCIArtifacts(svc *prbot.Service, cfg *prbot.Config) { func setupReviewer(svc *prbot.Service, cfg *prbot.Config, api gh.API) review.Reviewer { log.Info().Msg("Setting up reviewer") // mutex -> dedup -> precond -> rate limited -> reviewer - base := review.NewReviewer(api, svc.Metrics) + base := review.NewReviewer(api, svc.Metrics, cfg.GHE.ServiceAccount) throttler := setupThrottlers(svc, cfg) rateLimited := review.NewRateLimitedReviewer(base, api, throttler) precond := review.NewPreCondValidationReviewer(rateLimited) diff --git a/github/api.go b/github/api.go index 1273a784..c4eaacd2 100644 --- a/github/api.go +++ b/github/api.go @@ -45,6 +45,7 @@ More details on PR bot policy evaluation: link type API interface { ListReviews(ctx context.Context, id id.PR) ([]*github.PullRequestReview, error) AddReview(ctx context.Context, id id.PR, summary, event string) error + DismissReview(ctx context.Context, id id.PR, reviewID int64, message string) error EnableAutoMerge(ctx context.Context, id id.PR, method githubv4.PullRequestMergeMethod) error IssueComment(ctx context.Context, id id.PR, comment string) error IssueCommentForError(ctx context.Context, id id.PR, err pe.APIError) error diff --git a/github/github.go b/github/github.go index bb5154be..af91d96e 100644 --- a/github/github.go +++ b/github/github.go @@ -207,6 +207,19 @@ func (gh *githubDao) AddReview(ctx context.Context, id id.PR, summary, event str return nil } +// DismissReview implements API +func (gh *githubDao) DismissReview(ctx context.Context, id id.PR, reviewID int64, message string) error { + _, resp, err := gh.v3.PullRequests.DismissReview(ctx, id.Owner, id.Repo, id.Number, reviewID, + &github.PullRequestReviewDismissalRequest{ + Message: &message, + }) + if err != nil { + return err + } + gh.emitTokenExpiration(ctx, resp) + return nil +} + // IssueCommentForError implements Dao func (gh *githubDao) IssueCommentForError(ctx context.Context, id id.PR, apiError pe.APIError) error { b, err := json.MarshalIndent(apiError, "", " ") diff --git a/github/mock_api.go b/github/mock_api.go index 067de228..25eae37b 100644 --- a/github/mock_api.go +++ b/github/mock_api.go @@ -77,6 +77,24 @@ func (_c *MockAPI_AddReview_Call) RunAndReturn(run func(context.Context, id.PR, return _c } +// DismissReview provides a mock function with given fields: ctx, _a1, reviewID, message +func (_m *MockAPI) DismissReview(ctx context.Context, _a1 id.PR, reviewID int64, message string) error { + ret := _m.Called(ctx, _a1, reviewID, message) + + if len(ret) == 0 { + panic("no return value specified for DismissReview") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, id.PR, int64, string) error); ok { + r0 = rf(ctx, _a1, reviewID, message) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // EnableAutoMerge provides a mock function with given fields: ctx, _a1, method func (_m *MockAPI) EnableAutoMerge(ctx context.Context, _a1 id.PR, method githubv4.PullRequestMergeMethod) error { ret := _m.Called(ctx, _a1, method) diff --git a/go.mod b/go.mod index b4c0b7fb..0da0f4d1 100644 --- a/go.mod +++ b/go.mod @@ -19,6 +19,7 @@ require ( github.com/go-chi/httplog v0.3.2 github.com/go-chi/render v1.0.3 github.com/google/go-github/v50 v50.2.0 + github.com/google/uuid v1.6.0 github.com/ilyakaznacheev/cleanenv v1.5.0 github.com/jonboulle/clockwork v0.4.0 github.com/mennanov/limiters v1.2.2 @@ -75,7 +76,6 @@ require ( github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/protobuf v1.5.4 // indirect github.com/google/go-querystring v1.1.0 // indirect - github.com/google/uuid v1.6.0 // indirect github.com/gorilla/mux v1.8.1 // indirect github.com/hashicorp/consul/api v1.26.1 // indirect github.com/hashicorp/errwrap v1.1.0 // indirect diff --git a/opa/types/result.go b/opa/types/result.go index 977b0903..432e067f 100644 --- a/opa/types/result.go +++ b/opa/types/result.go @@ -17,6 +17,7 @@ const ( Approve Comment RequestChanges + Dismiss ) var ( @@ -25,6 +26,7 @@ var ( 1: "APPROVE", 2: "COMMENT", 3: "REQUEST_CHANGES", + 4: "DISMISS", } reviewTypeValues = reverseMap(reviewTypeNames) @@ -33,6 +35,7 @@ var ( 1: "APPROVED", 2: "COMMENTED", 3: "CHANGES_REQUESTED", + 4: "DISMISSED", } reviewStateValues = reverseMap(reviewStateNames) ) diff --git a/pullrequest/event_handler.go b/pullrequest/event_handler.go index 4bd2f355..bf4fe92f 100644 --- a/pullrequest/event_handler.go +++ b/pullrequest/event_handler.go @@ -94,6 +94,8 @@ func (eh *eventHandler) EvalAndReview(ctx context.Context, id id.PR, ghe input.G return eh.reviewer.RequestChanges(ctx, id, opaResult.Review.Body) case types.Comment: return eh.reviewer.Comment(ctx, id, opaResult.Review.Body) + case types.Dismiss: + return eh.reviewer.Dismiss(ctx, id, opaResult.Review.Body) default: oplog.Info().Msg("skipping review") } diff --git a/pullrequest/review/dedup_reviewer.go b/pullrequest/review/dedup_reviewer.go index ff667061..d54852e7 100644 --- a/pullrequest/review/dedup_reviewer.go +++ b/pullrequest/review/dedup_reviewer.go @@ -89,3 +89,7 @@ func (d *DedupReviewer) checkForReview(reviews []*github.PullRequestReview, revi } return false } + +func (d *DedupReviewer) Dismiss(ctx context.Context, id id.PR, body string) error { + return d.delegate.Dismiss(ctx, id, body) +} diff --git a/pullrequest/review/mock_reviewer.go b/pullrequest/review/mock_reviewer.go index f451f76d..2f5b7d9b 100644 --- a/pullrequest/review/mock_reviewer.go +++ b/pullrequest/review/mock_reviewer.go @@ -137,6 +137,24 @@ func (_m *MockReviewer) RequestChanges(ctx context.Context, _a1 id.PR, body stri return r0 } +// Dismiss provides a mock function with given fields: ctx, _a1, body +func (_m *MockReviewer) Dismiss(ctx context.Context, _a1 id.PR, body string) error { + ret := _m.Called(ctx, _a1, body) + + if len(ret) == 0 { + panic("no return value specified for Dismiss") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, id.PR, string) error); ok { + r0 = rf(ctx, _a1, body) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // MockReviewer_RequestChanges_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RequestChanges' type MockReviewer_RequestChanges_Call struct { *mock.Call diff --git a/pullrequest/review/mutex_reviewer.go b/pullrequest/review/mutex_reviewer.go index 29a1e56f..9a1dca6f 100644 --- a/pullrequest/review/mutex_reviewer.go +++ b/pullrequest/review/mutex_reviewer.go @@ -94,3 +94,12 @@ func NewMutexReviewer(delegate Reviewer, locker Locker) Reviewer { locker: locker, } } + +func (r *mutexReviewer) Dismiss(ctx context.Context, id id.PR, body string) error { + lock, err := r.acquireLock(ctx, id) + if err != nil { + return err + } + defer r.releaseLock(ctx, lock, id) + return r.delegate.Dismiss(ctx, id, body) +} diff --git a/pullrequest/review/pre_cond_validation_reviewer.go b/pullrequest/review/pre_cond_validation_reviewer.go index 6de7d936..3d716edf 100644 --- a/pullrequest/review/pre_cond_validation_reviewer.go +++ b/pullrequest/review/pre_cond_validation_reviewer.go @@ -43,3 +43,7 @@ func NewPreCondValidationReviewer(delegate Reviewer) Reviewer { delegate: delegate, } } + +func (p *preCondValidationReviewer) Dismiss(ctx context.Context, id id.PR, body string) error { + return p.delegate.Dismiss(ctx, id, body) +} diff --git a/pullrequest/review/rate_limited_reviewer.go b/pullrequest/review/rate_limited_reviewer.go index d718e1b6..e3e68023 100644 --- a/pullrequest/review/rate_limited_reviewer.go +++ b/pullrequest/review/rate_limited_reviewer.go @@ -52,3 +52,7 @@ func NewRateLimitedReviewer(delegate Reviewer, api gh.API, throttler rate.Thrott throttler: throttler, } } + +func (r *rateLimitedReviewer) Dismiss(ctx context.Context, id id.PR, body string) error { + return r.delegate.Dismiss(ctx, id, body) +} diff --git a/pullrequest/review/reviewer.go b/pullrequest/review/reviewer.go index ae0fbc5e..9e357544 100644 --- a/pullrequest/review/reviewer.go +++ b/pullrequest/review/reviewer.go @@ -28,11 +28,13 @@ type Reviewer interface { Approve(ctx context.Context, id id.PR, body string, opts ApproveOptions) error RequestChanges(ctx context.Context, id id.PR, body string) error Comment(ctx context.Context, id id.PR, body string) error + Dismiss(ctx context.Context, id id.PR, body string) error } type reviewer struct { - api gh.API - metrics metrics.Emitter + api gh.API + metrics metrics.Emitter + serviceAccount string } // Approve implements Reviewer. @@ -94,8 +96,46 @@ func (r *reviewer) RequestChanges(ctx context.Context, id id.PR, body string) er return nil } -func NewReviewer(dao gh.API, metrics metrics.Emitter) Reviewer { - return &reviewer{api: dao, metrics: metrics} +// Dismiss implements Reviewer. +func (r *reviewer) Dismiss(ctx context.Context, id id.PR, body string) error { + oplog := httplog.LogEntry(ctx) + + // Get existing reviews to find CHANGES_REQUESTED reviews to dismiss + reviews, err := r.api.ListReviews(ctx, id) + if err != nil { + oplog.Err(err).Msgf("error listing reviews for PR %v", id.URL) + ae := pe.ServiceFault(ctx, "Error listing reviews for PR", err) + return ae + } + + // Find and dismiss only CHANGES_REQUESTED reviews from the configured service account + dismissed := false + for _, review := range reviews { + if review.GetState() == "CHANGES_REQUESTED" && + review.GetUser().GetLogin() == r.serviceAccount { + err = r.api.DismissReview(ctx, id, review.GetID(), body) + if err != nil { + oplog.Err(err).Msgf("error dismissing review %d for PR %v", review.GetID(), id.URL) + ae := pe.ServiceFault(ctx, "Error dismissing review", err) + return ae + } + dismissed = true + oplog.Info().Msgf("dismissed review %d from %s for PR %v", review.GetID(), r.serviceAccount, id.URL) + } + } + + if dismissed { + tags := append(id.ToTags(), fmt.Sprintf("reviewType:%s", "dismiss")) + r.metrics.EmitDist(ctx, "reviewedPRs", 1.0, tags) + r.metrics.EmitDist(ctx, "dismissedPRs", 1.0, tags) + oplog.Info().Msgf("reviewed PR reviewType:dismiss") + } + + return nil +} + +func NewReviewer(dao gh.API, metrics metrics.Emitter, serviceAccount string) Reviewer { + return &reviewer{api: dao, metrics: metrics, serviceAccount: serviceAccount} } func (r *reviewer) handleAutoMergeError(ctx context.Context, id id.PR, err error) error { diff --git a/pullrequest/review/reviewer_test.go b/pullrequest/review/reviewer_test.go index 3b508e3e..f2ca3ae5 100644 --- a/pullrequest/review/reviewer_test.go +++ b/pullrequest/review/reviewer_test.go @@ -165,7 +165,7 @@ func Test_reviewer_Approve(t *testing.T) { t.Run(tt.name, func(t *testing.T) { mockAPI := gh.NewMockAPI(t) metrics := metrics.NewNoopEmitter() - r := review.NewReviewer(mockAPI, metrics) + r := review.NewReviewer(mockAPI, metrics, "test-service-account") tt.args.setExpectations(mockAPI) if err := r.Approve(ctx, tt.args.id, tt.args.body, review.ApproveOptions{ MergeMethod: tt.args.mergeMethod, @@ -220,7 +220,7 @@ func Test_reviewer_Comment(t *testing.T) { t.Run(tt.name, func(t *testing.T) { mockAPI := gh.NewMockAPI(t) metrics := metrics.NewNoopEmitter() - r := review.NewReviewer(mockAPI, metrics) + r := review.NewReviewer(mockAPI, metrics, "test-service-account") tt.args.setExpectations(mockAPI) if err := r.Comment(ctx, tt.args.id, tt.args.body); (err != nil) != tt.wantErr { t.Errorf("reviewer.Comment() error = %v, wantErr %v", err, tt.wantErr) @@ -273,7 +273,7 @@ func Test_reviewer_RequestChanges(t *testing.T) { t.Run(tt.name, func(t *testing.T) { mockAPI := gh.NewMockAPI(t) metrics := metrics.NewNoopEmitter() - r := review.NewReviewer(mockAPI, metrics) + r := review.NewReviewer(mockAPI, metrics, "test-service-account") tt.args.setExpectations(mockAPI) if err := r.RequestChanges(ctx, tt.args.id, tt.args.body); (err != nil) != tt.wantErr { t.Errorf("reviewer.RequestChanges() error = %v, wantErr %v", err, tt.wantErr)