diff --git a/conftest.py b/conftest.py index 2bf6f4e0..db3822f7 100644 --- a/conftest.py +++ b/conftest.py @@ -1175,6 +1175,9 @@ class Model: def __len__(self): return len(self._ids) + def __hash__(self): + return hash((self._model, frozenset(self._ids))) + def __eq__(self, other): if not isinstance(other, Model): return NotImplemented diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py index d3d2aacd..c7b82b30 100644 --- a/runbot_merge/models/batch.py +++ b/runbot_merge/models/batch.py @@ -92,4 +92,11 @@ class Batch(models.Model): p.display_name for p in unready ) else: + if batch.blocked and batch.cancel_staging: + batch.target.active_staging_id.cancel( + 'unstaged by %s on %s (%s)', + self.env.user.login, + batch, + ', '.join(batch.prs.mapped('display_name')), + ) batch.blocked = False diff --git a/runbot_merge/models/commands.py b/runbot_merge/models/commands.py index 7d097d47..0f46251f 100644 --- a/runbot_merge/models/commands.py +++ b/runbot_merge/models/commands.py @@ -281,6 +281,8 @@ class Parser: return Priority.ALONE def parse_cancel(self) -> CancelStaging: + self.assert_next('=') + self.assert_next('staging') return CancelStaging() def parse_skipchecks(self) -> SkipChecks: diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index fb34361a..02eab8c4 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -700,7 +700,7 @@ class PullRequests(models.Model): else: msg = self._approve(author, login) case commands.Reject() if is_author: - if cancellers := self.batch_id.cancel_staging: + if self.batch_id.cancel_staging: self.batch_id.cancel_staging = False if self.batch_id.skipchecks or self.reviewed_by: if self.error: @@ -756,14 +756,11 @@ class PullRequests(models.Model): p.reviewed_by = author case commands.CancelStaging() if is_admin: self.batch_id.cancel_staging = True - # FIXME: remove this when skipchecks properly affects state, - # maybe: staging cancellation should then only occur - # when a cancel_staging PR transitions to ready, or - # a ready PR is flagged as cancelling staging - self.target.active_staging_id.cancel( - "Unstaged by %s on %s", - author.github_login, self.display_name, - ) + if not self.batch_id.blocked: + self.target.active_staging_id.cancel( + "Unstaged by %s on %s", + author.github_login, self.display_name, + ) case commands.Override(statuses): for status in statuses: overridable = author.override_rights\ @@ -1081,12 +1078,6 @@ class PullRequests(models.Model): def write(self, vals): if vals.get('squash'): vals['merge_method'] = False - fields = [] - canceler = vals.get('cancel_staging') or any(p.cancel_staging for p in self) - if canceler: - fields.append('state') - fields.append('skipchecks') - prev = {pr.id: {field: pr[field] for field in fields} for pr in self} # when explicitly marking a PR as ready if vals.get('state') == 'ready': @@ -1144,21 +1135,6 @@ class PullRequests(models.Model): if newhead: c = self.env['runbot_merge.commit'].search([('sha', '=', newhead)]) self._validate(c.statuses or '{}') - - if canceler: - for pr in self: - if not pr.cancel_staging: - continue - - old = prev[pr.id] - def ready(pr): - return pr['state'] == 'ready'\ - or (pr['skipchecks'] and pr['state'] != 'error') - if ready(pr) and not ready(old): - if old['state'] == 'error': # error -> ready gets a bespoke message - pr.target.active_staging_id.cancel(f"retrying {pr.display_name}") - else: - pr.target.active_staging_id.cancel(f"{pr.display_name} became ready") return w def _check_linked_prs_statuses(self, commit=False): diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 3c72b392..813a2685 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -9,6 +9,7 @@ import functools import operator import time import xmlrpc.client +from itertools import repeat import pytest import requests @@ -1468,3 +1469,65 @@ def test_freeze_conflict(env, project, repo_a, repo_b, repo_c, users, config): with pytest.raises(AssertionError) as e: repo_b.get_ref('heads/1.1') assert e.value.args[0].startswith("Not Found") + +def test_cancel_staging(env, project, repo_a, repo_b, users, config): + """If a batch is flagged as staging cancelling (from any PR), the staging + should get cancelled if and when the batch transitions to unblocked + """ + with repo_a, repo_b: + make_branch(repo_a, 'master', 'initial', {'a': '1'}) + make_branch(repo_b, 'master', 'initial', {'b': '1'}) + + pr_a = make_pr(repo_a, 'batch', [{'a': '2'}], user=config['role_user']['token'], statuses=[], reviewer=None) + pr_b = make_pr(repo_b, 'batch', [{'b': '2'}], user=config['role_user']['token'], statuses=[], reviewer=None) + pr_lone = make_pr( + repo_a, + "C", + [{'c': '1'}], + user=config['role_user']['token'], + reviewer=config['role_reviewer']['token'], + ) + env.run_crons() + + a_id, b_id, lone_id = map(to_pr, repeat(env), [pr_a, pr_b, pr_lone]) + assert lone_id.staging_id + st = lone_id.staging_id + + with repo_a: + pr_a.post_comment("hansen cancel=staging", config['role_reviewer']['token']) + assert a_id.state == 'opened' + assert a_id.cancel_staging + assert b_id.cancel_staging + assert lone_id.staging_id == st + with repo_a: + pr_a.post_comment('hansen r+', config['role_reviewer']['token']) + assert a_id.state == 'approved' + assert lone_id.staging_id == st + with repo_a: + repo_a.post_status(a_id.head, 'success', 'ci/runbot') + repo_a.post_status(a_id.head, 'success', 'legal/cla') + env.run_crons() + assert a_id.state == 'ready' + assert lone_id.staging_id == st + + assert b_id.state == 'opened' + with repo_b: + pr_b.post_comment('hansen r+', config['role_reviewer']['token']) + assert b_id.state == 'approved' + assert lone_id.staging_id == st + with repo_b: + repo_b.post_status(b_id.head, 'success', 'ci/runbot') + repo_b.post_status(b_id.head, 'success', 'legal/cla') + assert b_id.state == 'approved' + assert lone_id.staging_id == st + env.run_crons() + assert b_id.state == 'ready' + # should have cancelled the staging, picked a and b, and re-staged the + # entire thing + assert lone_id.staging_id != st + + assert len({ + lone_id.staging_id.id, + a_id.staging_id.id, + b_id.staging_id.id, + }) == 1