diff --git a/runbot_merge/controllers.py b/runbot_merge/controllers.py index e29423aa..965033e3 100644 --- a/runbot_merge/controllers.py +++ b/runbot_merge/controllers.py @@ -132,16 +132,11 @@ def handle_pr(event): pr_obj.state = 'opened' elif pr_obj.state == 'ready': pr_obj.state = 'approved' - if pr_obj.staging_id: - _logger.info( - "Updated PR %s:%s, removing staging %s", - pr_obj.repository.name, pr_obj.number, - pr_obj.staging_id, - ) - # immediately cancel the staging? - staging = pr_obj.staging_id - staging.batch_ids.unlink() - staging.unlink() + pr_obj.staging_id.cancel( + "Updated PR %s:%s, removing staging %s", + pr_obj.repository.name, pr_obj.number, + pr_obj.staging_id, + ) # TODO: should we update squash as well? What of explicit squash commands? pr_obj.head = pr['head']['sha'] diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 99188d1a..86836875 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -113,39 +113,42 @@ class Project(models.Model): if branch.active_staging_id: continue - # Splits can generate inactive stagings, restage these first - if branch.staging_ids: + self.env.cr.execute(""" + SELECT + min(pr.priority) as priority, + array_agg(pr.id) AS match + FROM runbot_merge_pull_requests pr + WHERE pr.target = %s + AND pr.batch_id IS NULL + -- exclude terminal states (so there's no issue when + -- deleting branches & reusing labels) + AND pr.state != 'merged' + AND pr.state != 'closed' + GROUP BY pr.label + HAVING bool_or(pr.priority = 0) + OR bool_and(pr.state = 'ready') + ORDER BY min(pr.priority), min(pr.id) + """, [branch.id]) + # result: [(priority, [(repo_id, pr_id) for repo in repos] + rows = self.env.cr.fetchall() + priority = rows[0][0] if rows else -1 + if priority == 0: + # p=0 take precedence over all else + batches = [ + PRs.browse(pr_ids) + for _, pr_ids in takewhile(lambda r: r[0] == priority, rows) + ] + elif branch.staging_ids: + # Splits can generate inactive stagings, restage these first staging = branch.staging_ids[0] logger.info("Found inactive staging %s, reactivating", staging) batches = [batch.prs for batch in staging.batch_ids] staging.unlink() - else: - self.env.cr.execute(""" - SELECT - min(pr.priority) as priority, - array_agg(pr.id) AS match - FROM runbot_merge_pull_requests pr - WHERE pr.target = %s - AND pr.batch_id IS NULL - -- exclude terminal states (so there's no issue when - -- deleting branches & reusing labels) - AND pr.state != 'merged' - AND pr.state != 'closed' - GROUP BY pr.label - HAVING every(pr.state = 'ready') - ORDER BY min(pr.priority), min(pr.id) - """, [branch.id]) - # result: [(priority, [(repo_id, pr_id) for repo in repos] - rows = self.env.cr.fetchall() - logger.info( - "Looking for PRs to stage for %s... %s", - branch.name, rows - ) - if not rows: - continue - - priority = rows[0][0] + elif rows: + # p=1 or p=2 batches = [PRs.browse(pr_ids) for _, pr_ids in takewhile(lambda r: r[0] == priority, rows)] + else: + continue staged = Batch meta = {repo: {} for repo in project.repo_ids} @@ -454,6 +457,12 @@ class PullRequests(models.Model): if is_admin: ok = True self.priority = param + self.target.active_staging_id.cancel( + "P=0 on %s:%s by %s, unstaging %s", + self.repository.name, self.number, + author.github_login, self.target.name, + ) + _logger.info( "%s %s(%s) on %s:%s by %s (%s)", "applied" if ok else "ignored", @@ -585,6 +594,14 @@ class Stagings(models.Model): assert v == 'success' s.state = st + def cancel(self, reason, *args): + if not self: + return + + _logger.info(reason, *args) + self.batch_ids.unlink() + self.unlink() + def fail(self, message, prs=None): _logger.error("Staging %s failed: %s", self, message) prs = prs or self.batch_ids.prs diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index e834215c..f657877d 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -813,13 +813,20 @@ class TestPRUpdate(object): assert not env['runbot_merge.pull_requests'].search([('number', '=', prx.number)]) class TestBatching(object): - def _pr(self, repo, prefix, trees, target='master', user='user'): + def _pr(self, repo, prefix, trees, *, + target='master', user='user', reviewer='reviewer', + statuses=(('ci/runbot', 'success'), ('legal/cla', 'success')) + ): """ Helper creating a PR from a series of commits on a base :param prefix: a prefix used for commit messages, PR title & PR body :param trees: a list of dicts symbolising the tree for the corresponding commit. each tree is an update on the "current state" of the tree :param target: branch, both the base commit and the PR target + :type target: str + :type user: str + :type reviewer: str | None + :type statuses: List[(str, str)] """ base = repo.commit('heads/{}'.format(target)) tree = dict(repo.objects[base.tree]) @@ -828,9 +835,11 @@ class TestBatching(object): tree.update(t) c = repo.make_commit(c, 'commit_{}_{:02}'.format(prefix, i), None, tree=dict(tree)) pr = repo.make_pr('title {}'.format(prefix), 'body {}'.format(prefix), target=target, ctid=c, user=user, label='{}:{}'.format(user, prefix)) - repo.post_status(c, 'success', 'ci/runbot') - repo.post_status(c, 'success', 'legal/cla') - pr.post_comment('hansen r+', 'reviewer') + + for context, result in statuses: + repo.post_status(c, result, context) + if reviewer: + pr.post_comment('hansen r+', reviewer) return pr def _get(self, env, number): @@ -885,10 +894,6 @@ class TestBatching(object): assert pr11.staging_id == pr12.staging_id def test_batching_urgent(self, env, repo): - """ "Urgent" PRs should be selected before pressing & normal & batched together (?) - - TODO: should they also ignore validation aka immediately staged? - """ m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'}) repo.make_ref('heads/master', m) @@ -900,27 +905,64 @@ class TestBatching(object): pr11.post_comment('hansen priority=1', 'reviewer') pr12.post_comment('hansen priority=1', 'reviewer') - pr01 = self._pr(repo, 'Urgent 1', [{'n': 'n'}, {'o': 'o'}]) - pr02 = self._pr(repo, 'Urgent 2', [{'p': 'p'}, {'q': 'q'}]) - pr01.post_comment('hansen priority=0', 'reviewer') - pr02.post_comment('hansen priority=0', 'reviewer') + # stage PR1 + env['runbot_merge.project']._check_progress() + p_11, p_12, p_21, p_22 = \ + [self._get(env, pr.number) for pr in [pr11, pr12, pr21, pr22]] + assert not p_21.staging_id or p_22.staging_id + assert p_11.staging_id and p_12.staging_id + assert p_11.staging_id == p_12.staging_id + staging_1 = p_11.staging_id - pr01, pr02, pr11, pr12, pr21, pr22 = prs = \ - [self._get(env, pr.number) for pr in [pr01, pr02, pr11, pr12, pr21, pr22]] - assert pr01.priority == pr02.priority == 0 - assert pr11.priority == pr12.priority == 1 - assert pr21.priority == pr22.priority == 2 + # no statuses run on PR0s + pr01 = self._pr(repo, 'Urgent 1', [{'n': 'n'}, {'o': 'o'}], reviewer=None, statuses=[]) + pr01.post_comment('hansen priority=0', 'reviewer') + p_01 = self._get(env, pr01.number) + assert p_01.state == 'opened' + assert p_01.priority == 0 env['runbot_merge.project']._check_progress() + # first staging should be cancelled and PR0 should be staged + # regardless of CI (or lack thereof) + assert not staging_1.exists() + assert not p_11.staging_id and not p_12.staging_id + assert p_01.staging_id + + def test_batching_urgenter_than_split(self, env, repo): + """ p=0 PRs should take priority over split stagings (processing + of a staging having CI-failed and being split into sub-stagings) + """ + m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'}) + repo.make_ref('heads/master', m) + + pr1 = self._pr(repo, 'PR1', [{'a': 'AAA'}, {'b': 'BBB'}]) + p_1 = self._get(env, pr1.number) + pr2 = self._pr(repo, 'PR2', [{'a': 'some_content', 'c': 'CCC'}, {'d': 'DDD'}]) + p_2 = self._get(env, pr2.number) + + env['runbot_merge.project']._check_progress() + st = env['runbot_merge.stagings'].search([]) + # both prs should be part of the staging + assert st.mapped('batch_ids.prs') == p_1 | p_2 + # add CI failure + repo.post_status('heads/staging.master', 'failure', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + + env['runbot_merge.project']._check_progress() + # should have staged the first half + assert p_1.staging_id.heads + assert not p_2.staging_id.heads + + # during restaging of pr1, create urgent PR + pr0 = self._pr(repo, 'urgent', [{'a': 'a', 'b': 'b'}], reviewer=None, statuses=[]) + pr0.post_comment('hansen priority=0', 'reviewer') + + env['runbot_merge.project']._check_progress() + # TODO: maybe just deactivate stagings instead of deleting them when canceling? + assert not p_1.staging_id + assert self._get(env, pr0.number).staging_id + - assert all(pr.state == 'ready' for pr in prs) - assert pr01.staging_id - assert pr02.staging_id - assert pr01.staging_id == pr02.staging_id - assert not pr11.staging_id - assert not pr12.staging_id - assert not pr21.staging_id - assert not pr22.staging_id @pytest.mark.skip(reason="Maybe nothing to do, the PR is just skipped and put in error?") def test_batching_merge_failure(self, env, repo): @@ -932,28 +974,16 @@ class TestBatching(object): m = repo.make_commit(None, 'initial', None, tree={'a': 'some content'}) repo.make_ref('heads/master', m) - c10 = repo.make_commit(m, 'AAA', None, tree={'a': 'AAA'}) - c11 = repo.make_commit(c10, 'BBB', None, tree={'a': 'AAA', 'b': 'BBB'}) - pr1 = repo.make_pr('t1', 'b1', target='master', ctid=c11, user='user', label='user:a') - repo.post_status(pr1.head, 'success', 'ci/runbot') - repo.post_status(pr1.head, 'success', 'legal/cla') - pr1.post_comment('hansen r+', "reviewer") - - c20 = repo.make_commit(m, 'CCC', None, tree={'a': 'some content', 'c': 'CCC'}) - c21 = repo.make_commit(c20, 'DDD', None, tree={'a': 'some content', 'c': 'CCC', 'd': 'DDD'}) - pr2 = repo.make_pr('t2', 'b2', target='master', ctid=c21, user='user', label='user:b') - repo.post_status(pr2.head, 'success', 'ci/runbot') - repo.post_status(pr2.head, 'success', 'legal/cla') - pr2.post_comment('hansen r+', "reviewer") + pr1 = self._pr(repo, 'PR1', [{'a': 'AAA'}, {'b': 'BBB'}]) + pr2 = self._pr(repo, 'PR2', [{'a': 'some_content', 'c': 'CCC'}, {'d': 'DDD'}]) env['runbot_merge.project']._check_progress() st = env['runbot_merge.stagings'].search([]) # both prs should be part of the staging assert len(st.mapped('batch_ids.prs')) == 2 # add CI failure - h = repo.commit('heads/staging.master').id - repo.post_status(h, 'failure', 'ci/runbot') - repo.post_status(h, 'success', 'legal/cla') + repo.post_status('heads/staging.master', 'failure', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') pr1 = env['runbot_merge.pull_requests'].search([('number', '=', pr1.number)]) pr2 = env['runbot_merge.pull_requests'].search([('number', '=', pr2.number)]) diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 0d03ab35..edeefb14 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -11,8 +11,6 @@ import odoo import pytest -from fake_github import git - @pytest.fixture def project(env): env['res.partner'].create({ @@ -54,7 +52,20 @@ def repo_c(gh, project): ((odoo.http.root, '/runbot_merge/hooks'), ['pull_request', 'issue_comment', 'status']) ]) -def make_pr(repo, prefix, trees, target='master', user='user', label=None): +def make_pr(repo, prefix, trees, *, target='master', user='user', label=None, + statuses=(('ci/runbot', 'success'), ('legal/cla', 'success')), + reviewer='reviewer'): + """ + :type repo: fake_github.Repo + :type prefix: str + :type trees: list[dict] + :type target: str + :type user: str + :type label: str | None + :type statuses: list[(str, str)] + :type reviewer: str | None + :rtype: fake_github.PR + """ base = repo.commit('heads/{}'.format(target)) tree = dict(repo.objects[base.tree]) c = base.id @@ -64,9 +75,10 @@ def make_pr(repo, prefix, trees, target='master', user='user', label=None): tree=dict(tree)) pr = repo.make_pr('title {}'.format(prefix), 'body {}'.format(prefix), target=target, ctid=c, user=user, label=label and '{}:{}'.format(user, label)) - repo.post_status(c, 'success', 'ci/runbot') - repo.post_status(c, 'success', 'legal/cla') - pr.post_comment('hansen r+', 'reviewer') + for context, result in statuses: + repo.post_status(c, result, context) + if reviewer: + pr.post_comment('hansen r+', reviewer) return pr def to_pr(env, pr): return env['runbot_merge.pull_requests'].search([ @@ -344,3 +356,25 @@ def test_batching_split(env, repo_a, repo_b): assert len(st2.batch_ids) == 2 assert st2.mapped('batch_ids.prs') == \ prs[0][0] | prs[0][1] | prs[1][1] + +def test_urgent(env, repo_a, repo_b): + """ Either PR of a co-dependent pair being p=0 leads to the entire pair + being prioritized + """ + repo_a.make_ref('heads/master', repo_a.make_commit(None, 'initial', None, tree={'a0': 'a'})) + repo_b.make_ref('heads/master', repo_b.make_commit(None, 'initial', None, tree={'b0': 'b'})) + + pr_a = make_pr(repo_a, 'A', [{'a1': 'a'}, {'a2': 'a'}], label='batch', reviewer=None, statuses=[]) + pr_b = make_pr(repo_b, 'B', [{'b1': 'b'}, {'b2': 'b'}], label='batch', reviewer=None, statuses=[]) + pr_c = make_pr(repo_a, 'C', [{'c1': 'c', 'c2': 'c'}]) + + pr_b.post_comment('hansen p=0', 'reviewer') + + env['runbot_merge.project']._check_progress() + # should have batched pr_a and pr_b despite neither being reviewed or + # approved + p_a, p_b = to_pr(env, pr_a), to_pr(env, pr_b) + p_c = to_pr(env, pr_c) + assert p_a.batch_id and p_b.batch_id and p_a.batch_id == p_b.batch_id,\ + "a and b should have been recognised as co-dependent" + assert not p_c.staging_id