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):