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

Fail if changes are requested or not enough people approved the PR #362

Merged
merged 6 commits into from Mar 12, 2018

Conversation

Projects
None yet
4 participants
@grahamc
Contributor

grahamc commented Mar 12, 2018

Fixes #357

grahamc added some commits Mar 12, 2018

@grahamc

This comment has been minimized.

Contributor

grahamc commented Mar 12, 2018

@notriddle

bors r+

bors bot added a commit that referenced this pull request Mar 12, 2018

Merge #361 #362
361: Pull out the GitHub URL, so you should be able to use this with GitHub Enterprise r=notriddle a=notriddle



362: Fail if changes are requested or not enough people approved the PR r=notriddle a=grahamc

Fixes #357

notriddle added a commit to bors-ng/bors-ng.github.io that referenced this pull request Mar 12, 2018

@notriddle

This comment has been minimized.

Member

notriddle commented Mar 12, 2018

Would you, or someone else, like to add this to the reference docs?

@grahamc

This comment has been minimized.

Contributor

grahamc commented Mar 12, 2018

On it!

@bors

This comment has been minimized.

Contributor

bors bot commented Mar 12, 2018

@bors bors bot merged commit be12bf2 into bors-ng:master Mar 12, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
bors Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@notriddle

This comment has been minimized.

Member

notriddle commented Mar 12, 2018

You might also want to let JensRantil know that you implemented the feature he wanted. 😁

@heinrich5991

This comment has been minimized.

heinrich5991 commented Mar 12, 2018

@notriddle I think this is the culprit for ddnet/ddnet#1079 (comment).

@heinrich5991

This comment has been minimized.

heinrich5991 commented Mar 13, 2018

Maybe the default for number of code reviews should be 0, since the person writing "bors r+" presumably did one.

@notriddle

This comment has been minimized.

Member

notriddle commented Mar 13, 2018

That's not the bug, actually. The problem is that it's still counting the old Change Request, even though the same user later posted an Approval.

@heinrich5991

This comment has been minimized.

heinrich5991 commented Mar 13, 2018

AFAICT this is the standard way to deal with Change Requests. Github displays an "Approve Changes" link on the Change Request if you're the author which does what you can see in the PR.

As far as the Github UI is concerned, there's only "1 Approving Review" now.

@notriddle

This comment has been minimized.

Member

notriddle commented Mar 13, 2018

Yes, that's the bug.

@grahamc grahamc deleted the tweag:skip-change-requested branch Mar 13, 2018

bors bot added a commit that referenced this pull request Mar 13, 2018

Merge #365
365: Only count the latest review from any particular user r=notriddle a=notriddle

Fixes #364

Regression introduced by #362
@JensRantil

This comment has been minimized.

JensRantil commented Mar 15, 2018

FYI, I didn't know about this work so I put together https://github.com/tink-ab/four-eyes which solves the same case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment