Merge "Improve error reporting when pr merge fails"

tests/base.py

@@ -87,7 +87,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
 
@@ -2252,19 +2251,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

tests/fakegithub.py

@@ -28,6 +28,16 @@ from tests.fake_graphql import FakeGithubQuery
 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
@@ -276,16 +286,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'
 
@@ -472,6 +473,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

tests/unit/test_github_driver.py

@@ -620,6 +620,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
@@ -627,6 +628,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
@@ -645,7 +649,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):
@@ -1173,9 +1180,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):

zuul/driver/github/githubconnection.py

@@ -1736,13 +1736,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:

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)