diff --git a/policy/approval/approve.go b/policy/approval/approve.go index f9acbb22..dd957bf5 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -133,12 +133,21 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res } } res.PredicateResults = predicateResults - approved, msg, err := r.IsApproved(ctx, prctx) + + allowedCandidates, err := r.FilteredCandidates(ctx, prctx) + if err != nil { + res.Error = errors.Wrap(err, "failed to filter candidates") + return + } + + approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates) if err != nil { res.Error = errors.Wrap(err, "failed to compute approval status") return } + res.AllowedCandidates = allowedCandidates + res.StatusDescription = msg if approved { res.Status = common.StatusApproved @@ -146,6 +155,7 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res res.Status = common.StatusPending res.ReviewRequestRule = r.getReviewRequestRule() } + return } @@ -169,7 +179,7 @@ func (r *Rule) getReviewRequestRule() *common.ReviewRequestRule { } } -func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string, error) { +func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, string, error) { log := zerolog.Ctx(ctx) if r.Requires.Count <= 0 { @@ -177,11 +187,6 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string return true, "No approval required", nil } - candidates, err := r.filteredCandidates(ctx, prctx) - if err != nil { - return false, "", err - } - log.Debug().Msgf("found %d candidates for approval", len(candidates)) // collect users "banned" by approval options @@ -251,7 +256,7 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context) (bool, string return false, msg, nil } -func (r *Rule) filteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, error) { +func (r *Rule) FilteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, error) { candidates, err := r.Options.GetMethods().Candidates(ctx, prctx) if err != nil { return nil, errors.Wrap(err, "failed to get approval candidates") @@ -285,15 +290,12 @@ func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, c var allowedCandidates []*common.Candidate for _, candidate := range candidates { - if r.Options.IgnoreEditedComments { - if candidate.LastEditedAt.IsZero() { - allowedCandidates = append(allowedCandidates, candidate) - } + if candidate.LastEditedAt.IsZero() { + allowedCandidates = append(allowedCandidates, candidate) } } - log.Debug().Msgf("discarded %d candidates with edited comments", - len(candidates)-len(allowedCandidates)) + log.Debug().Msgf("discarded %d candidates with edited comments", len(candidates)-len(allowedCandidates)) return allowedCandidates, nil } diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go index 743a5884..01243615 100644 --- a/policy/approval/approve_test.go +++ b/policy/approval/approve_test.go @@ -135,7 +135,10 @@ func TestIsApproved(t *testing.T) { } assertApproved := func(t *testing.T, prctx pull.Context, r *Rule, expected string) { - approved, msg, err := r.IsApproved(ctx, prctx) + allowedCandidates, err := r.FilteredCandidates(ctx, prctx) + require.NoError(t, err) + + approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates) require.NoError(t, err) if assert.True(t, approved, "pull request was not approved") { @@ -144,7 +147,10 @@ func TestIsApproved(t *testing.T) { } assertPending := func(t *testing.T, prctx pull.Context, r *Rule, expected string) { - approved, msg, err := r.IsApproved(ctx, prctx) + allowedCandidates, err := r.FilteredCandidates(ctx, prctx) + require.NoError(t, err) + + approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates) require.NoError(t, err) if assert.False(t, approved, "pull request was incorrectly approved") { diff --git a/policy/common/methods.go b/policy/common/methods.go index 6b4787fa..5abbd712 100644 --- a/policy/common/methods.go +++ b/policy/common/methods.go @@ -34,7 +34,16 @@ type Methods struct { GithubReviewState pull.ReviewState `yaml:"-" json:"-"` } +type CandidateType string + +const ( + ReviewCandidate CandidateType = "review" + CommentCandidate CandidateType = "comment" +) + type Candidate struct { + Type CandidateType + ReviewID string User string CreatedAt time.Time LastEditedAt time.Time @@ -64,6 +73,7 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid for _, c := range comments { if m.CommentMatches(c.Body) { candidates = append(candidates, &Candidate{ + Type: CommentCandidate, User: c.Author, CreatedAt: c.CreatedAt, LastEditedAt: c.LastEditedAt, @@ -83,6 +93,8 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid if len(m.GithubReviewCommentPatterns) > 0 { if m.githubReviewCommentMatches(r.Body) { candidates = append(candidates, &Candidate{ + Type: ReviewCandidate, + ReviewID: r.ID, User: r.Author, CreatedAt: r.CreatedAt, LastEditedAt: r.LastEditedAt, @@ -90,6 +102,8 @@ func (m *Methods) Candidates(ctx context.Context, prctx pull.Context) ([]*Candid } } else { candidates = append(candidates, &Candidate{ + Type: ReviewCandidate, + ReviewID: r.ID, User: r.Author, CreatedAt: r.CreatedAt, LastEditedAt: r.LastEditedAt, diff --git a/policy/common/result.go b/policy/common/result.go index 91c43aad..99ed3d2d 100644 --- a/policy/common/result.go +++ b/policy/common/result.go @@ -69,6 +69,7 @@ type Result struct { Requires Actors ReviewRequestRule *ReviewRequestRule + AllowedCandidates []*Candidate Children []*Result } diff --git a/pull/context.go b/pull/context.go index 79e5bede..f3322cd5 100644 --- a/pull/context.go +++ b/pull/context.go @@ -198,6 +198,7 @@ const ( ) type Review struct { + ID string CreatedAt time.Time LastEditedAt time.Time Author string diff --git a/pull/github.go b/pull/github.go index ecbaba46..a68a2180 100644 --- a/pull/github.go +++ b/pull/github.go @@ -978,6 +978,7 @@ func (pi v4PageInfo) UpdateCursor(vars map[string]interface{}, name string) bool } type v4PullRequestReview struct { + ID string Author v4Actor State string Body string @@ -1004,6 +1005,7 @@ func (r *v4PullRequestReview) ToReview() *Review { } return &Review{ + ID: r.ID, CreatedAt: r.SubmittedAt, LastEditedAt: r.LastEditedAt, Author: r.Author.GetV3Login(), diff --git a/server/handler/base.go b/server/handler/base.go index 6eb544fc..6ab2491c 100644 --- a/server/handler/base.go +++ b/server/handler/base.go @@ -31,6 +31,7 @@ import ( "github.com/palantir/policy-bot/pull" "github.com/pkg/errors" "github.com/rs/zerolog" + "github.com/shurcooL/githubv4" ) const ( @@ -186,7 +187,17 @@ func (b *Base) Evaluate(ctx context.Context, installationID int64, trigger commo return err } - return b.RequestReviewsForResult(ctx, prctx, client, trigger, result) + err = b.RequestReviewsForResult(ctx, prctx, client, trigger, result) + if err != nil { + return err + } + + err = b.dismissStaleReviewsForResult(ctx, prctx, v4client, result) + if err != nil { + return err + } + + return nil } func (b *Base) ValidateFetchedConfig(ctx context.Context, prctx pull.Context, client *github.Client, fc FetchedConfig, trigger common.Trigger) (common.Evaluator, error) { @@ -391,6 +402,110 @@ func selectionToReviewersRequest(s reviewer.Selection) github.ReviewersRequest { return req } +func (b *Base) findResultsWithAllowedCandidates(result *common.Result) []*common.Result { + var results []*common.Result + for _, c := range result.Children { + results = append(results, b.findResultsWithAllowedCandidates(c)...) + } + + if len(result.Children) == 0 && len(result.AllowedCandidates) > 0 && result.Error == nil { + results = append(results, result) + } + + return results +} + +func (b *Base) reviewIsAllowed(review *pull.Review, allowedCandidates []*common.Candidate) bool { + for _, candidate := range allowedCandidates { + if review.ID == candidate.ReviewID { + return true + } + } + + return false +} + +// We already know that these are discarded review candidates for one of three reasons +// so first we check for edited and then we check to see if its a review thats at least +// 5 seconds old and we know that it was invalidated by a new commit, and then finally +// if it was missing a github review comment pattern that was required. +// +// This is brittle and may need refactoring in future versions because it assumes the bot +// will take less than 5 seconds to respond, but thought that having a dismissal reason +// was valuable. +func (b *Base) reasonForDismissedReview(review *pull.Review) string { + if !review.LastEditedAt.IsZero() { + return "was edited" + } + + if review.CreatedAt.Before(time.Now().Add(-5 * time.Second)) { + return "was invalidated by another commit" + } + + return "didn't include a valid comment pattern" +} + +func (b *Base) dismissStaleReviewsForResult(ctx context.Context, prctx pull.Context, v4client *githubv4.Client, result common.Result) error { + logger := zerolog.Ctx(ctx) + + var allowedCandidates []*common.Candidate + + results := b.findResultsWithAllowedCandidates(&result) + for _, res := range results { + for _, candidate := range res.AllowedCandidates { + if candidate.Type != common.ReviewCandidate { + continue + } + allowedCandidates = append(allowedCandidates, candidate) + } + } + + reviews, err := prctx.Reviews() + if err != nil { + return err + } + + for _, r := range reviews { + if r.State != pull.ReviewApproved { + continue + } + + if b.reviewIsAllowed(r, allowedCandidates) { + continue + } + + reason := b.reasonForDismissedReview(r) + message := fmt.Sprintf("Dismissed because the approval %s", reason) + logger.Info().Msgf("Dismissing stale review %s because it %s", r.ID, reason) + err := b.dismissPullRequestReview(ctx, v4client, r.ID, message) + if err != nil { + logger.Err(err).Msg("Failed to dismiss stale review") + return err + } + } + + return nil +} + +func (b *Base) dismissPullRequestReview(ctx context.Context, v4client *githubv4.Client, reviewID string, message string) error { + var m struct { + DismissPullRequestReview struct { + ClientMutationID *githubv4.String + } `graphql:"dismissPullRequestReview(input: $input)"` + } + + input := githubv4.DismissPullRequestReviewInput{ + PullRequestReviewID: githubv4.String(reviewID), + Message: githubv4.String(message), + } + + if err := v4client.Mutate(ctx, &m, input, nil); err != nil { + return errors.Wrapf(err, "failed to dismiss pull request review %s", reviewID) + } + + return nil +} + func setStringFromEnv(key, prefix string, value *string) bool { if v, ok := os.LookupEnv(prefix + key); ok { *value = v