From cfc7478fcfcdcfa0275405077365d9e9ff1e1f8e Mon Sep 17 00:00:00 2001 From: xmo-odoo Date: Wed, 31 Jul 2019 09:20:02 +0200 Subject: [PATCH] [FIX] runbot_merge: PR splits should be updated on PR state change On a PR being updated, closed or unreviewed, if it is part of an active staging that staging would get cancelled (yay). However, if the PR was part of a pending *split*, then the split would *not* get cancelled / updated (to remove the PR from it), and the PR could go on to get staged as if everything were right in the world which is an issue. It doesn't look like it actually happened (at least I got no echo of it), but it almost did at least once. fixes #160 --- runbot_merge/controllers/__init__.py | 4 +- runbot_merge/models/pull_requests.py | 23 ++++++-- runbot_merge/tests/remote.py | 4 +- runbot_merge/tests/test_basic.py | 86 ++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 9 deletions(-) diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 3a0ce2ad..e1ba8337 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -153,7 +153,7 @@ def handle_pr(env, event): return "It's my understanding that closed/merged PRs don't get sync'd" if pr_obj.state == 'ready': - pr_obj.staging_id.cancel( + pr_obj.unstage( "PR %s:%s updated by %s", pr_obj.repository.name, pr_obj.number, event['sender']['login'] @@ -201,7 +201,7 @@ def handle_pr(env, event): 'state_from': res[1] if not pr_obj.staging_id else 'staged', 'state_to': 'closed', }) - pr_obj.staging_id.cancel( + pr_obj.unstage( "PR %s:%s closed by %s", pr_obj.repository.name, pr_obj.number, event['sender']['login'] diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index b6cafcda..ca867775 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -698,11 +698,7 @@ class PullRequests(models.Model): newstate = RMINUS.get(self.state) if newstate: self.state = newstate - if self.staging_id: - self.staging_id.cancel( - "unreview (r-) by %s", - author.github_login - ) + self.unstage("unreview (r-) by %s", author.github_login) ok = True else: msg = "r- makes no sense in the current PR state." @@ -1040,6 +1036,23 @@ class PullRequests(models.Model): self.commits_map = json.dumps(commits_map) return merge_head + def unstage(self, reason, *args): + """ If the PR is staged, cancel the staging. If the PR is split and + waiting, remove it from the split (possibly delete the split entirely) + """ + split_batches = self.with_context(active_test=False).mapped('batch_ids').filtered('split_id') + if len(split_batches) > 1: + _logger.warn("Found a PR linked with more than one split batch: %s (%s)", self, split_batches) + for b in split_batches: + if len(b.split_id.batch_ids) == 1: + # only the batch of this PR -> delete split + b.split_id.unlink() + else: + # else remove this batch from the split + b.split_id = False + + self.staging_id.cancel(reason, *args) + # state changes on reviews RPLUS = { 'opened': 'approved', diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index 94184350..795b64c5 100644 --- a/runbot_merge/tests/remote.py +++ b/runbot_merge/tests/remote.py @@ -348,8 +348,8 @@ class Model: ids = self._env(self._model, 'exists', self._ids) return Model(self._env, self._model, ids) - def search(self, domain): - ids = self._env(self._model, 'search', domain) + def search(self, domain, **kw): + ids = self._env(self._model, 'search', domain, **kw) return Model(self._env, self._model, ids) def create(self, values): diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index c19e0f29..fa5686d5 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -1633,6 +1633,49 @@ class TestPRUpdate(object): assert not pr.staging_id assert not env['runbot_merge.stagings'].search([]) + def test_split(self, env, repo): + """ Should remove the PR from its split, and possibly delete the split + entirely. + """ + m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) + repo.make_ref('heads/master', m) + + c = repo.make_commit(m, 'first', None, tree={'m': 'm', '1': '1'}) + prx1 = repo.make_pr('t1', 'b1', target='master', ctid=c, user='user', label='p1') + repo.post_status(prx1.head, 'success', 'legal/cla') + repo.post_status(prx1.head, 'success', 'ci/runbot') + prx1.post_comment('hansen r+', user='reviewer') + + c = repo.make_commit(m, 'first', None, tree={'m': 'm', '2': '2'}) + prx2 = repo.make_pr('t2', 'b2', target='master', ctid=c, user='user', label='p2') + repo.post_status(prx2.head, 'success', 'legal/cla') + repo.post_status(prx2.head, 'success', 'ci/runbot') + prx2.post_comment('hansen r+', user='reviewer') + + run_crons(env) + + pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number') + assert pr1.number == prx1.number + assert pr2.number == prx2.number + assert pr1.staging_id == pr2.staging_id + s0 = pr1.staging_id + + repo.post_status('heads/staging.master', 'failure', 'ci/runbot') + run_crons(env) + + assert pr1.staging_id and pr1.staging_id != s0, "pr1 should have been re-staged" + assert not pr2.staging_id, "pr2 should not" + # TODO: remote doesn't currently handle env context so can't mess + # around using active_test=False + assert env['runbot_merge.split'].search([]) + + prx2.push(repo.make_commit(c, 'second', None, tree={'m': 'm', '2': '22'})) + # probably not necessary ATM but... + run_crons(env) + + assert pr2.state == 'opened', "state should have been reset" + assert not env['runbot_merge.split'].search([]), "there should be no split left" + def test_update_error(self, env, repo): m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) repo.make_ref('heads/master', m) @@ -2404,6 +2447,49 @@ class TestRMinus: assert pr.staging_id == st assert pr.state == 'ready' + def test_split(self, env, repo): + """ Should remove the PR from its split, and possibly delete the split + entirely. + """ + m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) + repo.make_ref('heads/master', m) + + c = repo.make_commit(m, 'first', None, tree={'m': 'm', '1': '1'}) + prx1 = repo.make_pr('t1', 'b1', target='master', ctid=c, user='user', label='p1') + repo.post_status(prx1.head, 'success', 'legal/cla') + repo.post_status(prx1.head, 'success', 'ci/runbot') + prx1.post_comment('hansen r+', user='reviewer') + + c = repo.make_commit(m, 'first', None, tree={'m': 'm', '2': '2'}) + prx2 = repo.make_pr('t2', 'b2', target='master', ctid=c, user='user', label='p2') + repo.post_status(prx2.head, 'success', 'legal/cla') + repo.post_status(prx2.head, 'success', 'ci/runbot') + prx2.post_comment('hansen r+', user='reviewer') + + run_crons(env) + + pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number') + assert pr1.number == prx1.number + assert pr2.number == prx2.number + assert pr1.staging_id == pr2.staging_id + s0 = pr1.staging_id + + repo.post_status('heads/staging.master', 'failure', 'ci/runbot') + run_crons(env) + + assert pr1.staging_id and pr1.staging_id != s0, "pr1 should have been re-staged" + assert not pr2.staging_id, "pr2 should not" + # TODO: remote doesn't currently handle env context so can't mess + # around using active_test=False + assert env['runbot_merge.split'].search([]) + + # prx2 was actually a terrible idea! + prx2.post_comment('hansen r-', user='reviewer') + # probably not necessary ATM but... + run_crons(env) + + assert pr2.state == 'validated', "state should have been reset" + assert not env['runbot_merge.split'].search([]), "there should be no split left" class TestComments: def test_address_method(self, repo, env):