[IMP] runbot_merge: prioritize p0 more

* p0 cancel existing stagings in order to be staged as soon as
  possible
* p0 PRs should be picked over split batches
* p0 bypass PR-level CI and review requirements
* p0 can be set on any of a batch's PR, matched PRs will be staged
  alongside even if their priority is the default

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']

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

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

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