From fafa7ef437d75d72e34be91af72e16c52135b7fa Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 7 Oct 2019 16:38:14 +0200 Subject: [PATCH] [IMP] *: attempt to avoid some of the FP spam Attempt to avoid some of the comment spam by dedup-ing input (only signaling when the status actually changes and ignoring identity transformations) and in case of failing CI keeping the last failed status and not signaling on the next update if it's the same failure. Closes #225 --- forwardport/models/project.py | 22 +++++++++++++++------- forwardport/tests/test_simple.py | 14 ++++++++------ runbot_merge/controllers/__init__.py | 9 ++++++--- runbot_merge/models/pull_requests.py | 28 +++++++++++++++++++--------- 4 files changed, 48 insertions(+), 25 deletions(-) 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']