[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

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

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,\

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'],

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']