From 793bb738a91af1c9f2fc8fa1e627f5f2ca254328 Mon Sep 17 00:00:00 2001 From: Tobias Henkel Date: Thu, 13 Feb 2020 14:09:26 +0100 Subject: [PATCH] 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 --- tests/base.py | 14 ---------- tests/fakegithub.py | 38 +++++++++++++++++++------- tests/unit/test_github_driver.py | 13 +++++++-- zuul/driver/github/githubconnection.py | 9 +++--- zuul/driver/github/githubreporter.py | 15 +++++----- 5 files changed, 51 insertions(+), 38 deletions(-) diff --git a/tests/base.py b/tests/base.py index 8ca571d070..4fb0a92e92 100644 --- a/tests/base.py +++ b/tests/base.py @@ -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 diff --git a/tests/fakegithub.py b/tests/fakegithub.py index f3ab7e69bc..75156a460b 100644 --- a/tests/fakegithub.py +++ b/tests/fakegithub.py @@ -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 diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index 3a9cbd0cac..7e97fa6b6d 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -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): diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 0bb1808d79..96736446c8 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -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: diff --git a/zuul/driver/github/githubreporter.py b/zuul/driver/github/githubreporter.py index 93b209242a..2c5fbdb2a7 100644 --- a/zuul/driver/github/githubreporter.py +++ b/zuul/driver/github/githubreporter.py @@ -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)