From 947bf3bb47909d84571a440f8c4e9a10919e89de Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 6 Oct 2021 14:42:54 +0200 Subject: [PATCH] [FIX] forwardport: don't re-approve approved PRs during an fw-bot r+ When using the forwardport's shortcut, the bot would not skip already-approved PRs leading to a warning from the mergebot that the PR was already approved (out of nowhere which was weird). During the walk to the ancestors, skip any PR which is not approvable (either already approved or in a state where that makes no sense e.g. closed). Fixes #543 --- forwardport/models/project.py | 3 +- forwardport/tests/test_simple.py | 48 ++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 40c52093..8b3e1f28 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -34,6 +34,7 @@ from odoo.exceptions import UserError from odoo.tools import topological_sort, groupby from odoo.tools.appdirs import user_cache_dir from odoo.addons.runbot_merge import utils +from odoo.addons.runbot_merge.models.pull_requests import RPLUS footer = '\nMore info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n' @@ -339,7 +340,7 @@ class PullRequests(models.Model): continue merge_bot = self.repository.project_id.github_prefix # don't update the root ever - for pr in filter(lambda p: p.parent_id, self._iter_ancestors()): + for pr in (p for p in self._iter_ancestors() if p.parent_id if p.state in RPLUS): # only the author is delegated explicitely on the pr._parse_commands(author, {**comment, 'body': merge_bot + ' r+'}, login) elif token == 'close': diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 46042730..fbce44d0 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -498,6 +498,54 @@ def signoff(conf, message): return signoff raise AssertionError("Failed to find signoff by %s in %s" % (conf, message)) + +def test_redundant_approval(env, config, make_repo, users): + """If a forward port sequence has been partially approved, fw-bot r+ should + not perform redundant approval as that triggers warning messages. + """ + prod, _ = make_basic(env, config, make_repo) + [project] = env['runbot_merge.project'].search([]) + with prod: + prod.make_commits( + 'a', Commit('p', tree={'x': '0'}), + ref='heads/early' + ) + pr0 = prod.make_pr(target='a', head='early') + prod.post_status('heads/early', 'success', 'legal/cla') + prod.post_status('heads/early', 'success', 'ci/runbot') + pr0.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + with prod: + prod.post_status('staging.a', 'success', 'legal/cla') + prod.post_status('staging.a', 'success', 'ci/runbot') + env.run_crons() + pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number asc') + with prod: + prod.post_status(pr1_id.head, 'success', 'legal/cla') + prod.post_status(pr1_id.head, 'success', 'ci/runbot') + env.run_crons() + + _, _, pr2_id = env['runbot_merge.pull_requests'].search([], order='number asc') + assert pr2_id.parent_id == pr1_id + assert pr1_id.parent_id == pr0_id + + pr1 = prod.get_pr(pr1_id.number) + pr2 = prod.get_pr(pr2_id.number) + with prod: + pr1.post_comment('hansen r+', config['role_reviewer']['token']) + with prod: + pr2.post_comment(f'{project.fp_github_name} r+', config['role_reviewer']['token']) + env.run_crons() + + assert pr1.comments == [ + seen(env, pr1, users), + (users['user'], 'This PR targets b and is part of the forward-port chain. ' + 'Further PRs will be created up to c.\n\n' + 'More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port\n'), + (users['reviewer'], 'hansen r+'), + ] + + def test_batched(env, config, make_repo, users): """ Tests for projects with multiple repos & sync'd branches. Batches should be FP'd to batches