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