zuul
/
zuul
Code Issues Proposed changes

Improve error reporting when pr merge fails 

When merging a pull request in GitHub fails we currently report the
standard message when a merge confict happened. This is actually
rarely the case because when using proper gating a merge should almost
never fail because of a merge conflict. Instead report the error we
get from GitHub so the user knows why the merge failed. In most cases
this is because branch protection refuses to merge because of a
condition zuul doesn't know about. This will then be reported to the
user so he can do something about it without asking a zuul admin for
the internal logs.

Change-Id: I1c3e82a2ac20e2177352b1a4d078839d3e114467
This commit is contained in:
Tobias Henkel 2020-02-13 14:09:26 +01:00
parent 96ad25036d
commit 793bb738a9
No known key found for this signature in database
GPG Key ID: 03750DEC158E5FA2
5 changed files with 51 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
tests/base.py
View File

@ -86,7 +86,6 @@ import zuul.nodepool
import zuul.rpcclient
import zuul.zk
import zuul.configloader
from zuul.exceptions import MergeFailure
from zuul.lib.config import get_default
from zuul.lib.logutil import get_annotated_logger
@ -2251,19 +2250,6 @@ class FakeGithubConnection(githubconnection.GithubConnection):
pull_request = self.pull_requests[int(pr_number)]
pull_request.addComment(message)
def mergePull(self, project, pr_number, commit_message='', sha=None,
method='merge', zuul_event_id=None):
# record that this got reported
self.reports.append((project, pr_number, 'merge', method))
pull_request = self.pull_requests[int(pr_number)]
if self.merge_failure:
raise Exception('Pull request was not merged')
if self.merge_not_allowed_count > 0:
self.merge_not_allowed_count -= 1
raise MergeFailure('Merge was not successful due to mergeability'
' conflict')
pull_request.setMerged(commit_message)
def setCommitStatus(self, project, sha, state, url='', description='',
context='default', user='zuul', zuul_event_id=None):
# record that this got reported and call original method

38
tests/fakegithub.py
View File

@ -24,6 +24,16 @@ from requests.structures import CaseInsensitiveDict
FAKE_BASE_URL = 'https://example.com/api/v3/'
class ErrorResponse:
status_code = 0
message = ''
def json(self):
return {
'message': self.message
}
class FakeUser(object):
def __init__(self, login):
self.login = login
@ -260,16 +270,7 @@ class FakeRepository(object):
if self.fail_not_found > 0:
self.fail_not_found -= 1
class Response:
status_code = 0
message = ''
def json(self):
return {
'message': self.message
}
resp = Response()
resp = ErrorResponse()
resp.status_code = 404
resp.message = 'Not Found'
@ -455,6 +456,23 @@ class FakePull(object):
# with the head_sha as the only commit
return [self.head]
def merge(self, commit_message=None, sha=None, merge_method=None):
conn = self._fake_pull_request.github
pr = self._fake_pull_request
# record that this got reported
conn.reports.append((pr.project, pr.number, 'merge', merge_method))
if conn.merge_failure:
raise Exception('Unknown merge failure')
if conn.merge_not_allowed_count > 0:
conn.merge_not_allowed_count -= 1
resp = ErrorResponse()
resp.status_code = 403
resp.message = 'Merge not allowed'
raise github3.exceptions.MethodNotAllowed(resp)
pr.setMerged(commit_message)
return True
def as_dict(self):
pr = self._fake_pull_request
connection = pr.github

13
tests/unit/test_github_driver.py
View File

@ -618,6 +618,7 @@ class TestGithubDriver(ZuulTestCase):
self.assertTrue(A.is_merged)
self.assertThat(A.merge_message,
MatchesRegex(r'.*PR title.*Reviewed-by.*', re.DOTALL))
self.assertEqual(len(A.comments), 0)
# pipeline merges the pull request on success after failure
self.fake_github.merge_failure = True
@ -625,6 +626,9 @@ class TestGithubDriver(ZuulTestCase):
self.fake_github.emitEvent(B.getCommentAddedEvent('merge me'))
self.waitUntilSettled()
self.assertFalse(B.is_merged)
self.assertEqual(len(B.comments), 1)
self.assertEqual(B.comments[0],
'Pull request merge failed: Unknown merge failure')
self.fake_github.merge_failure = False
# pipeline merges the pull request on second run of merge
@ -643,7 +647,10 @@ class TestGithubDriver(ZuulTestCase):
self.waitUntilSettled()
self.assertFalse(D.is_merged)
self.assertEqual(len(D.comments), 1)
self.assertEqual(D.comments[0], 'Merge failed')
# Validate that the merge failure comment contains the message github
# returned
self.assertEqual(D.comments[0],
'Pull request merge failed: 403 Merge not allowed')
@simple_layout('layouts/dependent-github.yaml', driver='github')
def test_draft_pr(self):
@ -1171,9 +1178,11 @@ class TestGithubDriver(ZuulTestCase):
# the change should have entered the gate
self.assertEqual(2, len(self.history))
# Merge should have failed because cherry-pick is not supported
self.assertEqual(2, len(A.comments))
self.assertFalse(A.is_merged)
self.assertIn('Merge Failed', A.comments[1])
self.assertEquals(A.comments[1],
'Merge mode cherry-pick not supported by Github')
@simple_layout('layouts/gate-github-squash-merge.yaml', driver='github')
def test_merge_method_squash_merge(self):

9
zuul/driver/github/githubconnection.py
View File

@ -1744,13 +1744,12 @@ class GithubConnection(BaseConnection):
try:
result = pull_request.merge(commit_message=commit_message, sha=sha,
merge_method=method)
except github3.exceptions.MethodNotAllowed as e:
raise MergeFailure('Merge was not successful due to mergeability'
' conflict, original error is %s' % e)
except Exception as e:
raise MergeFailure('Pull request merge failed: %s' % e)
log.debug("Merged PR %s/%s#%s", owner, proj, pr_number)
if not result:
raise Exception('Pull request was not merged')
raise MergeFailure('Pull request was not merged')
log.debug("Merged PR %s/%s#%s", owner, proj, pr_number)
def _getCommit(self, repository, sha, retries=5):
try:

15
zuul/driver/github/githubreporter.py
View File

@ -95,10 +95,10 @@ class GithubReporter(BaseReporter):
if self._create_comment or errors_received:
self.addPullComment(item)
if (self._merge):
self.mergePull(item)
if not item.change.is_merged:
msg = self._formatItemReportMergeFailure(item)
self.addPullComment(item, msg)
try:
self.mergePull(item)
except Exception as e:
self.addPullComment(item, str(e))
def _formatItemReportJobs(self, item):
# Return the list of jobs portion of the report
@ -161,8 +161,7 @@ class GithubReporter(BaseReporter):
if merge_mode not in self.merge_modes:
mode = [x[0] for x in MERGER_MAP.items() if x[1] == merge_mode][0]
self.log.warning('Merge mode %s not supported by Github', mode)
# TODO(tobiash): report error to user
return
raise MergeFailure('Merge mode %s not supported by Github' % mode)
merge_mode = self.merge_modes[merge_mode]
project = item.change.project.name
@ -179,13 +178,15 @@ class GithubReporter(BaseReporter):
zuul_event_id=item.event)
item.change.is_merged = True
return
except MergeFailure:
except MergeFailure as e:
log.exception('Merge attempt of change %s %s/2 failed.',
item.change, i, exc_info=True)
error_message = str(e)
if i == 1:
time.sleep(2)
log.warning('Merge of change %s failed after 2 attempts, giving up',
item.change)
raise MergeFailure(error_message)
def addReview(self, item):
log = get_annotated_logger(self.log, item.event)