From 52699d901abe61432c0526a072c75bf434e42fec Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 18 Sep 2019 08:32:38 +0200 Subject: [PATCH] [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 | 31 +++++++++++++++++------ forwardport/tests/test_simple.py | 37 +++++++++++++++++++++++++--- runbot_merge/models/pull_requests.py | 18 ++++++++------ 3 files changed, 69 insertions(+), 17 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index b3dce75c..110aa64f 100644 --- a/forwardport/models/project.py +++ b/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``. diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 8ef76dc8..17b4557a 100644 --- a/forwardport/tests/test_simple.py +++ b/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 diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 8bff67db..9ae5fb2a 100644 --- a/runbot_merge/models/pull_requests.py +++ b/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()