mirror of
https://github.com/odoo/runbot.git
synced 2025-03-15 23:45:44 +07:00
[IMP] runbot_merge: link to failed runbot builds
a0063f9df0
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.
This commit is contained in:
parent
8f7a5e55ef
commit
7310cd1f1d
@ -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'
|
||||
|
@ -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
|
||||
|
@ -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}
|
||||
|
@ -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 {})
|
||||
}
|
||||
))
|
||||
|
||||
|
@ -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()
|
||||
|
@ -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)
|
||||
|
@ -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):
|
||||
|
Loading…
Reference in New Issue
Block a user