diff --git a/runbot_merge/models/stagings_create.py b/runbot_merge/models/stagings_create.py index 231ee969..017b63a9 100644 --- a/runbot_merge/models/stagings_create.py +++ b/runbot_merge/models/stagings_create.py @@ -61,8 +61,8 @@ def try_staging(branch: Branch) -> Optional[Stagings]: def log(label: str, batches: Batch) -> None: _logger.info(label, ', '.join(batches.mapped('prs.display_name'))) - alone, batches = ready_prs(for_branch=branch) - print('alone', alone, 'ready', batches) + alone, batches = ready_batches(for_branch=branch) + if alone: log("staging high-priority PRs %s", batches) elif branch.project_id.staging_priority == 'default': @@ -75,13 +75,6 @@ def try_staging(branch: Branch) -> Optional[Stagings]: # first as long as there's room log("staging ready PRs %s (prioritising splits)", batches) elif branch.project_id.staging_priority == 'ready': - # splits are ready by definition, we need to exclude them from the - # ready rows otherwise we'll re-stage the splits so if an error is legit - # we cycle forever - # FIXME: once the batches are less shit, store this durably on the - # batches and filter out when fetching readies (?) - batches -= branch.split_ids.batch_ids - if batches: log("staging ready PRs %s (prioritising ready)", batches) else: @@ -91,11 +84,6 @@ def try_staging(branch: Branch) -> Optional[Stagings]: log("staging split PRs %s (prioritising ready)", batches) else: assert branch.project_id.staging_priority == 'largest' - # splits are ready by definition, we need to exclude them from the - # ready rows otherwise ready always wins but we re-stage the splits, so - # if an error is legit we'll cycle forever - batches -= branch.split_ids.batch_ids - maxsplit = max(branch.split_ids, key=lambda s: len(s.batch_ids), default=branch.env['runbot_merge.split']) _logger.info("largest split = %d, ready = %d", len(maxsplit.batch_ids), len(batches)) # bias towards splits if len(ready) = len(batch_ids) @@ -201,13 +189,17 @@ For-Commit-Id: {it.head} return st -def ready_prs(for_branch: Branch) -> Tuple[bool, Batch]: +def ready_batches(for_branch: Branch) -> Tuple[bool, Batch]: env = for_branch.env + # splits are ready by definition, we need to exclude them from the ready + # rows otherwise if a prioritised (alone) PR is part of a split it'll be + # staged through priority *and* through split. + split_ids = for_branch.split_ids.batch_ids.ids env.cr.execute(""" SELECT max(priority) FROM runbot_merge_batch - WHERE blocked IS NULL AND target = %s - """, [for_branch.id]) + WHERE blocked IS NULL AND target = %s AND NOT id = any(%s) + """, [for_branch.id, split_ids]) alone = env.cr.fetchone()[0] == 'alone' return ( @@ -216,6 +208,7 @@ def ready_prs(for_branch: Branch) -> Tuple[bool, Batch]: ('target', '=', for_branch.id), ('blocked', '=', False), ('priority', '=', 'alone') if alone else (1, '=', 1), + ('id', 'not in', split_ids), ], order="priority DESC, id ASC"), ) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 273c0aae..dc458b34 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2969,6 +2969,50 @@ class TestBatching(object): assert not p_01.staging_id, "p_01 should not be picked up as it's failed" assert p_21.staging_id, "p_21 should have been staged" + def test_urgent_split(self, env, repo, config): + """Ensure that urgent (alone) PRs which get split don't get + double-merged + """ + with repo: + repo.make_commits( + None, + Commit("initial", tree={'a': '1'}), + ref="heads/master" + ) + + pr01 = self._pr( + repo, "PR1", [{'b': '1'}], + user=config['role_user']['token'], + reviewer=None, + ) + pr01.post_comment('hansen alone r+', config['role_reviewer']['token']) + pr02 = self._pr( + repo, "PR2", [{'c': '1'}], + user=config['role_user']['token'], + reviewer=None, + ) + pr02.post_comment('hansen alone r+', config['role_reviewer']['token']) + env.run_crons('runbot_merge.process_updated_commits') + pr01_id = to_pr(env, pr01) + assert pr01_id.blocked is False + pr02_id = to_pr(env, pr02) + assert pr01_id.blocked is False + + env.run_crons() + st = pr01_id.staging_id + assert st and pr02_id.staging_id == st + with repo: + repo.post_status('staging.master', 'failure', 'ci/runbot') + env.run_crons() + # should have cancelled the staging, split it, and re-staged the first + # half of the split + assert st.state == 'failure' + assert pr01_id.staging_id and pr01_id.staging_id != st + assert not pr02_id.staging_id + split_prs = env['runbot_merge.split'].search([]).batch_ids.prs + assert split_prs == pr02_id, \ + f"only the unstaged PR {pr02_id} should be in a split, found {split_prs}" + @pytest.mark.skip(reason="Maybe nothing to do, the PR is just skipped and put in error?") def test_batching_merge_failure(self): pass