[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
This commit is contained in:
Xavier Morel 2022-06-08 14:45:32 +02:00
parent 2204c0410a
commit 66c2bdc25b
4 changed files with 26 additions and 9 deletions

View File

@ -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}).'

View File

@ -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

View File

@ -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)

View File

@ -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">