From ab4bc76d003f25b75c5d74c93558f05bc91fa936 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Tue, 16 May 2023 18:38:45 -0400 Subject: [PATCH] Improve review dismissal behavior (#577) Existing dismissal logic compares all reviews on the PR to the set of reviews that might satisfy any rule. This does not work well when policies combine rules that invalidate on push with rules that do not. In this situation, reviews are not dismissed when new commits are pushed because they might satisfy the rules that do not invalidate. Instead, a user's review is dismissed when they submit a second review. This is because we only consider the most recent review from a user, which means their previous review doesn't count for any rules and is eligible to be dismissed. We chose this approach to try to dismiss reviews that did not match required comment patterns, but ended up removing that before merging the feature. This commit switches back to the original approach, which is to collect specific reviews to dismiss, rather than trying to infer what is safe to dismiss. This should be more reliable and is easier to reason about. --- policy/approval/approve.go | 134 ++++++++++++++--------- policy/approval/approve_test.go | 34 +++--- policy/common/result.go | 13 ++- server/handler/eval_context_dismissal.go | 90 ++++++--------- 4 files changed, 141 insertions(+), 130 deletions(-) diff --git a/policy/approval/approve.go b/policy/approval/approve.go index 76575097..499725ea 100644 --- a/policy/approval/approve.go +++ b/policy/approval/approve.go @@ -132,21 +132,22 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res } res.PredicateResults = predicateResults - allowedCandidates, err := r.FilteredCandidates(ctx, prctx) + candidates, dismissals, 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) + approved, approvers, err := r.IsApproved(ctx, prctx, candidates) if err != nil { res.Error = errors.Wrap(err, "failed to compute approval status") return } - res.AllowedCandidates = allowedCandidates + res.Approvers = approvers + res.Dismissals = dismissals + res.StatusDescription = r.statusDescription(approved, approvers, candidates) - res.StatusDescription = msg if approved { res.Status = common.StatusApproved } else { @@ -177,12 +178,12 @@ func (r *Rule) getReviewRequestRule() *common.ReviewRequestRule { } } -func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, string, error) { +func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) (bool, []*common.Candidate, error) { log := zerolog.Ctx(ctx) if r.Requires.Count <= 0 { log.Debug().Msg("rule requires no approvals") - return true, "No approval required", nil + return true, nil, nil } log.Debug().Msgf("found %d candidates for approval", len(candidates)) @@ -202,7 +203,7 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates [] if !r.Options.AllowContributor && !r.Options.AllowNonAuthorContributor { commits, err := r.filteredCommits(ctx, prctx) if err != nil { - return false, "", err + return false, nil, err } for _, c := range commits { @@ -215,7 +216,7 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates [] } // filter real approvers using banned status and required membership - var approvers []string + var approvers []*common.Candidate for _, c := range candidates { if banned[c.User] { log.Debug().Str("user", c.User).Msg("rejecting approval by banned user") @@ -224,113 +225,118 @@ func (r *Rule) IsApproved(ctx context.Context, prctx pull.Context, candidates [] isApprover, err := r.Requires.Actors.IsActor(ctx, prctx, c.User) if err != nil { - return false, "", errors.Wrap(err, "failed to check candidate status") + return false, nil, errors.Wrap(err, "failed to check candidate status") } if !isApprover { - log.Debug().Str("user", c.User).Msg("ignoring approval by non-whitelisted user") + log.Debug().Str("user", c.User).Msg("ignoring approval by non-required user") continue } - approvers = append(approvers, c.User) + approvers = append(approvers, c) } log.Debug().Msgf("found %d/%d required approvers", len(approvers), r.Requires.Count) - remaining := r.Requires.Count - len(approvers) - - if remaining <= 0 { - msg := fmt.Sprintf("Approved by %s", strings.Join(approvers, ", ")) - return true, msg, nil - } - - if len(candidates) > 0 && len(approvers) == 0 { - msg := fmt.Sprintf("%d/%d approvals required. Ignored %s from disqualified users", - len(approvers), - r.Requires.Count, - numberOfApprovals(len(candidates))) - return false, msg, nil - } - - msg := fmt.Sprintf("%d/%d approvals required", len(approvers), r.Requires.Count) - return false, msg, nil + return len(approvers) >= r.Requires.Count, approvers, nil } -func (r *Rule) FilteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, error) { +// FilteredCandidates returns the potential approval candidates and any +// candidates that should be dimissed due to rule options. +func (r *Rule) FilteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, []*common.Dismissal, error) { + candidates, err := r.Options.GetMethods().Candidates(ctx, prctx) if err != nil { - return nil, errors.Wrap(err, "failed to get approval candidates") + return nil, nil, errors.Wrap(err, "failed to get approval candidates") } sort.Stable(common.CandidatesByCreationTime(candidates)) + var editDismissals []*common.Dismissal if r.Options.IgnoreEditedComments { - candidates, err = r.filterEditedCandidates(ctx, prctx, candidates) + candidates, editDismissals, err = r.filterEditedCandidates(ctx, prctx, candidates) if err != nil { - return nil, err + return nil, nil, err } } + var pushDismissals []*common.Dismissal if r.Options.InvalidateOnPush { - candidates, err = r.filterInvalidCandidates(ctx, prctx, candidates) + candidates, pushDismissals, err = r.filterInvalidCandidates(ctx, prctx, candidates) if err != nil { - return nil, err + return nil, nil, err } } - return candidates, nil + var dismissals []*common.Dismissal + dismissals = append(dismissals, editDismissals...) + dismissals = append(dismissals, pushDismissals...) + + return candidates, dismissals, nil } -func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, error) { +func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, []*common.Dismissal, error) { log := zerolog.Ctx(ctx) if !r.Options.IgnoreEditedComments { - return candidates, nil + return candidates, nil, nil } - var allowedCandidates []*common.Candidate - for _, candidate := range candidates { - if candidate.LastEditedAt.IsZero() { - allowedCandidates = append(allowedCandidates, candidate) + var allowed []*common.Candidate + var dismissed []*common.Dismissal + for _, c := range candidates { + if c.LastEditedAt.IsZero() { + allowed = append(allowed, c) + } else { + dismissed = append(dismissed, &common.Dismissal{ + Candidate: c, + Reason: "Comment was edited", + }) } } - log.Debug().Msgf("discarded %d candidates with edited comments", len(candidates)-len(allowedCandidates)) + log.Debug().Msgf("discarded %d candidates with edited comments", len(dismissed)) - return allowedCandidates, nil + return allowed, dismissed, nil } -func (r *Rule) filterInvalidCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, error) { +func (r *Rule) filterInvalidCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, []*common.Dismissal, error) { log := zerolog.Ctx(ctx) if !r.Options.InvalidateOnPush { - return candidates, nil + return candidates, nil, nil } commits, err := r.filteredCommits(ctx, prctx) if err != nil { - return nil, err + return nil, nil, err } if len(commits) == 0 { - return candidates, nil + return candidates, nil, nil } last := findLastPushed(commits) if last == nil { - return nil, errors.New("no commit contained a push date") + return nil, nil, errors.New("no commit contained a push date") } - var allowedCandidates []*common.Candidate - for _, candidate := range candidates { - if candidate.CreatedAt.After(*last.PushedAt) { - allowedCandidates = append(allowedCandidates, candidate) + var allowed []*common.Candidate + var dismissed []*common.Dismissal + for _, c := range candidates { + if c.CreatedAt.After(*last.PushedAt) { + allowed = append(allowed, c) + } else { + dismissed = append(dismissed, &common.Dismissal{ + Candidate: c, + Reason: fmt.Sprintf("Invalidated by push of %.7s", last.SHA), + }) } } log.Debug().Msgf("discarded %d candidates invalidated by push of %s at %s", - len(candidates)-len(allowedCandidates), + len(dismissed), last.SHA, last.PushedAt.Format(time.RFC3339)) - return allowedCandidates, nil + return allowed, dismissed, nil } func (r *Rule) filteredCommits(ctx context.Context, prctx pull.Context) ([]*pull.Commit, error) { @@ -369,6 +375,26 @@ func (r *Rule) filteredCommits(ctx context.Context, prctx pull.Context) ([]*pull return filtered, nil } +func (r *Rule) statusDescription(approved bool, approvers, candidates []*common.Candidate) string { + if approved { + if len(approvers) == 0 { + return "No approval required" + } + + var names []string + for _, c := range approvers { + names = append(names, c.User) + } + return fmt.Sprintf("Approved by %s", strings.Join(names, ", ")) + } + + desc := fmt.Sprintf("%d/%d required approvals", len(approvers), r.Requires.Count) + if disqualified := len(candidates) - len(approvers); disqualified > 0 { + desc += fmt.Sprintf(". Ignored %s from disqualified users", numberOfApprovals(disqualified)) + } + return desc +} + func isUpdateMerge(commits []*pull.Commit, c *pull.Commit) bool { // must be a simple merge commit (exactly 2 parents) if len(c.Parents) != 2 { diff --git a/policy/approval/approve_test.go b/policy/approval/approve_test.go index 91e8fb56..b484d1af 100644 --- a/policy/approval/approve_test.go +++ b/policy/approval/approve_test.go @@ -141,25 +141,27 @@ func TestIsApproved(t *testing.T) { } assertApproved := func(t *testing.T, prctx pull.Context, r *Rule, expected string) { - allowedCandidates, err := r.FilteredCandidates(ctx, prctx) + allowedCandidates, _, err := r.FilteredCandidates(ctx, prctx) require.NoError(t, err) - approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates) + approved, approvers, err := r.IsApproved(ctx, prctx, allowedCandidates) require.NoError(t, err) if assert.True(t, approved, "pull request was not approved") { + msg := r.statusDescription(approved, approvers, allowedCandidates) assert.Equal(t, expected, msg) } } assertPending := func(t *testing.T, prctx pull.Context, r *Rule, expected string) { - allowedCandidates, err := r.FilteredCandidates(ctx, prctx) + allowedCandidates, _, err := r.FilteredCandidates(ctx, prctx) require.NoError(t, err) - approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates) + approved, approvers, err := r.IsApproved(ctx, prctx, allowedCandidates) require.NoError(t, err) if assert.False(t, approved, "pull request was incorrectly approved") { + msg := r.statusDescription(approved, approvers, allowedCandidates) assert.Equal(t, expected, msg) } } @@ -177,7 +179,7 @@ func TestIsApproved(t *testing.T) { Count: 1, }, } - assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users") }) t.Run("authorCannotApprove", func(t *testing.T) { @@ -315,7 +317,7 @@ func TestIsApproved(t *testing.T) { }, }, } - assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users") }) t.Run("specificOrgApproves", func(t *testing.T) { @@ -338,7 +340,7 @@ func TestIsApproved(t *testing.T) { }, }, } - assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users") }) t.Run("specificOrgsOrUserApproves", func(t *testing.T) { @@ -377,7 +379,7 @@ func TestIsApproved(t *testing.T) { assertApproved(t, prctx, r, "Approved by comment-approver") r.Options.InvalidateOnPush = true - assertPending(t, prctx, r, "0/1 approvals required. Ignored 6 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 6 approvals from disqualified users") }) t.Run("invalidateReviewOnPush", func(t *testing.T) { @@ -402,7 +404,7 @@ func TestIsApproved(t *testing.T) { assertApproved(t, prctx, r, "Approved by review-approver") r.Options.InvalidateOnPush = true - assertPending(t, prctx, r, "0/1 approvals required. Ignored 1 approval from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 1 approval from disqualified users") }) t.Run("ignoreUpdateMergeAfterReview", func(t *testing.T) { @@ -429,7 +431,7 @@ func TestIsApproved(t *testing.T) { InvalidateOnPush: true, }, } - assertPending(t, prctx, r, "0/1 approvals required. Ignored 6 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 6 approvals from disqualified users") r.Options.IgnoreUpdateMerges = true assertApproved(t, prctx, r, "Approved by comment-approver") @@ -461,7 +463,7 @@ func TestIsApproved(t *testing.T) { }, }, } - assertPending(t, prctx, r, "0/1 approvals required. Ignored 8 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 8 approvals from disqualified users") r.Options.IgnoreUpdateMerges = true assertApproved(t, prctx, r, "Approved by merge-committer") @@ -483,7 +485,7 @@ func TestIsApproved(t *testing.T) { }, }, } - assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users") r.Options.IgnoreCommitsBy = common.Actors{ Users: []string{"comment-approver"}, @@ -507,7 +509,7 @@ func TestIsApproved(t *testing.T) { }, }, } - assertPending(t, prctx, r, "0/1 approvals required. Ignored 7 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 7 approvals from disqualified users") }) t.Run("ignoreCommitsInvalidateOnPush", func(t *testing.T) { @@ -554,7 +556,7 @@ func TestIsApproved(t *testing.T) { r.Options.IgnoreEditedComments = true - assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 5 approvals from disqualified users") }) t.Run("ignoreEditedComments", func(t *testing.T) { @@ -573,7 +575,7 @@ func TestIsApproved(t *testing.T) { r.Options.IgnoreEditedComments = true - assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 5 approvals from disqualified users") }) t.Run("ignoreEditedCommentsWithBodyPattern", func(t *testing.T) { @@ -600,7 +602,7 @@ func TestIsApproved(t *testing.T) { r.Options.IgnoreEditedComments = true - assertPending(t, prctx, r, "0/1 approvals required. Ignored 5 approvals from disqualified users") + assertPending(t, prctx, r, "0/1 required approvals. Ignored 5 approvals from disqualified users") }) } diff --git a/policy/common/result.go b/policy/common/result.go index fef0d90a..3d3ab509 100644 --- a/policy/common/result.go +++ b/policy/common/result.go @@ -74,8 +74,19 @@ type Result struct { Requires Requires Methods *Methods + // Approvers contains the candidates that satisfied the rule. + Approvers []*Candidate + + // Dismissals contains candidates that should be discarded because they + // cannot satisfy any future evaluations. + Dismissals []*Dismissal + ReviewRequestRule *ReviewRequestRule - AllowedCandidates []*Candidate Children []*Result } + +type Dismissal struct { + Candidate *Candidate + Reason string +} diff --git a/server/handler/eval_context_dismissal.go b/server/handler/eval_context_dismissal.go index 5375510e..466b51f7 100644 --- a/server/handler/eval_context_dismissal.go +++ b/server/handler/eval_context_dismissal.go @@ -16,11 +16,8 @@ package handler import ( "context" - "fmt" - "time" "github.com/palantir/policy-bot/policy/common" - "github.com/palantir/policy-bot/pull" "github.com/pkg/errors" "github.com/rs/zerolog" "github.com/shurcooL/githubv4" @@ -29,86 +26,61 @@ import ( func (ec *EvalContext) dismissStaleReviewsForResult(ctx context.Context, result common.Result) error { logger := zerolog.Ctx(ctx) - var allowedCandidates []*common.Candidate + approvers := findAllApprovers(&result) - results := findResultsWithAllowedCandidates(&result) - for _, res := range results { - for _, candidate := range res.AllowedCandidates { - if candidate.Type != common.ReviewCandidate { - continue - } - allowedCandidates = append(allowedCandidates, candidate) - } - } - - reviews, err := ec.PullContext.Reviews() - if err != nil { - return err - } - - for _, r := range reviews { - if r.State != pull.ReviewApproved { + alreadyDismissed := make(map[string]bool) + for _, d := range findAllDismissals(&result) { + // Only dismiss stale reviews for now (ignore comments) + if d.Candidate.Type != common.ReviewCandidate { continue } - - if reviewIsAllowed(r, allowedCandidates) { + // Only dismiss reviews from users who are not currently approvers + if approvers[d.Candidate.User] { continue } - - reason := reasonForDismissedReview(r) - if reason == "" { + // Only dismiss reviews once if they're dismissed by multiple rules + if alreadyDismissed[d.Candidate.ReviewID] { continue } - message := fmt.Sprintf("Dismissed because the approval %s", reason) - logger.Info().Msgf("Dismissing stale review %s because it %s", r.ID, reason) - if err := dismissPullRequestReview(ctx, ec.V4Client, r.ID, message); err != nil { + logger.Info().Str("reason", d.Reason).Msgf("Dismissing stale review %s", d.Candidate.ReviewID) + if err := dismissPullRequestReview(ctx, ec.V4Client, d.Candidate.ReviewID, d.Reason); err != nil { return err } + alreadyDismissed[d.Candidate.ReviewID] = true } return nil } -func findResultsWithAllowedCandidates(result *common.Result) []*common.Result { - var results []*common.Result - for _, c := range result.Children { - results = append(results, findResultsWithAllowedCandidates(c)...) - } +func findAllDismissals(result *common.Result) []*common.Dismissal { + var dismissals []*common.Dismissal - if len(result.Children) == 0 && len(result.AllowedCandidates) > 0 && result.Error == nil { - results = append(results, result) + if len(result.Children) == 0 && result.Error == nil { + dismissals = append(dismissals, result.Dismissals...) + } + for _, c := range result.Children { + dismissals = append(dismissals, findAllDismissals(c)...) } - return results + return dismissals } -func reviewIsAllowed(review *pull.Review, allowedCandidates []*common.Candidate) bool { - for _, candidate := range allowedCandidates { - if review.ID == candidate.ReviewID { - return true - } - } - return false -} +func findAllApprovers(result *common.Result) map[string]bool { + approvers := make(map[string]bool) -// We already know that these are discarded review candidates for 1 of 2 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. -// -// 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 reasonForDismissedReview(review *pull.Review) string { - if !review.LastEditedAt.IsZero() { - return "was edited" + if len(result.Children) == 0 && result.Error == nil { + for _, a := range result.Approvers { + approvers[a.User] = true + } } - - if review.CreatedAt.Before(time.Now().Add(-5 * time.Second)) { - return "was invalidated by another commit" + for _, c := range result.Children { + for u := range findAllApprovers(c) { + approvers[u] = true + } } - return "" + return approvers } func dismissPullRequestReview(ctx context.Context, v4client *githubv4.Client, reviewID string, message string) error {