From 66c2bdc25b2524cb92087d100017ee3d15f8b3ca Mon Sep 17 00:00:00 2001
From: Xavier Morel <xmo@odoo.com>
Date: Wed, 8 Jun 2022 14:45:32 +0200
Subject: [PATCH] [IMP] runbot_merge: error reporting on fast-forward failure

When a staging's fast-forward (to the target branch) fails, the
mergebot would provide no useful information on the staging or the
dashboard.

This is because the reason was set to the HTTP status, which in case
of a fast-forward error is just "422 client error: unprocessable
entity".

Improve this by trying to parse github's response in that case, and
using the JSON error message as failure reason. This provides more
useful failure information like "update is not a fast forward",
"reference does not exist", or a branch protection failure.

Closes #591
---
 mergebot_test_utils/utils.py     |  2 +-
 runbot_merge/github.py           | 15 ++++++++++++---
 runbot_merge/tests/test_basic.py | 13 +++++++++++--
 runbot_merge/views/templates.xml |  5 ++---
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/mergebot_test_utils/utils.py b/mergebot_test_utils/utils.py
index e3d09349..4c50afb3 100644
--- a/mergebot_test_utils/utils.py
+++ b/mergebot_test_utils/utils.py
@@ -49,7 +49,7 @@ class re_matches:
         return self._r.match(text)
 
     def __repr__(self):
-        return '~' + self._r.pattern + '~'
+        return self._r.pattern + '...'
 
 def seen(env, pr, users):
     return users['user'], f'[Pull request status dashboard]({to_pr(env, pr).url}).'
diff --git a/runbot_merge/github.py b/runbot_merge/github.py
index 8a368ed2..786f2924 100644
--- a/runbot_merge/github.py
+++ b/runbot_merge/github.py
@@ -202,10 +202,19 @@ class GH(object):
             def _wait_for_update():
                 if not self._check_updated(branch, sha):
                     return
-                raise exceptions.FastForwardError(self._repo)
-        except requests.HTTPError:
+                raise exceptions.FastForwardError(self._repo) \
+                    from Exception("timeout: never saw %s" % sha)
+        except requests.HTTPError as e:
             _logger.debug('fast_forward(%s, %s, %s) -> ERROR', self._repo, branch, sha, exc_info=True)
-            raise exceptions.FastForwardError(self._repo)
+            if e.response.status_code == 422:
+                try:
+                    r = e.response.json()
+                except Exception:
+                    pass
+                else:
+                    if isinstance(r, dict) and 'message' in r:
+                        e = Exception(r['message'].lower())
+            raise exceptions.FastForwardError(self._repo) from e
 
     def set_ref(self, branch, sha):
         # force-update ref
diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py
index 99358859..085455af 100644
--- a/runbot_merge/tests/test_basic.py
+++ b/runbot_merge/tests/test_basic.py
@@ -574,7 +574,7 @@ def test_staging_ci_failure_single(env, repo, users, config, page):
     assert dangerbox
     assert dangerbox[0].text == 'ci/runbot'
 
-def test_ff_failure(env, repo, config):
+def test_ff_failure(env, repo, config, page):
     """ target updated while the PR is being staged => redo staging """
     with repo:
         m = repo.make_commit(None, 'initial', None, tree={'m': 'm'})
@@ -587,10 +587,11 @@ def test_ff_failure(env, repo, config):
         repo.post_status(prx.head, 'success', 'ci/runbot')
         prx.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
     env.run_crons()
-    assert env['runbot_merge.pull_requests'].search([
+    st = env['runbot_merge.pull_requests'].search([
         ('repository.name', '=', repo.name),
         ('number', '=', prx.number)
     ]).staging_id
+    assert st
 
     with repo:
         m2 = repo.make_commit('heads/master', 'cockblock', None, tree={'m': 'm', 'm2': 'm2'})
@@ -603,6 +604,14 @@ def test_ff_failure(env, repo, config):
         repo.post_status(staging.id, 'success', 'ci/runbot')
     env.run_crons()
 
+    assert st.reason == 'update is not a fast forward'
+    # check that it's added as title on the staging
+    doc = html.fromstring(page('/runbot_merge'))
+    _new, prev = doc.cssselect('li.staging')
+
+    assert 'bg-gray-lighter' in prev.classes, "ff failure is ~ cancelling"
+    assert prev.get('title') == re_matches('fast forward failed \(update is not a fast forward\)')
+
     assert env['runbot_merge.pull_requests'].search([
         ('repository.name', '=', repo.name),
         ('number', '=', prx.number)
diff --git a/runbot_merge/views/templates.xml b/runbot_merge/views/templates.xml
index fab3b1e6..c0a86866 100644
--- a/runbot_merge/views/templates.xml
+++ b/runbot_merge/views/templates.xml
@@ -135,14 +135,13 @@
                     <t t-if="staging_index >= 4">visible-lg-block</t>
                 </t>
                 <t t-set="title">
-                    <t t-if="staging.state == 'ff_failed'">fast forward failed</t>
+                    <t t-if="staging.state == 'ff_failed'">fast forward failed (<t t-esc="staging.reason"/>)</t>
                     <t t-if="staging.state == 'pending'">last status</t>
-                    <t t-if="staging.state not in ('pending', 'ff_failed')"><t t-esc="staging.reason"/></t>
                 </t>
                 <!-- separate concatenation to avoid having line-break in title as some browsers trigger it -->
                 <!-- write-date may have microsecond precision, strip that information -->
                 <!-- use write-date under assumption that a staging is last modified when it ends -->
-                <t t-set="title"><t t-esc="title.strip()"/> at <t t-esc="staging.write_date.replace(microsecond=0)"/>Z</t>
+                <t t-set="title"><t t-esc="title.strip() or staging.reason"/> at <t t-esc="staging.write_date.replace(microsecond=0)"/>Z</t>
                 <li t-attf-class="staging {{stateclass.strip()}} {{decorationclass.strip()}}" t-att-title="title">
                     <ul class="list-unstyled">
                         <li t-foreach="staging.batch_ids" t-as="batch" class="batch">