[IMP] forwardport: ping on CI failure

It's especially important as users / assignees don't get
pinged *during* the forwardport process.

closes #203

forwardport/models/project.py

@@ -222,13 +222,23 @@ class PullRequests(models.Model):
 
     def _validate(self, statuses):
         _logger = logging.getLogger(__name__).getChild('forwardport.next')
-        super()._validate(statuses)
+        failed = super()._validate(statuses)
         # if the PR has a parent and is CI-validated, enqueue the next PR
         for pr in self:
             _logger.info('Checking if forward-port %s (%s)', pr, pr.number)
-            if not pr.parent_id or pr.state not in ('validated', 'ready'):
-                _logger.info('-> no parent or wrong state (%s)', pr.state)
+            if not pr.source_id:
+                _logger.info('-> no parent (%s)', pr)
                 continue
+            if pr.state not in ['validated', 'ready']:
+                _logger.info('-> wrong state (%s)', pr.state)
+                if pr in failed:
+                    self.env['runbot_merge.pull_requests.feedback'].create({
+                        'repository': pr.repository.id,
+                        'pull_request': pr.number,
+                        'message': pr.source_id._pingline() + '\n\nCI failed on this forward-port PR'
+                    })
+                continue
+
             # if it already has a child, bail
             if self.search_count([('parent_id', '=', self.id)]):
                 _logger.info('-> already has a child')
@@ -262,6 +272,7 @@ class PullRequests(models.Model):
                 'batch_id': batch.id,
                 'source': 'fp',
             })
+        return failed
 
     def _forward_port_sequence(self):
         # risk: we assume disabled branches are still at the correct location
@@ -452,8 +463,6 @@ class PullRequests(models.Model):
                 ]
             })
 
-            assignees = (source.author | source.reviewed_by).mapped('github_login')
-            ping = "Ping %s" % ', '.join('@' + login for login in assignees if login)
             if h:
                 sout = serr = ''
                 if out.strip():
@@ -461,7 +470,7 @@ class PullRequests(models.Model):
                 if err.strip():
                     serr = "\nstderr:\n```\n%s\n```\n" % err
 
-                message = ping + """
+                message = source._pingline() + """
 Cherrypicking %s of source #%d failed
 %s%s
 Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).
@@ -474,7 +483,7 @@ In the former case, you may want to edit this PR message as well.
                     for p in pr._iter_ancestors()
                     if p.parent_id and p.parent_id != source
                 )
-                message = ping + """
+                message = source._pingline() + """
 This PR targets %s and is the last of the forward-port chain%s
 %s
 To merge the full chain, say
@@ -508,6 +517,14 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
             'active': not has_conflicts,
         })
 
+    def _pingline(self):
+        assignees = (self.author | self.reviewed_by).mapped('github_login')
+        return "Ping %s" % ', '.join(
+            '@' + login
+            for login in assignees
+            if login
+        )
+
     def _create_fp_branch(self, target_branch, fp_branch_name, cleanup):
         """ Creates a forward-port for the current PR to ``target_branch`` under
         ``fp_branch_name``.

forwardport/tests/test_simple.py

@@ -254,7 +254,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
     # check that we didn't just smash the original trees
     assert prod.read_tree(prod.commit('a')) != b_tree != c_tree
 
-def test_update_pr(env, config, make_repo):
+def test_update_pr(env, config, make_repo, users):
     """ Even for successful cherrypicks, it's possible that e.g. CI doesn't
     pass or the reviewer finds out they need to update the code.
 
@@ -282,10 +282,41 @@ def test_update_pr(env, config, make_repo):
     env.run_crons()
 
     pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number')
-    with prod: # FP to c as well
-        prod.post_status(pr1.head, 'success', 'ci/runbot')
+
+    fp_intermediate = (users['user'], '''\
+This PR targets b and is part of the forward-port chain. Further PRs will be created up to c.
+
+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)
+
+    # 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()
+    # 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"
+    pr1_remote = prod.get_pr(pr1.number)
+    assert pr1_remote.comments == [fp_intermediate, ci_warning]
+
+    # it was a false positive, rebuild... it fails again!
+    with prod:
+        prod.post_status(pr1.head, 'failure', 'ci/runbot')
+    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,\
+        "ensure it still hasn't restarted"
+    assert pr1_remote.comments == [fp_intermediate, ci_warning, ci_warning]
+
+    # nb: updating the head would detach the PR and not put it in the warning
+    # path anymore
+
+    # rebuild again, finally passes
+    with prod:
+        prod.post_status(pr1.head, 'success', 'ci/runbot')
+    env.run_crons()
 
     pr0, pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number')
     assert pr1.parent_id == pr0

runbot_merge/models/pull_requests.py

@@ -810,6 +810,7 @@ class PullRequests(models.Model):
         # could have two PRs (e.g. one open and one closed) at least
         # temporarily on the same head, or on the same head with different
         # targets
+        failed = self.browse(())
         for pr in self:
             required = pr.repository.project_id.required_statuses.split(',')
 
@@ -820,13 +821,15 @@ class PullRequests(models.Model):
                     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({
-                        'repository': pr.repository.id,
-                        'pull_request': pr.number,
-                        'message': "%r failed on this reviewed PR." % ci,
-                    })
+                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,
+                        })
 
             if success:
                 oldstate = pr.state
@@ -834,6 +837,7 @@ class PullRequests(models.Model):
                     pr.state = 'validated'
                 elif oldstate == 'approved':
                     pr.state = 'ready'
+        return failed
 
     def _auto_init(self):
         super(PullRequests, self)._auto_init()