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

Add check for required statuses. #51

Merged
merged 3 commits into from
Mar 20, 2018
Merged

Add check for required statuses. #51

merged 3 commits into from
Mar 20, 2018

Conversation

kurtisvg
Copy link
Contributor

@kurtisvg kurtisvg commented Mar 19, 2018

This adds a check that all statuses are required by the protections on the branch are required to be reported before DPE bot merges a branch. This will help prevent accidental merges like GoogleCloudPlatform/java-docs-samples#1049 when a required check hasn't started yet.

Protection API Reference

I've tested the new functions locally to confirm that they work, but there is a difference between some of the syntax in the older functions. I was unable to get the following to work:

'https://api.github.com/repos/{}/{}/pulls/{}/reviews'.format(
    pr.repository[0], pr.repository[1], pr.number),

and instead had to use:

'https://api.github.com/repos/{}/pulls/{}/reviews'.format(
    pr.repository, pr.number),

I am not sure why this is, but suspected maybe a difference in library version. If you can clarify which is preferred for DPEbots environment, I can update to whatever works.

@kurtisvg kurtisvg requested a review from theacodes March 19, 2018 22:32
@@ -183,6 +183,11 @@ def merge_pull_request(repo, pull, commit_sha=None):
logging.info('Not merging {}, not labeled automerge'.format(pull))
return

# only merge if all required status are reported
if not github_helper.has_required_statuses(pull):
logging.info('Not merging {}, missing required status')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you forgot format(pull) here. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@theacodes theacodes merged commit a4c6f40 into master Mar 20, 2018
@theacodes theacodes deleted the protect-update branch March 20, 2018 16:19
@theacodes
Copy link

deployed.

@kurtisvg
Copy link
Contributor Author

@lesv FYI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants