Dismiss stale reviews per rule #463

devinburnette · 2022-08-30T21:06:42Z

Closes #239

After we migrated over to policy bot, I had a couple people ask why the dismiss feature didn't work the same. If you check the dismiss reviews option in the branch protection rules on the branch it will dismiss all reviews, but what if you have a policy bot policy that only cares to invalidate a push on one rule but not another. Without the dismiss option in github selected, you still get a green check on the approval comment even if policy bot treats it as invalidated so that causes a little bit of confusion.

After reading through the issues I found this and wanted to see what it could look like. In theory, I think something like this could work. I ran into something weird while testing it however, Github's dismiss review API is returning a 403 for me even though their docs indicate that this is something that a github app should have permission to do. So maybe there's just something else up, I need to write some tests to see what's happening, but before I go too far down that rabbit hole wanted to surface this to see if it would be a feature worth entertaining. I didn't see much discussion on the original issue and that was a while ago.

/cc @bluekeyes

server/handler/pull_request.go

bluekeyes · 2022-08-30T22:07:35Z

Thanks starting an implementation of this feature! I don't have any objections to dismissing stale reviews, so I think it's worthwhile to continue testing and iterating here. This hasn't been a common complaint or request internally at Palantir, so the original issue never made it to the top of our priorities, but it feels like one of those features where everyone will appreciate it once it exists.

For the 403 error, I wonder if by default apps can only dismiss their own reviews and have to be granted some additional permission to dismiss other reviews. The docs for the dismissal API warn:

Note: To dismiss a pull request review on a protected branch, you must be a repository administrator or be included in the list of people or teams who can dismiss pull request reviews.

devinburnette · 2022-08-31T19:12:02Z

@bluekeyes yea i've tried both removing the branch protection outright and granting admin write to no avail. i get the same error no matter what I do, so maybe its something else i'm doing wrong?

403 Branch protections do not permit dismissing this review []

the fact that those [] are empty there at the end, that must mean something.

bluekeyes · 2022-08-31T21:02:01Z

Not sure if you've tried this yet, but for weird API issues, I usually resort to using the Ruby script from Authenticating as a GitHub App to generate a JWT, exchange it for installation token, and then test requests with curl. That way you can see raw errors from GitHub and try things like preview headers quickly.

We don't use any GraphQL mutations in Policy Bot yet, but it looks like there's a dismissPullRequestReview mutation you could try (probably using the curl method.) If it also doesn't it work, it might give a more helpful error.

devinburnette · 2022-09-01T02:36:31Z

@bluekeyes yea looks like the REST API just doesn't work, but the GraphQL one does, so I'll try that route and see how things look.

devinburnette · 2022-09-06T19:51:52Z

@bluekeyes i think i'm ready for a first glance at this.. this is working well in my testing, before i get too deep at writing tests, i could use your expertise on where you think the bulk of the logic should live?

I went with combining the pull request and pull request review handlers so that these functions could be triggered as a result of an event which invalidates an approval candidate because it didn't feel right putting in inside the filtering logic, so I exported that function and refactored it a little to support the use case.

bluekeyes · 2022-09-06T23:28:54Z

Yeah, I agree that implementing dismissal as part of the filtering logic would feel wrong, but I don't think combing the pull_request and pull_request_review handlers is the best alternative.

There might be a flaw in this I haven't seen yet, but my initial thought is to structure this similar to review requests. Because we're only dismissing reviews that Policy Bot would ignore anyway, it shouldn't matter if we do it before or after evaluating the policy. That means we could add a new field to the common.Result struct that contains the dismissal information and have a handlers.Base function that acts on that when it is set.

If possible, I'd like to keep the flow Event -> Evaluation -> Result -> Side Effects, where the "evaluation" step can be treated like a single function without exposing internal logic like the concept of candidates and filtering.

I see you just marked this as ready for review too, so I'll try to take a closer look this week.

devinburnette · 2022-09-08T19:02:46Z

@bluekeyes i've updated my approach to utilize common.Result but I'm still finding myself having to pass information through the candidate abstraction, it's probably fine, but wanted to see how you felt about it.

additionally, I opened a case with Github about the REST API not working as described by the documentation and the findings are that content: write and pull requests: write are required via REST and only pull requests: write is required via GraphQL not sure how they will fix that up, but I'm doubtful they will add the content requirement to GraphQL since that would be a breaking change to whoever is currently using it that way and the documentation for contents seems to have nothing to do with PR review dismissal.

bluekeyes

Thanks for following up with GitHub on the permissions. I agree that it makes sense to keep using the GraphQL mutation (unless/until GitHub fixes permissions for the REST API), rather than trying to give Policy Bot elevated permissions that it otherwise doesn't need.

I had a few suggestions about the structure here and I think we can probably minimize the changes to common.Candidate (adding only the review ID and the type) with a bit of refactoring. But what you have looks like a good start.

bluekeyes · 2022-09-09T20:53:24Z

policy/approval/approve.go

 	return
 }

+func (r *Rule) getDiscardedReviews(ctx context.Context, prctx pull.Context) ([]*common.DiscardedReview, error) {
+	_, discarded, err := r.filteredCandidates(ctx, prctx)


The caching in pull.Context means this shouldn't have to make any API requests while recomputing candidates, but it would be nice if we could only do this once anyway.

This would be a larger refactor (mostly because of the tests), so maybe something for a follow-up PR (I could do this if you are not interested.) I think having IsApproved take a list of candidates instead of compute the candidates would be the easiest. IsApproved could also return the list of discarded candidates, but that feels like a larger break in abstraction.

I think this change would also make the tests for IsApproved better because everything wouldn't have to be carefully encoded in to the single pull.Context we currently use.

bluekeyes · 2022-09-09T20:54:13Z

policy/approval/approve.go

+
+	var discardedReviews []*common.DiscardedReview
+
+	if len(discarded) != 0 {


nit: should be no need for this check, the range statement will exit immediately if the list is empty

bluekeyes · 2022-09-09T20:56:25Z

policy/approval/approve.go

@@ -251,29 +281,40 @@ 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, []*common.Candidate, error) {


nit: let's name these return values so that it's clear which is which

Suggested change

func (r *Rule) filteredCandidates(ctx context.Context, prctx pull.Context) ([]*common.Candidate, []*common.Candidate, error) {

func (r *Rule) filteredCandidates(ctx context.Context, prctx pull.Context) (allow []*common.Candidate, discard []*common.Candidate, err error) {

bluekeyes · 2022-09-09T21:06:45Z

pull/context.go

@@ -92,6 +92,9 @@ type Context interface {
 	// implementation dependent.
 	Reviews() ([]*Review, error)

+	// DismissPullRequestReview dismisses a review on a Pull Request.
+	DismissPullRequestReview(reviewID string, message string) error


This should be defined in the handler package where it is used. The pull.Context type is used for reading information about pull requests and by convention, all mutations happen directly in the handlers (assigning reviewers, posting status checks, etc.)

bluekeyes · 2022-09-09T21:09:20Z

server/handler/base.go

@@ -373,7 +383,7 @@ func (b *Base) requestReviews(ctx context.Context, prctx pull.Context, client *g
 	return nil
 }

-func selectionToReviewersRequest(s reviewer.Selection) github.ReviewersRequest {
+func (b *Base) selectionToReviewersRequest(s reviewer.Selection) github.ReviewersRequest {


What was the reason for adding the *Base receiver on this function?

bluekeyes · 2022-09-09T21:20:10Z

server/handler/base.go

+	}
+
+	for _, d := range result.DiscardedReviews {
+		if d.State != pull.ReviewApproved {


I think passing the review state through to here is unnecessary. When computing the list of candidates, reviews are pre-filtered to only have the correct state (approval in this case.) A non-approved review would not be a candidate for granting approval.

ahh right, thanks.

bluekeyes · 2022-09-09T21:29:52Z

server/handler/base.go

+		var reasons []string
+		for _, reason := range d.Reason {
+			reasons = append(reasons, string(reason))
+		}
+		because := strings.Join(reasons, " and ")
+		message := fmt.Sprintf("%s was dismissed by policy-bot because the approval was %s", result.Name, because)


I'm wondering if it's worth simplifying this. Having a reason is helpful, but if a review is dismissed for multiple reasons, I'm not sure it adds much to know all of them instead of the first. What do you think?

Also, once we're accounting for multiple rules dismissing the same review, the result.Name part will become kind of arbitrary and I don't think will add much. Rule names can also be quite long, which might hit character limits in the GitHub API.

yea, i think you're right, putting user input like the rule names in the message is asking for character limit problems, we should probably just keep this simple with just the reason and i think you're right, it doesn't really provide much value to include both.

bluekeyes · 2022-09-09T21:30:55Z

policy/common/methods.go

+	CreatedAt        time.Time
+	LastEditedAt     time.Time
+	DiscardedBecause []DiscardReason
+	ReviewState      pull.ReviewState


As mentioned below, I'm not sure ReviewState is necessary

bluekeyes · 2022-09-09T21:31:50Z

policy/common/methods.go

-	CreatedAt    time.Time
-	LastEditedAt time.Time
+	Type             CandidateType
+	ID               string


nit: I'd call this ReviewID to be clear it is not set for other candidate types

bluekeyes · 2022-09-09T21:45:26Z

policy/approval/approve.go

+	var discardedCandidates []*common.Candidate
+
+	for _, c := range candidates {
+		if len(c.DiscardedBecause) == 0 {


While this works, I'm wondering if it would be cleaner to pass this distinction down to filterInvalidCandidates and filterEditedCandidates, having them return two lists (and not setting a reason). This function could then aggregate the results, attach the reason based on which function discarded the candidate, possibly returning a []*common.DismissedReview instead of a []*common.Candidate`.

Ideally, I'd like to keep valid and discarded candidates separate, rather than having some functions that return a mixed list (where you have to check the reason) and some that return separate lists (where it's expected that one list sets the reason and the other doesn't.) This could also allow removing the DiscardedBecause field from common.Candidate.

this is what i had at first, and changed it, will change it back.

devinburnette · 2022-09-11T21:19:30Z

@bluekeyes i've taken a stab at implementing the feedback, and while testing this common.Result approach after implementing your suggestions, I think I've found a minor hurdle in review dismissal post evaluation for edited reviews. Basically when a review with an approval comment is edited and it no longer matches the required pattern, then it is no longer considered a candidate so the github dismissal never happens.

Since the pattern no longer matches, i'm sure it would be very easy for a user to tell why their PR isn't approved, but the green check mark still being there isn't the most ideal. What do you think?

bluekeyes

Thanks for all the updates, the code looks good aside from one thing with de-duplication.

For the problem with edited reviews, one idea (that unfortunately undoes some of the work you did) is to invert what we pass through to the result. Instead of including the reviews to dismiss (the discarded candidates), we include the reviews that might be valid (the allowed candidates.) Then, the dismissal function lists all of the reviews on the PR (pull.Context caches these, so it should be the same list we used earlier) and dismisses any "approved" reviews that are not allowed candidates.

bluekeyes · 2022-09-16T20:30:46Z

server/handler/base.go

@@ -391,6 +402,73 @@ func selectionToReviewersRequest(s reviewer.Selection) github.ReviewersRequest {
 	return req
 }

+func (b *Base) dedupDiscardedReviews(discardedReviews []*common.DiscardedReview) []*common.DiscardedReview {


Seeing as a DiscardedReview is an ID and a reason, I wonder if we could avoid the de-duplication function by passing around a map[id]reason instead of a slice.

Even if not, I recommend implementing this function by building up a map instead of using the dual loop. I don't think the caller cares whether it iterates over a slice or a map.

bluekeyes · 2022-09-16T20:32:35Z

server/handler/base.go

+
+	results := b.findResultsWithDiscardedReviews(&result)
+	for _, r := range results {
+		dedupedReviews := b.dedupDiscardedReviews(r.DiscardedReviews)


I think we need to deduplicate the list after combining the reviews from all of the results. I expect duplicates to appear because multiple rules dismiss the same review rather than a single result dismissing the same review twice.

i removed the dedup, since I changed it to use the cached reviews from prctx instead.

devinburnette · 2022-09-16T23:48:47Z

policy/approval/approve.go

+		return
+	}
+
+	approved, msg, err := r.IsApproved(ctx, prctx, allowedCandidates)


i left this bit because i still thought it was a good idea for refactoring the tests, but let me know if you want me to revert this too.

devinburnette · 2022-09-16T23:57:35Z

server/handler/base.go

+	return false
+}
+
+func (b *Base) reasonForDimissedReview(review *pull.Review) string {


this is the only part that felt a little weird to me since we are no longer passing the discarded candidates, then we have to infer the reason based on the fields we have. this is probably okay now, but could get missed if we decide to add more ways that a review candidate could be filtered.

I agree this is fine for now - review handling hadn't changed until the introduction of comment matching and the edit filter, so it's not really a high-traffic area in the code. If this is insufficient for future candidate filtering, we can refactor then.

bluekeyes

This looks good, thanks for doing the refactor. I had a question/suggestion about the dismissal heuristic that I'd like your opinion on, but I don't think it's blocking.

bluekeyes · 2022-09-20T23:21:06Z

server/handler/base.go

+	return false
+}
+
+func (b *Base) reasonForDimissedReview(review *pull.Review) string {


I agree this is fine for now - review handling hadn't changed until the introduction of comment matching and the edit filter, so it's not really a high-traffic area in the code. If this is insufficient for future candidate filtering, we can refactor then.

bluekeyes · 2022-09-20T23:48:30Z

server/handler/base.go

+		return "was edited"
+	}
+
+	if review.CreatedAt.Before(time.Now().Add(-5 * time.Second)) {


I guess the assumption here is that if the review was submitted or edited (and doesn't match a pattern), the bot is responding to that event within 5 seconds, whereas it's unlikely that someone will submit a review and then a new commit will be pushed within 5 seconds?

I'm always hesitant about fixed time ranges like this, but as a heuristic for generating informational text, it's probably fine. An alternative might be to also pass the push time of the most recent commit in the Result struct and compare against that, but I'm not sure it is worth the effort.

yea it feels brittle to me but works.. I think it's hard to grok because it requires so much knowledge about the filtering.. 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 the finally is that it didn't include a github review comment pattern that was also required.

but yes this assumes the bot is responding within those 5 seconds, which seemed fine in my brief testing, but also still small enough to be unlikely that someone would push a new commit in that time frame. but yes still feels brittle, so i was hoping you would have a better solution. i'll try this!

Adding some comments on the function or next to each condition about what it's checking for and why this works might help decipher the logic for readers in the future too.

i tried all the approaches i could think of including your Result suggestion and nothing really felt any better, so just added a comment explaining the function as much as i could.

bluekeyes · 2022-09-20T23:48:53Z

server/handler/base.go

+
+		reason := b.reasonForDimissedReview(r)
+		message := fmt.Sprintf("dismissed because the approval %s", reason)
+		logger.Info().Msgf("dismissing stale review %s because it %s", r.ID, reason)


Suggested change

logger.Info().Msgf("dismissing stale review %s because it %s", r.ID, reason)

logger.Info().Msgf("Dismissing stale review %s because it %s", r.ID, reason)

bluekeyes · 2022-09-20T23:49:31Z

server/handler/base.go

+		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(errors.WithStack(err)).Msg("Failed to dismiss stale review")


Suggested change

logger.Err(errors.WithStack(err)).Msg("Failed to dismiss stale review")

logger.Err(err).Msg("Failed to dismiss stale review")

err should already have a stack from the errors.Wrap in dissmissPullRequestReview

bluekeyes

Looks good! Thanks for implementing this and iterating with me on the PR.

devinburnette added 5 commits August 27, 2022 12:04

dismiss stale reviews

e79bd9e

also dismiss for edited candidates too

5618099

working but with permissions issues

7d7e691

use enum

984a8d9

Merge branch 'develop' into devin-dismiss-reviews

e129af2

devinburnette commented Aug 30, 2022

View reviewed changes

server/handler/pull_request.go Outdated Show resolved Hide resolved

devinburnette added 10 commits September 1, 2022 01:03

working

7e42996

exit early if no reviews

723a861

oops

1434ffb

switch these

b854004

rm unnecessary if

18e6fed

dont need this

ab39ae7

continue if nothing to do

a3760e4

combine these

5d87aea

fix

2e73996

print all the reasons

35e41b5

move mutation to github

770bacc

devinburnette marked this pull request as ready for review September 6, 2022 23:22

devinburnette force-pushed the devin-dismiss-reviews branch 4 times, most recently from f2f9d4a to 72ca961 Compare September 7, 2022 22:32

use result instead

42b8acd

devinburnette force-pushed the devin-dismiss-reviews branch from 72ca961 to 42b8acd Compare September 7, 2022 22:34

bluekeyes reviewed Sep 9, 2022

View reviewed changes

devinburnette added 2 commits September 10, 2022 00:50

implement feedback

bbb50c6

dedup discarded reviews from common result children

4b84279

devinburnette force-pushed the devin-dismiss-reviews branch from 2161619 to 1b4c175 Compare September 11, 2022 00:03

fix tests

8ffc2c0

devinburnette force-pushed the devin-dismiss-reviews branch from 1b4c175 to 8ffc2c0 Compare September 11, 2022 00:23

fixed children

a3d3768

devinburnette requested a review from bluekeyes September 12, 2022 15:53

bluekeyes reviewed Sep 16, 2022

View reviewed changes

devinburnette commented Sep 16, 2022

View reviewed changes

devinburnette force-pushed the devin-dismiss-reviews branch 2 times, most recently from 7e27182 to ee20e96 Compare September 16, 2022 23:55

devinburnette commented Sep 16, 2022

View reviewed changes

devinburnette requested a review from bluekeyes September 17, 2022 00:00

devinburnette force-pushed the devin-dismiss-reviews branch from ee20e96 to bc6f2d3 Compare September 17, 2022 02:48

change approach to handle edited comments better

d86a367

devinburnette force-pushed the devin-dismiss-reviews branch from bc6f2d3 to d86a367 Compare September 17, 2022 07:04

bluekeyes approved these changes Sep 20, 2022

View reviewed changes

implement feedback

697a273

devinburnette force-pushed the devin-dismiss-reviews branch from 043a61f to 697a273 Compare September 21, 2022 17:59

bluekeyes approved these changes Sep 23, 2022

View reviewed changes

bluekeyes merged commit 47c25e3 into palantir:develop Sep 23, 2022

devinburnette deleted the devin-dismiss-reviews branch September 24, 2022 00:22

devinburnette mentioned this pull request Oct 13, 2022

Fix review dismissals bug #485

Merged


		var discardedReviews []*common.DiscardedReview

		if len(discarded) != 0 {

	func (r Rule) filteredCandidates(ctx context.Context, prctx pull.Context) ([]common.Candidate, []*common.Candidate, error) {
	func (r Rule) filteredCandidates(ctx context.Context, prctx pull.Context) (allow []common.Candidate, discard []*common.Candidate, err error) {

	logger.Info().Msgf("dismissing stale review %s because it %s", r.ID, reason)
	logger.Info().Msgf("Dismissing stale review %s because it %s", r.ID, reason)

	logger.Err(errors.WithStack(err)).Msg("Failed to dismiss stale review")
	logger.Err(err).Msg("Failed to dismiss stale review")

Dismiss stale reviews per rule #463

Dismiss stale reviews per rule #463

Conversation

devinburnette commented Aug 30, 2022 • edited Loading

bluekeyes commented Aug 30, 2022

devinburnette commented Aug 31, 2022

bluekeyes commented Aug 31, 2022 • edited Loading

devinburnette commented Sep 1, 2022

devinburnette commented Sep 6, 2022

bluekeyes commented Sep 6, 2022

devinburnette commented Sep 8, 2022

bluekeyes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinburnette commented Sep 11, 2022

bluekeyes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluekeyes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluekeyes left a comment

Choose a reason for hiding this comment

devinburnette commented Aug 30, 2022 •

edited

Loading

bluekeyes commented Aug 31, 2022 •

edited

Loading