Handle review requirements in canMerge
Since GitHub 2.21 we now can query the reviewDecision flag on a PR which tells us if a review is required and if it's approved or not [1]. This can be leveraged in the canMerge check so we now finally can accurately check if a change is allowed to enter the gate. [1] https://developer.github.com/enterprise/2.21/v4/object/pullrequest/ Change-Id: I792a28b9f3c7d40ac21e22438bd7c09d3174cbb2
This commit is contained in:
Tobias Henkel
parent
0986673990
commit
b2f6d48cc5
|
|
@ -0,0 +1,6 @@
|
|||
---
|
||||
features:
|
||||
- |
|
||||
Zuul now respects GitHub review requirements when enqueuing into gate
|
||||
pipelines. This works for github.com and GitHub Enterprise starting with
|
||||
version 2.21.0.
|
||||
|
|
@ -133,10 +133,31 @@ class FakeCommit(ObjectType):
|
|||
|
||||
class FakePullRequest(ObjectType):
|
||||
isDraft = Boolean()
|
||||
reviewDecision = String()
|
||||
|
||||
def resolve_isDraft(parent, info):
|
||||
return parent.draft
|
||||
|
||||
def resolve_reviewDecision(parent, info):
|
||||
if hasattr(info.context, 'version') and info.context.version:
|
||||
if info.context.version < (2, 21, 0):
|
||||
raise Exception('Field unsupported')
|
||||
|
||||
# Check branch protection rules if reviews are required
|
||||
org, project = parent.project.split('/')
|
||||
repo = info.context._data.repos[(org, project)]
|
||||
rule = repo._branch_protection_rules.get(parent.branch)
|
||||
if not rule or not rule.require_reviews:
|
||||
# Github returns None if there is no review required
|
||||
return None
|
||||
|
||||
approvals = [r for r in parent.reviews
|
||||
if r.data['state'] == 'APPROVED']
|
||||
if approvals:
|
||||
return 'APPROVED'
|
||||
|
||||
return 'REVIEW_REQUIRED'
|
||||
|
||||
|
||||
class FakeRepository(ObjectType):
|
||||
name = String()
|
||||
|
|
@ -163,4 +184,4 @@ class FakeGithubQuery(ObjectType):
|
|||
name=String(required=True))
|
||||
|
||||
def resolve_repository(root, info, owner, name):
|
||||
return info.context.repos.get((owner, name))
|
||||
return info.context._data.repos.get((owner, name))
|
||||
|
|
|
|||
|
|
@ -240,7 +240,7 @@ class FakeRepository(object):
|
|||
return self._branches
|
||||
|
||||
def _set_branch_protection(self, branch_name, protected=True,
|
||||
contexts=None):
|
||||
contexts=None, require_review=False):
|
||||
if not protected:
|
||||
if branch_name in self._branch_protection_rules:
|
||||
del self._branch_protection_rules[branch_name]
|
||||
|
|
@ -249,6 +249,7 @@ class FakeRepository(object):
|
|||
rule = self._branch_protection_rules[branch_name]
|
||||
rule.pattern = branch_name
|
||||
rule.required_contexts = contexts or []
|
||||
rule.require_reviews = require_review
|
||||
|
||||
def _set_permission(self, key, value):
|
||||
# NOTE (felix): Currently, this is only used to mock a repo with
|
||||
|
|
@ -624,7 +625,11 @@ class FakeGithubSession(object):
|
|||
query = json.get('query')
|
||||
variables = json.get('variables')
|
||||
result = self.schema.execute(
|
||||
query, variables=variables, context=self.client._data)
|
||||
query, variables=variables, context=self.client)
|
||||
if result.errors:
|
||||
# Note that github really returns 200 and an errors field in
|
||||
# case of an error.
|
||||
return FakeResponse({'errors': result.errors}, 200)
|
||||
return FakeResponse({'data': result.data}, 200)
|
||||
|
||||
# Handle creating comments
|
||||
|
|
@ -760,5 +765,13 @@ class FakeGithubClient(object):
|
|||
|
||||
class FakeGithubEnterpriseClient(FakeGithubClient):
|
||||
|
||||
version = '2.21.0'
|
||||
|
||||
def __init__(self, url, session=None, verify=True):
|
||||
super().__init__(session=session)
|
||||
|
||||
def meta(self):
|
||||
data = {
|
||||
'installed_version': self.version,
|
||||
}
|
||||
return data
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
- pipeline:
|
||||
name: merge
|
||||
description: Pipeline for merging the pull request
|
||||
manager: independent
|
||||
manager: dependent
|
||||
merge-failure-message: Merge failed
|
||||
trigger:
|
||||
github:
|
||||
|
|
|
|||
|
|
@ -0,0 +1,20 @@
|
|||
[gearman]
|
||||
server=127.0.0.1
|
||||
|
||||
[web]
|
||||
root=http://zuul.example.com/
|
||||
|
||||
[merger]
|
||||
git_dir=/tmp/zuul-test/git
|
||||
git_user_email=zuul@example.com
|
||||
git_user_name=zuul
|
||||
|
||||
[executor]
|
||||
git_dir=/tmp/zuul-test/executor-git
|
||||
|
||||
[connection github]
|
||||
driver=github
|
||||
webhook_token=0000000000000000000000000000000000000000
|
||||
app_id=1
|
||||
app_key=$APP_KEY_FIXTURE$
|
||||
server=github.enterprise.io
|
||||
|
|
@ -25,6 +25,7 @@ from unittest import mock, skip
|
|||
import git
|
||||
import github3.exceptions
|
||||
|
||||
from tests.fakegithub import FakeGithubEnterpriseClient
|
||||
from zuul.driver.github.githubconnection import GithubShaCache
|
||||
import zuul.rpcclient
|
||||
|
||||
|
|
@ -2145,3 +2146,81 @@ class TestCheckRunAnnotations(ZuulGithubAppTestCase, AnsibleZuulTestCase):
|
|||
"start_line": 3,
|
||||
"end_line": 3,
|
||||
})
|
||||
|
||||
|
||||
class TestGithubDriverEnterise(ZuulGithubAppTestCase):
|
||||
config_file = 'zuul-github-driver-enterprise.conf'
|
||||
|
||||
@simple_layout('layouts/merging-github.yaml', driver='github')
|
||||
def test_report_pull_merge(self):
|
||||
github = self.fake_github.getGithubClient()
|
||||
repo = github.repo_from_project('org/project')
|
||||
repo._set_branch_protection(
|
||||
'master', require_review=True)
|
||||
|
||||
# pipeline merges the pull request on success
|
||||
A = self.fake_github.openFakePullRequest('org/project', 'master',
|
||||
'PR title',
|
||||
body='I shouldnt be seen',
|
||||
body_text='PR body')
|
||||
|
||||
self.fake_github.emitEvent(A.getCommentAddedEvent('merge me'))
|
||||
self.waitUntilSettled()
|
||||
|
||||
# Since the PR was not approved it should not be merged
|
||||
self.assertFalse(A.is_merged)
|
||||
|
||||
A.addReview('derp', 'APPROVED')
|
||||
self.fake_github.emitEvent(A.getCommentAddedEvent('merge me'))
|
||||
self.waitUntilSettled()
|
||||
|
||||
# After approval it should be merged
|
||||
self.assertTrue(A.is_merged)
|
||||
self.assertThat(A.merge_message,
|
||||
MatchesRegex(r'.*PR title\n\nPR body.*', re.DOTALL))
|
||||
self.assertThat(A.merge_message,
|
||||
Not(MatchesRegex(
|
||||
r'.*I shouldnt be seen.*',
|
||||
re.DOTALL)))
|
||||
self.assertEqual(len(A.comments), 0)
|
||||
|
||||
|
||||
class TestGithubDriverEnteriseLegacy(ZuulGithubAppTestCase):
|
||||
config_file = 'zuul-github-driver-enterprise.conf'
|
||||
|
||||
def setUp(self):
|
||||
self.old_version = FakeGithubEnterpriseClient.version
|
||||
FakeGithubEnterpriseClient.version = '2.19.0'
|
||||
|
||||
super().setUp()
|
||||
|
||||
def tearDown(self):
|
||||
super().tearDown()
|
||||
FakeGithubEnterpriseClient.version = self.old_version
|
||||
|
||||
@simple_layout('layouts/merging-github.yaml', driver='github')
|
||||
def test_report_pull_merge(self):
|
||||
github = self.fake_github.getGithubClient()
|
||||
repo = github.repo_from_project('org/project')
|
||||
repo._set_branch_protection(
|
||||
'master', require_review=True)
|
||||
|
||||
# pipeline merges the pull request on success
|
||||
A = self.fake_github.openFakePullRequest('org/project', 'master',
|
||||
'PR title',
|
||||
body='I shouldnt be seen',
|
||||
body_text='PR body')
|
||||
|
||||
self.fake_github.emitEvent(A.getCommentAddedEvent('merge me'))
|
||||
self.waitUntilSettled()
|
||||
|
||||
# Note: PR was not approved but old github does not support
|
||||
# reviewDecision so this gets ignored and zuul merges nevertheless
|
||||
self.assertTrue(A.is_merged)
|
||||
self.assertThat(A.merge_message,
|
||||
MatchesRegex(r'.*PR title\n\nPR body.*', re.DOTALL))
|
||||
self.assertThat(A.merge_message,
|
||||
Not(MatchesRegex(
|
||||
r'.*I shouldnt be seen.*',
|
||||
re.DOTALL)))
|
||||
self.assertEqual(len(A.comments), 0)
|
||||
|
|
|
|||
|
|
@ -868,6 +868,9 @@ class GithubClientManager:
|
|||
self.installation_map = {}
|
||||
self.installation_token_cache = {}
|
||||
|
||||
# The version of github enterprise stays None for github.com
|
||||
self._github_version = None
|
||||
|
||||
def initialize(self):
|
||||
self.log.info('Authing to GitHub')
|
||||
self._authenticateGithubAPI()
|
||||
|
|
@ -920,9 +923,17 @@ class GithubClientManager:
|
|||
"GitHub Enterprise")
|
||||
github = self.github_enterprise_class(
|
||||
url, session=session, verify=self.verify_ssl)
|
||||
if not self._github_version:
|
||||
version = github.meta().get('installed_version')
|
||||
self._github_version = tuple(
|
||||
[int(v) for v in version.split('.', 2)])
|
||||
else:
|
||||
github = self.github_class(session=session)
|
||||
|
||||
# Attach a version number to the github client so we can support per
|
||||
# version features.
|
||||
github.version = self._github_version
|
||||
|
||||
# anything going through requests to http/s goes through cache
|
||||
github.session.mount('http://', self.cache_adapter)
|
||||
github.session.mount('https://', self.cache_adapter)
|
||||
|
|
@ -1634,15 +1645,12 @@ class GithubConnection(BaseConnection):
|
|||
if not self._hasRequiredStatusChecks(allow_needs, canmerge_data):
|
||||
return False
|
||||
|
||||
if canmerge_data.get('requiresApprovingReviews'):
|
||||
if canmerge_data.get('requiresCodeOwnerReviews'):
|
||||
# we need to process the reviews using code owners
|
||||
# TODO(tobiash): not implemented yet
|
||||
pass
|
||||
else:
|
||||
# we need to process the review using access rights
|
||||
# TODO(tobiash): not implemented yet
|
||||
pass
|
||||
review_decision = canmerge_data['reviewDecision']
|
||||
if review_decision and review_decision != 'APPROVED':
|
||||
# If we got a review decision it must be approved
|
||||
log.debug('Change %s can not merge because it is not approved',
|
||||
change)
|
||||
return False
|
||||
|
||||
return True
|
||||
|
||||
|
|
|
|||
|
|
@ -36,6 +36,7 @@ class GraphQLClient:
|
|||
self.log.debug('Loading prepared graphql queries')
|
||||
query_names = [
|
||||
'canmerge',
|
||||
'canmerge-legacy',
|
||||
]
|
||||
for query_name in query_names:
|
||||
self.queries[query_name] = resource_string(
|
||||
|
|
@ -57,7 +58,15 @@ class GraphQLClient:
|
|||
'pull': pull,
|
||||
'head_sha': sha,
|
||||
}
|
||||
query = self._prepare_query(self.queries['canmerge'], variables)
|
||||
if github.version and github.version[:2] < (2, 21):
|
||||
# Github Enterprise prior to 2.21 doesn't offer the review decision
|
||||
# so don't request it as this will result in an error.
|
||||
query = self.queries['canmerge-legacy']
|
||||
else:
|
||||
# Since GitHub Enterprise 2.21 and on github.com we can request the
|
||||
# review decision state of the pull request.
|
||||
query = self.queries['canmerge']
|
||||
query = self._prepare_query(query, variables)
|
||||
response = github.session.post(self.url, json=query)
|
||||
return response.json()
|
||||
|
||||
|
|
@ -96,6 +105,11 @@ class GraphQLClient:
|
|||
pull_request = nested_get(repository, 'pullRequest')
|
||||
result['isDraft'] = nested_get(pull_request, 'isDraft', default=False)
|
||||
|
||||
# Get review decision. This is supported since GHE 2.21. Default to
|
||||
# None to signal if the field is not present.
|
||||
result['reviewDecision'] = nested_get(
|
||||
pull_request, 'reviewDecision', default=None)
|
||||
|
||||
# Add status checks
|
||||
result['status'] = {}
|
||||
commit = nested_get(data, 'data', 'repository', 'object')
|
||||
|
|
|
|||
|
|
@ -0,0 +1,40 @@
|
|||
query canMergeData(
|
||||
$owner: String!
|
||||
$repo: String!
|
||||
$pull: Int!
|
||||
$head_sha: String!
|
||||
) {
|
||||
repository(owner: $owner, name: $repo) {
|
||||
branchProtectionRules(first: 100) {
|
||||
nodes {
|
||||
pattern
|
||||
requiredStatusCheckContexts
|
||||
requiresApprovingReviews
|
||||
requiresCodeOwnerReviews
|
||||
}
|
||||
}
|
||||
pullRequest(number: $pull) {
|
||||
isDraft
|
||||
}
|
||||
object(expression: $head_sha) {
|
||||
... on Commit {
|
||||
checkSuites(first: 100) {
|
||||
nodes {
|
||||
checkRuns(first: 100) {
|
||||
nodes {
|
||||
name
|
||||
conclusion
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
status {
|
||||
contexts {
|
||||
state
|
||||
context
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -15,6 +15,7 @@ query canMergeData(
|
|||
}
|
||||
pullRequest(number: $pull) {
|
||||
isDraft
|
||||
reviewDecision
|
||||
}
|
||||
object(expression: $head_sha) {
|
||||
... on Commit {
|
||||
|
|
|
|||
Loading…
Reference in New Issue