From 42f58abf025b596931a439ce50c07382987a66d9 Mon Sep 17 00:00:00 2001
From: Tobias Henkel <tobias.henkel@bmw.de>
Date: Mon, 5 Oct 2020 14:35:44 +0200
Subject: [PATCH] Restore correct reason reporting of merge failures

The optimization of merges in GitHub [1] has a side effect that the
merge failure reason always is 'Method not allowed'. This has been
unnoticed because the test framework did not distinguish between
status message and response body. GitHub reports such failures with
405 Method not allowed and a detailed reason in the response body.

[1] I48dbb5fc1b7e7dec4cb09c779b9661d82935d9df

Change-Id: Ic2e47ee14c7f97defba0905db68ef8706108c081
---
 tests/fakegithub.py                    | 34 +++++++++-----------------
 tests/unit/test_github_driver.py       |  3 ++-
 zuul/driver/github/githubconnection.py |  8 ++++++
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/tests/fakegithub.py b/tests/fakegithub.py
index 2c226fe4fc..91245b3ce2 100644
--- a/tests/fakegithub.py
+++ b/tests/fakegithub.py
@@ -31,16 +31,6 @@ from zuul.driver.github.githubconnection import utc
 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
@@ -329,11 +319,7 @@ class FakeRepository(object):
 
         if self.fail_not_found > 0:
             self.fail_not_found -= 1
-
-            resp = ErrorResponse()
-            resp.status_code = 404
-            resp.message = 'Not Found'
-
+            resp = FakeResponse(404, 'Not found')
             raise github3.exceptions.NotFoundError(resp)
 
         commit = self._commits.get(sha, None)
@@ -565,8 +551,9 @@ class FakeIssueSearchResult(object):
 
 
 class FakeResponse(object):
-    def __init__(self, data, status_code=200):
+    def __init__(self, data, status_code=200, status_message='OK'):
         self.status_code = status_code
+        self.status_message = status_message
         self.data = data
         self.links = {}
 
@@ -581,8 +568,9 @@ class FakeResponse(object):
 
     def raise_for_status(self):
         if 400 <= self.status_code < 600:
-            raise HTTPError('{} Something went wrong'.format(self.status_code),
-                            response=self)
+            raise HTTPError(
+                '{} {}'.format(self.status_code, self.status_message),
+                response=self)
 
 
 class FakeGithubSession(object):
@@ -667,10 +655,12 @@ class FakeGithubSession(object):
                 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)
+                # GitHub returns 405 Method not allowed with more details in
+                # the body of the response.
+                data = {
+                    'message': 'Merge not allowed because of fake reason',
+                }
+                return FakeResponse(data, 405, 'Method not allowed')
             pr.setMerged(json["commit_message"])
             return FakeResponse({"merged": True}, 200)
 
diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py
index fe79e41918..35c8d92f53 100644
--- a/tests/unit/test_github_driver.py
+++ b/tests/unit/test_github_driver.py
@@ -675,7 +675,8 @@ class TestGithubDriver(ZuulTestCase):
         # Validate that the merge failure comment contains the message github
         # returned
         self.assertEqual(D.comments[0],
-                         'Pull request merge failed: 403 Merge not allowed')
+                         'Pull request merge failed: Merge not allowed '
+                         'because of fake reason')
 
     @simple_layout('layouts/merging-github.yaml', driver='github')
     def test_report_pull_merge_message_reviewed_by(self):
diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py
index f0452f8fe0..50433ef518 100644
--- a/zuul/driver/github/githubconnection.py
+++ b/zuul/driver/github/githubconnection.py
@@ -1892,6 +1892,14 @@ class GithubConnection(BaseConnection):
             else:
                 result = data["merged"]
         except Exception as e:
+            if hasattr(e, 'response'):
+                response = e.response
+                try:
+                    raise MergeFailure('Pull request merge failed: '
+                                       '%s' % response.json().get('message'))
+                except ValueError:
+                    # There was no json body so use the generic message below.
+                    pass
             raise MergeFailure('Pull request merge failed: %s' % e)
 
         if not result: