Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR merges already when reviewers have commented #153

Closed
tdeekens opened this issue Aug 2, 2019 · 17 comments · Fixed by #182
Closed

PR merges already when reviewers have commented #153

tdeekens opened this issue Aug 2, 2019 · 17 comments · Fixed by #182
Labels
bug Something isn't working

Comments

@tdeekens
Copy link

tdeekens commented Aug 2, 2019

Thanks again for the bot! We noticed something confusing yesterday I wanted to clarify/ask about.

Given I create a PR 
And I assign the required "ship it"-label
And I assign two reviewers
And the bot is configured to merge only on no pending reviews
And one reviewers approves
And the 2nd only comments
Then Kodiak merges the PR

If this behaviour intended or am I confusing something? :)

@tdeekens tdeekens added the bug Something isn't working label Aug 2, 2019
@chdsbd
Copy link
Owner

chdsbd commented Aug 2, 2019

GitHub counts a comment as a review. Since Kodiak depends on the PR state to find this information, once a comment is left the review request is gone and Kodiak thinks the PR is mergeable.

As a workaround you may be able to use branch protection to require approving reviews.

I’m not immediately sure how to fix this issue.

I’m hesitant to store state about PRs in kodiak because I think it could make the app brittle. So I think the best solution would be to find if a user was requested as a reviewer and see if they approved, but I’m not sure if there is an api to see that activity.

Anyway, the behavior of this feature is confusing and should be improved. Thanks for reporting the issue.

@chdsbd
Copy link
Owner

chdsbd commented Aug 2, 2019

I think we can probably use the issue events api to derive this information so we don’t need to keep the state ourselves.

https://developer.github.com/v3/issues/events/#list-events-for-an-issue

@chdsbd
Copy link
Owner

chdsbd commented Aug 14, 2019

@tdeekens I was thinking about this issue and I'm not sure how it would be possible to cancel a review request once a user's review has been requested.

I'm curious if you have any ideas how to handle canceling a review request if we were to fix this issue.

@tdeekens
Copy link
Author

I think I am lacking context. Doesn't GitHub emit a review_request_removed event which Kodiak could use to reevaluate if a reviewer was removed again?

@chdsbd
Copy link
Owner

chdsbd commented Aug 20, 2019

I think that's correct, but I believe if someone were to leave a comment instead of making a review, that would satisfy the review request. So a user wouldn't be able to trigger a review_request_removed event by removing a review request because it would already be gone after the comment.

@tdeekens
Copy link
Author

Ah. Then I guess one has to really configure required approvals through GitHub and not Kodiak?

@staabm
Copy link
Contributor

staabm commented Oct 17, 2019

Then I guess one has to really configure required approvals through GitHub and not Kodiak?

yep.

@tdeekens
Copy link
Author

Sorry if closing was eager but I wasn't sure if there was work planned on this.

@chdsbd
Copy link
Owner

chdsbd commented Oct 18, 2019

This is still a problem I encounter that I would like to fix so I'm going to reopen this. I can create a new ticket if you'd like to close this @tdeekens.

@chdsbd chdsbd reopened this Oct 18, 2019
@tdeekens
Copy link
Author

No worries. Thanks, I just did not want to bother the project with long running/open issues. You efforts are much appreciated. Our team enjoys the bot every day.

chdsbd added a commit that referenced this issue Oct 20, 2019
kodiakhq bot pushed a commit that referenced this issue Oct 20, 2019
chdsbd added a commit that referenced this issue Oct 26, 2019
block_on_reviews_requested is fundamentally broken and cannot be fixed. This change documents this in the README.

closes #153
@kodiakhq kodiakhq bot closed this as completed in #182 Oct 26, 2019
kodiakhq bot pushed a commit that referenced this issue Oct 26, 2019
block_on_reviews_requested is fundamentally broken and cannot be fixed. This change documents this in the README.

closes #153
@levino
Copy link

levino commented Jun 26, 2020

Disclaimer: This is not intended as advertisement or anything so I wont put links. It is a remark on this dicussion!

I just installed mergify and mergify supports "Waiting for approving reviews" without Github branch protection rules. We have the use case that we want to enable the devs to merge without an approval but all the tests have passed and automatically merge once an approving review has been submitted. This is in order to provide the devs with the necessary autonomy to ship small, safe changes and stop the game of "Hey you, could you please approve this quickly so it will get merged.".

I thought this setup was not possible because the github API would not support it as claimed above by @chdsbd . At least the mergify team has found a workaround (maybe they do good old scraping?) to make it happen...

@chdsbd
Copy link
Owner

chdsbd commented Jun 26, 2020

Hi @levino,

Kodiak is mostly configured via GitHub branch protection rules in order to simplify its configuration and ideally make it "just work". As you've noticed you can configure Kodiak to wait for approving reviews via GitHub branch protection settings.

If you wanted Kodiak to wait for approving reviews without configuring GitHub branch protection settings, that's something that could be built.
Here's the code that looks for approving reviews when the "Require approving reviews" branch protection setting is enabled. You can see it's only enabled when requiresApprovingReviews is enabled via github branch protection.

if (
branch_protection.requiresApprovingReviews
and branch_protection.requiredApprovingReviewCount
):
reviews_by_author: MutableMapping[str, List[PRReview]] = defaultdict(list)
for review in sorted(reviews, key=lambda x: x.createdAt):
if review.author.permission not in {Permission.ADMIN, Permission.WRITE}:
continue
reviews_by_author[review.author.login].append(review)
successful_reviews = 0
for author_name, review_list in reviews_by_author.items():
review_state = review_status(review_list)
# blocking review
if review_state == PRReviewState.CHANGES_REQUESTED:
await block_merge(
api, pull_request, f"changes requested by {author_name!r}"
)
return
# successful review
if review_state == PRReviewState.APPROVED:
successful_reviews += 1
# missing required review count
if successful_reviews < branch_protection.requiredApprovingReviewCount:
await block_merge(
api,
pull_request,
f"missing required reviews, have {successful_reviews!r}/{branch_protection.requiredApprovingReviewCount!r}",
)
return

Feel free to open a feature request if you'd like this feature to be configurable outside GitHub branch protection.

The feature mentioned in this issue is a different from that mergify feature. Kodiak was waiting for any review, not just approving reviews. When a user left a comment via a GitHub review it counted as a review instead of a comment, so it confusingly satisfied this requirement.

@staabm
Copy link
Contributor

staabm commented Jun 26, 2020

you could also use a GithubAction to approve the PR automatically

@levino
Copy link

levino commented Jun 29, 2020

you could also use a GithubAction to approve the PR automatically

Why should one want a PR to be automatically approved? Then I could just not require reviews and merge once all the tests have passed.

@levino
Copy link

levino commented Jun 29, 2020

The feature mentioned in this issue is a different from that mergify feature. Kodiak was waiting for any review, not just approving reviews. When a user left a comment via a GitHub review it counted as a review instead of a comment, so it confusingly satisfied this requirement.

I am pretty sure that OP said that it counted a "Comment review" as an "Approving review". Even if, what is the point of merging a pull request if there are "x reviews" without making sure that the x reviews are approving reviews (and there is no rejecting review which in all the scenarios I am aware of counts as a veto and has to stop the PR from being merged). "Merging on x reviews no matter what their outcome" is not a reasonable use case for kodiak.

@chdsbd
Copy link
Owner

chdsbd commented Jun 29, 2020

Sorry for the confusion.

I think the GitHub action would approve the PR when you add a specific "approve" label. This would allow you to keep the "Require approving reviews" branch protection setting enabled while also allowing developers to merge their pull requests without a review.

The feature discussed in this issue was not about merging pull requests based on reviews. It was about not merging when a user's review was requested. The idea was that if you wanted someone to review your pull request, and didn't want Kodiak to merge until they reviewed, you could enabled this setting and Kodiak would wait for their review.

The problem with this feature was that while Kodiak would wait when there were requested reviews, those requested reviews would be removed by GitHub when a user commented, not just when they approved or requested changes. This was confusing, which is why this issue was opened and the feature removed.

The "Require approving reviews" branch protection setting is the best way to require reviews before merging a pull request.

On a slight tangent, I think if someone wanted to implement this "wait for requested reviews" feature correctly they could use issue "Assignees" and have a bot (Kodiak, a GitHub Action, etc.) set a blocking status check on the pull request until that assignee approved or requested changes.

@levino
Copy link

levino commented Jun 30, 2020

The "Require approving reviews" branch protection setting is the best way to require reviews before merging a pull request.

I disagree and that is also why I commented on this issue in the first place: It is much better, not to require approving reviews via branch protection. Consider the (common) situation that a team member opens a PR with a very simple, safe change like updating some config file. All checks pass but they cannot merge because the branch protection says: "You need two approving reviews.". So they start poking other team members to "quickly approve this so it will get merged". That is simply an annoying work flow. One could allow these developers to overrule the branch protection by giving them privileges but then they can also overrule failing checks! So instead I want to give the team members slightly more power and responsibility. They should be able to merge "by force" when all the checks have passed, even if the PR was not yet reviewed but not when checks fail. So instead of having kodiak merge the PR once the github user could merge the pull request, kodiak should be the "perfect citizen" who goes the extra mile and waits for reviews instead of hastily merging the PR. As I said, this is implemented in other solutions and as such possible. I just thought that you were under the impression that this setup would be impossible due to the poor feature set of githubs API, but maybe that was a misunderstanding. Still what I described would be a quite useful setup for kodiak too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants