From b2a4da4739c6411645304d1aba8de59d5e8133ff Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 28 May 2020 13:10:55 +0200 Subject: [PATCH] [FIX] runbot_merge: update squash flag on reopen While the head gets updated (properly), the squash flag did not which could lead to odd results. Since a PR can only be reopened if it was regular-pushed to (not after a force push) there are two scenarios: * the PR updated to have 0 commits, closed, pushed to with one commit then reopened, after reopening the PR would be marked as !squash and would ask for a merge method (that's what happened with odoo/odoo#51763) * the PR has a single commit, is closed, pushed to then reopened, after reopening the PR would still be marked a squash and potentially straight rebased without asking for a merge method Nothing would break per-se but both scenarios are undesirable. Close #373 --- runbot_merge/controllers/__init__.py | 1 + runbot_merge/tests/test_basic.py | 62 +++++++++++++++------------- 2 files changed, 35 insertions(+), 28 deletions(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 7972e194..ab52d7ea 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -207,6 +207,7 @@ def handle_pr(env, event): 'state': 'opened', # updating the head triggers a revalidation 'head': pr['head']['sha'], + 'squash': pr['commits'] == 1, }) return 'Reopened {}'.format(pr_obj.id) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index e59cccb1..3a1e4b53 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -935,34 +935,6 @@ def test_ci_failure_after_review(env, repo, users, config): (users['user'], "'ci/runbot' failed on this reviewed PR.".format_map(users)), ] -def test_reopen_state(env, repo): - """ The PR should be validated on opening and reopening in case there's - already a CI+ stored (as the CI might never trigger unless explicitly - re-requested) - """ - with repo: - m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) - repo.make_ref('heads/master', m) - - c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'}) - repo.post_status(c, 'success', 'legal/cla') - repo.post_status(c, 'success', 'ci/runbot') - prx = repo.make_pr(title='title', body='body', target='master', head=c) - - pr = env['runbot_merge.pull_requests'].search([ - ('repository.name', '=', repo.name), - ('number', '=', prx.number), - ]) - assert pr.state == 'validated', \ - "if a PR is created on a CI'd commit, it should be validated immediately" - - with repo: prx.close() - assert pr.state == 'closed' - - with repo: prx.open() - assert pr.state == 'validated', \ - "if a PR is reopened and had a CI'd head, it should be validated immediately" - def test_reopen_merged_pr(env, repo, config, users): """ Reopening a *merged* PR should cause us to immediately close it again, and insult whoever did it @@ -2147,6 +2119,10 @@ class TestPRUpdate(object): ('repository.name', '=', repo.name), ('number', '=', prx.number) ]) + assert pr.state == 'opened' + assert pr.head == c + assert pr.squash + with repo: prx.close() @@ -2156,11 +2132,41 @@ class TestPRUpdate(object): assert pr.state == 'closed' assert pr.head == c + assert pr.squash with repo: prx.open() assert pr.state == 'opened' assert pr.head == c2 + assert not pr.squash + + def test_update_closed_revalidate(self, env, repo): + """ The PR should be validated on opening and reopening in case there's + already a CI+ stored (as the CI might never trigger unless explicitly + re-requested) + """ + with repo: + m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) + repo.make_ref('heads/master', m) + + c = repo.make_commit(m, 'fist', None, tree={'m': 'c1'}) + repo.post_status(c, 'success', 'legal/cla') + repo.post_status(c, 'success', 'ci/runbot') + prx = repo.make_pr(title='title', body='body', target='master', head=c) + + pr = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', prx.number), + ]) + assert pr.state == 'validated', \ + "if a PR is created on a CI'd commit, it should be validated immediately" + + with repo: prx.close() + assert pr.state == 'closed' + + with repo: prx.open() + assert pr.state == 'validated', \ + "if a PR is reopened and had a CI'd head, it should be validated immediately" @pytest.mark.xfail(reason="github doesn't allow reopening force-pushed PRs", strict=True) def test_force_update_closed(self, env, repo):