From 7310cd1f1d94baab5462f3c292255005eb984015 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 17 Sep 2018 11:04:31 +0200 Subject: [PATCH] [IMP] runbot_merge: link to failed runbot builds a0063f9df091ef901cfb055b002e9cf4107688bc slightly improved the error message on non-PR ci failure (e.g. a community PR makes enterprise break) by adding the failed commit, but that's still not exactly clear, even for technical users (plus it requires having access to all the repos which is not the case for everyone). This commit further improves the situation by storing the target_url and description fields of the commit statuses, and printing out the target_url on failure if it's present. That way the PR comment denoting build failure should now have a link to the relevant failed build on runbot, as that's the target_url it provides. The change is nontrivial as it tries to be compatible with both old and new statuses storage format, such that there is no migration to perform. --- runbot_merge/controllers/__init__.py | 17 ++++++--- runbot_merge/github.py | 3 +- runbot_merge/models/pull_requests.py | 43 +++++++++++++++++++--- runbot_merge/tests/fake_github/__init__.py | 18 +++++---- runbot_merge/tests/remote.py | 4 +- runbot_merge/tests/test_basic.py | 14 ++++++- runbot_merge/tests/test_multirepo.py | 6 +-- 7 files changed, 78 insertions(+), 27 deletions(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 0de09319..bdaa6672 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -181,21 +181,28 @@ def handle_pr(env, event): def handle_status(env, event): _logger.info( - 'status %s:%s on commit %s', - event['context'], event['state'], - event['sha'], + 'status %(context)s:%(state)s on commit %(sha)s (%(target_url)s)', + event ) Commits = env['runbot_merge.commit'] c = Commits.search([('sha', '=', event['sha'])]) if c: c.statuses = json.dumps({ **json.loads(c.statuses), - event['context']: event['state'] + event['context']: { + 'state': event['state'], + 'target_url': event['target_url'], + 'description': event['description'] + } }) else: Commits.create({ 'sha': event['sha'], - 'statuses': json.dumps({event['context']: event['state']}) + 'statuses': json.dumps({event['context']: { + 'state': event['state'], + 'target_url': event['target_url'], + 'description': event['description'] + }}) }) return 'ok' diff --git a/runbot_merge/github.py b/runbot_merge/github.py index 4f93123f..da08656d 100644 --- a/runbot_merge/github.py +++ b/runbot_merge/github.py @@ -166,8 +166,7 @@ class GH(object): r = self('get', 'commits/{}/status'.format(h)).json() return [{ 'sha': r['sha'], - 'context': s['context'], - 'state': s['state'], + **s, } for s in r['statuses']] PR_COMMITS_MAX = 50 diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 3cfa6813..e8ff718a 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -585,7 +585,7 @@ class PullRequests(models.Model): # targets for pr in self: required = pr.repository.project_id.required_statuses.split(',') - if all(statuses.get(r.strip()) == 'success' for r in required): + if all(state_(statuses, r) == 'success' for r in required): oldstate = pr.state if oldstate == 'opened': pr.state = 'validated' @@ -790,7 +790,7 @@ class Stagings(models.Model): st = 'success' for c in commits: statuses = json.loads(c.statuses) - for v in map(statuses.get, reqs): + for v in map(lambda n: state_(statuses, n), reqs): if st == 'failure' or v in ('error', 'failure'): st = 'failure' elif v in (None, 'pending'): @@ -859,9 +859,10 @@ class Stagings(models.Model): commit = self.env['runbot_merge.commit'].search([ ('sha', '=', head) ]) + statuses = json.loads(commit.statuses) reason = next(( - ctx for ctx, result in json.loads(commit.statuses).items() - if result in ('error', 'failure') + ctx for ctx, result in statuses.items() + if to_status(result).get('state') in ('error', 'failure') ), None) if not reason: continue @@ -870,10 +871,15 @@ class Stagings(models.Model): pr for pr in self.batch_ids.prs if pr.repository.name == repo ), None) + + status = to_status(statuses[reason]) + viewmore = '' + if status.get('target_url'): + viewmore = ' (view more at %(target_url)s)' % status if pr: - self.fail(reason, pr) + self.fail("%s%s" % (reason, viewmore), pr) else: - self.fail('%s failed on %s' % (reason, head)) + self.fail('%s on %s%s' % (reason, head, viewmore)) return False # the staging failed but we don't have a specific culprit, fail @@ -1013,3 +1019,28 @@ class FetchJob(models.Model): active = fields.Boolean(default=True) repository = fields.Many2one('runbot_merge.repository', index=True, required=True) number = fields.Integer(index=True, required=True) + +# The commit (and PR) statuses was originally a map of ``{context:state}`` +# however it turns out to clarify error messages it'd be useful to have +# a bit more information e.g. a link to the CI's build info on failure and +# all that. So the db-stored statuses are now becoming a map of +# ``{ context: {state, target_url, description } }``. The issue here is +# there's already statuses stored in the db so we need to handle both +# formats, hence these utility functions) +def state_(statuses, name): + """ Fetches the status state """ + name = name.strip() + v = statuses.get(name) + if isinstance(v, dict): + return v.get('state') + return v +def to_status(v): + """ Converts old-style status values (just a state string) to new-style + (``{state, target_url, description}``) + + :type v: str | dict + :rtype: dict + """ + if isinstance(v, dict): + return v + return {'state': v, 'target_url': None, 'description': None} diff --git a/runbot_merge/tests/fake_github/__init__.py b/runbot_merge/tests/fake_github/__init__.py index 2faf54cf..67ee3fc7 100644 --- a/runbot_merge/tests/fake_github/__init__.py +++ b/runbot_merge/tests/fake_github/__init__.py @@ -132,11 +132,11 @@ class Repo(object): commits.extend(self.commit(r) for r in c.parents) yield c.to_json() - def post_status(self, ref, status, context='default', description=""): - assert status in ('error', 'failure', 'pending', 'success') + def post_status(self, ref, state, context='default', **kw): + assert state in ('error', 'failure', 'pending', 'success') c = self.commit(ref) - c.statuses.append((status, context, description)) - self.notify('status', self.name, context, status, c.id) + c.statuses.append({'state': state, 'context': context, **kw}) + self.notify('status', self.name, context, state, c.id, kw) def make_commit(self, ref, message, author, committer=None, tree=None): assert tree, "a commit must provide either a full tree" @@ -307,9 +307,8 @@ class Repo(object): 'total_count': len(c.statuses), # TODO: combined? 'statuses': [ - {'context': context, 'state': state} - for state, context, _ in reversed(c.statuses) - ] + {'description': None, 'target_url': None, **st} + for st in reversed(c.statuses)] }) def _read_issue(self, r, number): @@ -703,7 +702,7 @@ class Client(werkzeug.test.Client): } )) - def status(self, repository, context, state, sha): + def status(self, repository, context, state, sha, kw): assert state in ('success', 'failure', 'pending') return self.open(self._make_env( 'status', { @@ -712,6 +711,9 @@ class Client(werkzeug.test.Client): 'state': state, 'sha': sha, 'repository': self._repo(repository), + 'target_url': None, + 'description': None, + **(kw or {}) } )) diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index f0af3288..e0f313d0 100644 --- a/runbot_merge/tests/remote.py +++ b/runbot_merge/tests/remote.py @@ -518,12 +518,12 @@ class Repo: wait_for_hook(2) return PR(self, 'heads/' + ref, r.json()['number']) - def post_status(self, ref, status, context='default', description=""): + def post_status(self, ref, status, context='default', **kw): assert status in ('error', 'failure', 'pending', 'success') r = self._session.post('https://api.github.com/repos/{}/statuses/{}'.format(self.name, self.get_ref(ref)), json={ 'state': status, 'context': context, - 'description': description, + **kw }) assert 200 <= r.status_code < 300, r.json() wait_for_hook() diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index ebd389fe..b42b252e 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -1,4 +1,5 @@ import datetime +import json import re import pytest @@ -31,6 +32,10 @@ def test_trivial_flow(env, repo): # nothing happened repo.post_status(c1, 'success', 'legal/cla') + # rewrite status payload in old-style to ensure it does not break + c = env['runbot_merge.commit'].search([('sha', '=', c1)]) + c.statuses = json.dumps({k: v['state'] for k, v in json.loads(c.statuses).items()}) + repo.post_status(c1, 'success', 'ci/runbot') assert pr.state == 'validated' env['runbot_merge.project']._check_progress() @@ -1509,7 +1514,8 @@ class TestUnknownPR: c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'}) prx = repo.make_pr('title', 'body', target='master', ctid=c1, user='user') repo.post_status(prx.head, 'success', 'legal/cla') - repo.post_status(prx.head, 'success', 'ci/runbot') + repo.post_status(prx.head, 'success', 'ci/runbot', target_url="http://example.org/wheee") + # assume an unknown but ready PR: we don't know the PR or its head commit env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), @@ -1525,6 +1531,12 @@ class TestUnknownPR: env['runbot_merge.project']._check_fetch() assert not Fetch.search([('repository', '=', repo.name), ('number', '=', prx.number)]) + c = env['runbot_merge.commit'].search([('sha', '=', prx.head)]) + assert json.loads(c.statuses) == { + 'legal/cla': {'state': 'success', 'target_url': None, 'description': None}, + 'ci/runbot': {'state': 'success', 'target_url': 'http://example.org/wheee', 'description': None} + } + pr = env['runbot_merge.pull_requests'].search([ ('repository.name', '=', repo.name), ('number', '=', prx.number) diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index cfbd541e..dee975ee 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -286,9 +286,9 @@ def test_other_failed(env, project, repo_a, repo_b, owner, users): assert pr.staging_id repo_a.post_status('heads/staging.master', 'success', 'legal/cla') - repo_a.post_status('heads/staging.master', 'success', 'ci/runbot') + repo_a.post_status('heads/staging.master', 'success', 'ci/runbot', target_url="http://example.org/a") repo_b.post_status('heads/staging.master', 'success', 'legal/cla') - repo_b.post_status('heads/staging.master', 'failure', 'ci/runbot') + repo_b.post_status('heads/staging.master', 'failure', 'ci/runbot', target_url="http://example.org/b") env['runbot_merge.project']._check_progress() sth = repo_b.commit('heads/staging.master').id @@ -296,7 +296,7 @@ def test_other_failed(env, project, repo_a, repo_b, owner, users): assert pr.state == 'error' assert pr_a.comments == [ (users['reviewer'], 'hansen r+'), - (users['user'], 'Staging failed: ci/runbot failed on %s' % sth) + (users['user'], 'Staging failed: ci/runbot on %s (view more at http://example.org/b)' % sth) ] def test_batching(env, project, repo_a, repo_b):