From 62fbda52a8aa7b2ea6df0226c1b0bfbf27bb79cd Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 3 Dec 2024 14:52:12 +0100 Subject: [PATCH] [IMP] runbot_merge: a PR can't be reopened if its batch is merged In that case, ignore the reopen, close the PR, and tell the idiot to fuck off. Also the case where a PR is reopened while its batch is staged was already handled, but probably not tested: it was implicit in forcefully updating the HEAD of the PR, which triggers an unstage since c8a06601a7c223cdfc9da23f2c799b4c660f274b. Now that scenario is tested, which should lower the odds of breaking it in the future. Fixes #965 --- runbot_merge/controllers/__init__.py | 13 ++- ..._merge.pull_requests.feedback.template.csv | 7 +- runbot_merge/tests/test_batch_consistency.py | 109 +++++++++++++++++- 3 files changed, 120 insertions(+), 9 deletions(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 60692c3f..7fe99d9d 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -334,16 +334,17 @@ def handle_pr(env, event): return 'Ignored: could not lock rows (probably being merged)' if event['action'] == 'reopened' : - if pr_obj.state == 'merged': - feedback( - close=True, - message=env.ref('runbot_merge.handle.pr.merged')._format(event=event), - ) + if pr_obj.merge_date: + if pr_obj.state == 'merged': + message = env.ref('runbot_merge.handle.pr.merged')._format(event=event) + else: + message = env.ref('runbot_merge.handle.pr.mergedbatch')._format(event=event) + feedback(close=True, message=message) elif pr_obj.closed: _logger.info('%s reopening %s', event['sender']['login'], pr_obj.display_name) pr_obj.write({ 'closed': False, - # updating the head triggers a revalidation + # updating the head triggers a revalidation, and unstages the batch 'head': pr['head']['sha'], 'squash': pr['commits'] == 1, }) diff --git a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv index b140ec43..543459de 100644 --- a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv +++ b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv @@ -11,14 +11,17 @@ branch: branch (ref) name event: complete pr event" runbot_merge.handle.pr.merged,@{event[sender][login]} ya silly goose you can't reopen a merged PR.,"Notifies that a user tried to reopen a merged PR. -Event: complete PR event" +event: complete PR event" +runbot_merge.handle.pr.mergedbatch,"Reopening a PR in a merged batch is not allowed, create a new PR.","Notifies that a user tried to reopen a closed PR whose batch is merged. + +event: complete PR event" runbot_merge.pr.load.unmanaged,"Branch `{pr[base][ref]}` is not within my remit, imma just ignore it.","Notifies that a user tried to load a PR targeting a non-handled branch. pr: pull request (github object) Repository: repository object (???)" runbot_merge.pr.load.fetched,"{ping}I didn't know about this PR and had to retrieve its information, you may have to re-approve it as I didn't see previous commands.","Notifies that we did retrieve an unknown PR (either by request or as side effect of an interaction). -Pr: pr object we just created" +pr: pr object we just created" runbot_merge.pr.branch.disabled,"{pr.ping}the target branch {pr.target.name!r} has been disabled, you may want to close this PR.","Notifies that the target branch for this PR was deactivated. pr: pull request in question" diff --git a/runbot_merge/tests/test_batch_consistency.py b/runbot_merge/tests/test_batch_consistency.py index 0cc3b8b0..d5516786 100644 --- a/runbot_merge/tests/test_batch_consistency.py +++ b/runbot_merge/tests/test_batch_consistency.py @@ -3,7 +3,7 @@ without wider relevance and thus other location. """ import pytest -from utils import Commit, to_pr, pr_page +from utils import Commit, to_pr, pr_page, seen def test_close_single(env, repo): @@ -199,3 +199,110 @@ Inconsistent targets: env.run_crons() assert env['runbot_merge.stagings'].search_count([]) + +def test_reopen_pr_in_staged_batch(env, project, make_repo2, config): + """Reopening a closed PR from a staged batch should cancel the staging + """ + repo1 = make_repo2('a') + repo2 = make_repo2('b') + + with repo1: + [m1, _] = repo1.make_commits( + None, + Commit('a', tree={'a': 'a'}), + Commit('b', tree={'b': 'b'}), + ref='heads/p', + ) + repo1.make_ref('heads/master', m1) + pr1 = repo1.make_pr(target='master', head='p') + with repo2: + [m2, _] = repo2.make_commits( + None, + Commit('a', tree={'a': 'a'}), + Commit('b', tree={'b': 'b'}), + ref='heads/p', + ) + repo2.make_ref('heads/master', m2) + pr2 = repo2.make_pr(target='master', head='p') + + pr1_id = to_pr(env, pr1) + pr2_id = to_pr(env, pr2) + batch_id = pr1_id.batch_id + assert batch_id + assert batch_id == pr2_id.batch_id + + with repo1: + repo1.post_status(pr1.head, 'success') + pr1.post_comment("hansen r+", config['role_reviewer']['token']) + with repo2: + pr2.close() + + env.run_crons(None) + + assert pr2_id.state == 'closed' + assert batch_id.staging_ids.filtered(lambda s: s.active) + + with repo2: + pr2.open() + assert pr2_id.state == 'opened' + assert not batch_id.staging_ids.filtered(lambda s: s.active) + assert batch_id.blocked + +def test_reopen_pr_in_merged_batch(env, project, make_repo2, config, users): + """If the batch is merged, the pr should just be re-closed with a message + """ + repo1 = make_repo2('a') + repo2 = make_repo2('b') + + with repo1: + [m1, _] = repo1.make_commits( + None, + Commit('a', tree={'a': 'a'}), + Commit('b', tree={'b': 'b'}), + ref='heads/p', + ) + repo1.make_ref('heads/master', m1) + pr1 = repo1.make_pr(target='master', head='p') + with repo2: + [m2, _] = repo2.make_commits( + None, + Commit('a', tree={'a': 'a'}), + Commit('b', tree={'b': 'b'}), + ref='heads/p', + ) + repo2.make_ref('heads/master', m2) + pr2 = repo2.make_pr(target='master', head='p') + + pr1_id = to_pr(env, pr1) + pr2_id = to_pr(env, pr2) + batch_id = pr1_id.batch_id + assert batch_id + assert batch_id == pr2_id.batch_id + + with repo1: + repo1.post_status(pr1.head, 'success') + pr1.post_comment("hansen r+", config['role_reviewer']['token']) + with repo2: + pr2.close() + + env.run_crons(None) + + with repo1, repo2: + repo1.post_status('staging.master', 'success') + repo2.post_status('staging.master', 'success') + env.run_crons(None) + + + assert pr1_id.state == 'merged' + assert pr2_id.state == 'closed' + + with repo2: + pr2.open() + env.run_crons(None) + + assert pr2_id.closed + assert pr2_id.state == 'closed' + assert pr2.comments == [ + seen(env, pr2, users), + (users['user'], 'Reopening a PR in a merged batch is not allowed, create a new PR.'), + ]