policy-bot
policy-bot copied to clipboard
palantir
Dismiss stale reviews per rule
Closes https://github.com/palantir/policy-bot/issues/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
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.
@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.
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.
@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.
@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.
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.
@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 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?