diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 24a8e15b..cf3f0c20 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -252,6 +252,21 @@ class PullRequests(models.Model): 'token_field': 'fp_github_token', }) + def _notify_ci_failed(self, ci): + # only care about FP PRs which are not staged / merged yet + # NB: probably ignore approved PRs as normal message will handle them? + if not (self.state == 'opened' and self.parent_id): + return + + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': self.repository.id, + 'pull_request': self.number, + 'token_field': 'fp_github_token', + 'message': '%s\n\n%s failed on this forward-port PR' % ( + self.source_id._pingline(), + ci, + ) + }) def _validate(self, statuses): _logger = logging.getLogger(__name__).getChild('forwardport.next') @@ -264,13 +279,6 @@ class PullRequests(models.Model): continue if pr.state not in ['validated', 'ready']: _logger.info('-> wrong state (%s)', pr.state) - if pr in failed and pr.state not in ['closed', 'merged']: - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'token_field': 'fp_github_token', - 'message': pr.source_id._pingline() + '\n\nCI failed on this forward-port PR' - }) continue # check if we've already forward-ported this branch: diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 6069b615..4eb41393 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -236,13 +236,15 @@ This PR targets b and is part of the forward-port chain. Further PRs will be cre More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port ''') - ci_warning = (users['user'], 'Ping @%(user)s, @%(reviewer)s\n\nCI failed on this forward-port PR' % users) + ci_warning = (users['user'], 'Ping @%(user)s, @%(reviewer)s\n\nci/runbot failed on this forward-port PR' % users) # oh no CI of the first FP PR failed! - with prod: - prod.post_status(pr1.head, 'failure', 'ci/runbot') - prod.post_status(pr1.head, 'success', 'legal/cla') - env.run_crons() + # simulate status being sent multiple times (e.g. on multiple repos) with + # some delivery lag allowing for the cron to run between each delivery + for st, ctx in [('failure', 'ci/runbot'), ('failure', 'ci/runbot'), ('success', 'legal/cla'), ('success', 'legal/cla')]: + with prod: + prod.post_status(pr1.head, st, ctx) + env.run_crons() # check that FP did not resume & we have a ping on the PR assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\ "forward port should not continue on CI failure" @@ -251,7 +253,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port # it was a false positive, rebuild... it fails again! with prod: - prod.post_status(pr1.head, 'failure', 'ci/runbot') + prod.post_status(pr1.head, 'failure', 'ci/runbot', target_url='http://example.org/4567890') env.run_crons() # check that FP did not resume & we have a ping on the PR assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\ diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 40975dbc..d2a6357e 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -190,14 +190,17 @@ def handle_status(env, event): env.cr.execute('SELECT id FROM runbot_merge_commit WHERE sha=%s FOR UPDATE', [event['sha']]) c = Commits.browse(env.cr.fetchone()) if c: - c.statuses = json.dumps({ - **json.loads(c.statuses), + old = json.loads(c.statuses) + new = { + **old, event['context']: { 'state': event['state'], 'target_url': event['target_url'], 'description': event['description'] } - }) + } + if new != old: # don't update the commit if nothing's changed (e.g dupe status) + c.statuses = json.dumps(new) else: Commits.create({ 'sha': event['sha'], diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index efb04e78..b3994924 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -505,6 +505,7 @@ class PullRequests(models.Model): statuses = fields.Text(compute='_compute_statuses') status = fields.Char(compute='_compute_statuses') + previous_failure = fields.Char(default='{}') batch_id = fields.Many2one('runbot_merge.batch',compute='_compute_active_batch', store=True) batch_ids = fields.Many2many('runbot_merge.batch') @@ -823,13 +824,7 @@ class PullRequests(models.Model): success = False if st in ('error', 'failure'): failed |= pr - # only report an issue of the PR is already approved (r+'d) - if pr.state == 'approved': - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr.repository.id, - 'pull_request': pr.number, - 'message': "%r failed on this reviewed PR." % ci, - }) + self._notify_ci_new_failure(ci, to_status(statuses.get(ci.strip(), 'pending'))) if success: oldstate = pr.state if oldstate == 'opened': @@ -838,6 +833,23 @@ class PullRequests(models.Model): pr.state = 'ready' return failed + def _notify_ci_new_failure(self, ci, st): + # only sending notification if the newly failed status is different than + # the old one + prev = json.loads(self.previous_failure) + if st != prev: + self.previous_failure = json.dumps(st) + self._notify_ci_failed(ci) + + def _notify_ci_failed(self, ci): + # only report an issue of the PR is already approved (r+'d) + if self.state == 'approved': + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': self.repository.id, + 'pull_request': self.number, + 'message': "%r failed on this reviewed PR." % ci, + }) + def _auto_init(self): super(PullRequests, self)._auto_init() # incorrect index: unique(number, target, repository). @@ -1254,8 +1266,6 @@ class Commit(models.Model): r = super(Commit, self).write(values) return r - # NB: GH recommends doing heavy work asynchronously, may be a good - # idea to defer this to a cron or something def _notify(self): Stagings = self.env['runbot_merge.stagings'] PRs = self.env['runbot_merge.pull_requests']