From a6a37f8896ae3c0728ec1e83f5edb9dbb7f0f350 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 9 Feb 2024 08:58:19 +0100 Subject: [PATCH] [FIX] runbot_merge: handling of staging cancellation Move staging cancellation to the batch, remove its (complicated) handling from the PRs. This loses some precision in the cancellation message, but that could likely be recovered (in part) by adding more precise checks & diagnostic extractions in the compute. --- conftest.py | 3 ++ runbot_merge/models/batch.py | 7 ++++ runbot_merge/models/commands.py | 2 + runbot_merge/models/pull_requests.py | 36 +++------------- runbot_merge/tests/test_multirepo.py | 63 ++++++++++++++++++++++++++++ 5 files changed, 81 insertions(+), 30 deletions(-) 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