From 6cb58a322de0fa600f80eee62b0c4eda43113810 Mon Sep 17 00:00:00 2001 From: xmo-odoo Date: Wed, 31 Jul 2019 09:19:50 +0200 Subject: [PATCH] [IMP] runbot_merge: send feedback when approving PR which failed CI Also add test for it & feedback of an approved PR failing CI, and fix corner case with it (might not send a warning immediately on CI failure depending on status requirement ordering). Fixes #158 --- runbot_merge/models/pull_requests.py | 35 +++++++++++++++++--- runbot_merge/tests/test_basic.py | 49 ++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 710a682e..b6cafcda 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -490,6 +490,7 @@ class PullRequests(models.Model): ], default=2, index=True) statuses = fields.Text(compute='_compute_statuses') + status = fields.Char(compute='_compute_statuses') batch_id = fields.Many2one('runbot_merge.batch',compute='_compute_active_batch', store=True) batch_ids = fields.Many2many('runbot_merge.batch') @@ -517,13 +518,26 @@ class PullRequests(models.Model): for pr in self: pr.blocked = pr.id not in stageable - @api.depends('head') + @api.depends('head', 'repository.project_id.required_statuses') def _compute_statuses(self): Commits = self.env['runbot_merge.commit'] for s in self: c = Commits.search([('sha', '=', s.head)]) - if c and c.statuses: - s.statuses = pprint.pformat(json.loads(c.statuses)) + if not (c and c.statuses): + continue + + statuses = json.loads(c.statuses) + s.statuses = pprint.pformat(statuses) + + st = 'success' + for ci in s.repository.project_id.required_statuses.split(','): + v = state_(statuses, ci) or 'pending' + if v in ('error', 'failure'): + st = 'failure' + break + if v == 'pending': + st = 'pending' + s.status = st @api.depends('batch_ids.active') def _compute_active_batch(self): @@ -665,10 +679,19 @@ class PullRequests(models.Model): elif command == 'review': if param and is_reviewer: newstate = RPLUS.get(self.state) + print('r+', self.state, self.status) if newstate: self.state = newstate self.reviewed_by = author ok = True + if self.status == 'failure': + # the normal infrastructure is for failure and + # prefixes messages with "I'm sorry" + Feedback.create({ + 'repository': self.repository.id, + 'pull_request': self.number, + 'message': "You may want to rebuild or fix this PR as it has failed CI.", + }) else: msg = "This PR is already reviewed, reviewing it again is useless." elif not param and is_author: @@ -752,11 +775,13 @@ class PullRequests(models.Model): for pr in self: required = pr.repository.project_id.required_statuses.split(',') + success = True for ci in required: st = state_(statuses, ci) or 'pending' if st == 'success': continue + success = False # only report an issue of the PR is already approved (r+'d) if st in ('error', 'failure') and pr.state == 'approved': self.env['runbot_merge.pull_requests.feedback'].create({ @@ -764,8 +789,8 @@ class PullRequests(models.Model): 'pull_request': pr.number, 'message': "%r failed on this reviewed PR." % ci, }) - break - else: # all success (no break) + + if success: oldstate = pr.state if oldstate == 'opened': pr.state = 'validated' diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index b3efd92a..c19e0f29 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2469,6 +2469,55 @@ class TestComments: # check that PR is still unreviewed assert pr.state == 'opened' +class TestFeedback: + def test_ci_approved(self, repo, env, users): + """CI failing on an r+'d PR sends feedback""" + m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) + repo.make_ref('heads/master', m) + + c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'}) + prx = repo.make_pr('title', 'body', target='master', ctid=c1, user='user') + pr = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', prx.number) + ]) + + prx.post_comment('hansen r+', user='reviewer') + assert pr.state == 'approved' + + repo.post_status(prx.head, 'failure', 'ci/runbot') + run_crons(env) + + assert prx.comments == [ + (users['reviewer'], 'hansen r+'), + (users['user'], "'ci/runbot' failed on this reviewed PR.") + ] + + def test_review_unvalidated(self, repo, env, users): + """r+-ing a PR with failed CI sends feedback""" + m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) + repo.make_ref('heads/master', m) + + c1 = repo.make_commit(m, 'first', None, tree={'m': 'c1'}) + prx = repo.make_pr('title', 'body', target='master', ctid=c1, user='user') + pr = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', prx.number) + ]) + + repo.post_status(prx.head, 'failure', 'ci/runbot') + run_crons(env) + assert pr.state == 'opened' + + prx.post_comment('hansen r+', user='reviewer') + assert pr.state == 'approved' + + run_crons(env) + + assert prx.comments == [ + (users['reviewer'], 'hansen r+'), + (users['user'], "You may want to rebuild or fix this PR as it has failed CI.") + ] class TestInfrastructure: def test_protection(self, repo): """ force-pushing on a protected ref should fail