From 7711d09854c0a28f3cba6a544b97b6b10b289603 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 12 Jun 2024 16:08:25 +0200 Subject: [PATCH] [IMP] *: add fw=no, deprecate ignore Without fw-bot being its bearer, "ignore" is a lot less clear than it used to as it looks to be asking to ignore the PR entirely (as if it was targeted to an unmanaged branch). Deprecate this command, and tack on the shortcut to the fw subcommand. It is slightly sub-par as technically it does not quite fit with the other subcommands, and furthermore can't be disabled via fw=default... although maybe it could be? Maybe instead of setting the limit fw=no could set that value to the forwardport mode, and the fw_policy users could check that? It would require some more finessing tho: - `DEFAULT` would need to be accessible to the author as well as the reviewers so the author could toggle between `NO` and `DEFAULT`. - There should probably be a warning of some sort when setting a limit to an unportable PR. - The dashboards would need to take `NO` in account (though I guess that's just defaulting the limit to the target). --- forwardport/tests/test_limit.py | 24 +++++++++++++++++------- runbot_merge/models/commands.py | 1 + runbot_merge/models/pull_requests.py | 21 ++++++++++++++++++++- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index b0f469db..14645c26 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -45,29 +45,39 @@ def test_configure_fp_limit(env, config, make_repo, source, limit, count, port): r = requests.get(f'http://localhost:{port}/{prod.name}/pull/{pr.number}.png') assert r.ok -def test_ignore(env, config, make_repo): +def test_ignore(env, config, make_repo, users): """ Provide an "ignore" command which is equivalent to setting the limit to target """ - prod, other = make_basic(env, config, make_repo) + prod, _ = make_basic(env, config, make_repo, statuses="default") branch_a = env['runbot_merge.branch'].search([('name', '=', 'a')]) with prod: [c] = prod.make_commits('a', Commit('c', tree={'0': '0'}), ref='heads/mybranch') pr = prod.make_pr(target='a', head='mybranch') - prod.post_status(c, 'success', 'legal/cla') - prod.post_status(c, 'success', 'ci/runbot') - pr.post_comment('hansen r+ ignore', config['role_reviewer']['token']) + prod.post_status(c, 'success') + env.run_crons() + with prod: + pr.post_comment('hansen ignore', config['role_reviewer']['token']) + pr.post_comment('hansen r+ fw=no', config['role_reviewer']['token']) env.run_crons() pr_id = env['runbot_merge.pull_requests'].search([('number', '=', pr.number)]) assert pr_id.limit_id == branch_a with prod: - prod.post_status('staging.a', 'success', 'legal/cla') - prod.post_status('staging.a', 'success', 'ci/runbot') + prod.post_status('staging.a', 'success') + env.run_crons() assert env['runbot_merge.pull_requests'].search([]) == pr_id,\ "should not have created a forward port" + assert pr.comments == [ + seen(env, pr, users), + (users['reviewer'], "hansen ignore"), + (users['reviewer'], "hansen r+ fw=no"), + (users['user'], "'ignore' is deprecated, use 'fw=no' to disable forward porting."), + (users['user'], "Forward-port disabled."), + (users['user'], "Forward-port disabled."), + ] @pytest.mark.expect_log_errors(reason="one of the limit-setting does not provide a branch name") def test_disable(env, config, make_repo, users): diff --git a/runbot_merge/models/commands.py b/runbot_merge/models/commands.py index 0f46251f..2911638b 100644 --- a/runbot_merge/models/commands.py +++ b/runbot_merge/models/commands.py @@ -137,6 +137,7 @@ class SkipChecks: class FW(enum.Enum): DEFAULT = enum.auto() + NO = enum.auto() SKIPCI = enum.auto() SKIPMERGE = enum.auto() diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 53386012..0656c851 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -812,7 +812,24 @@ class PullRequests(models.Model): case commands.Close() if source_author: feedback(close=True) case commands.FW(): - if source_reviewer or is_reviewer: + if command == commands.FW.NO: + if is_author: + for p in self.batch_id.prs: + ping, m = p._maybe_update_limit(self.target.name) + + if ping and p == self: + msg = m + else: + if ping: + m = f"@{login} {m}" + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': p.repository.id, + 'pull_request': p.number, + 'message': m, + }) + else: + msg = "you can't set a forward-port limit." + elif source_reviewer or is_reviewer: (self.source_id or self).batch_id.fw_policy = command.name.lower() match command: case commands.FW.DEFAULT: @@ -825,6 +842,8 @@ class PullRequests(models.Model): else: msg = "you can't configure forward-port CI." case commands.Limit(branch) if is_author: + if branch is None: + feedback(message="'ignore' is deprecated, use 'fw=no' to disable forward porting.") limit = branch or self.target.name for p in self.batch_id.prs: ping, m = p._maybe_update_limit(limit)