From aa614c6077dd6b95b18d9b74b3625f64e2538079 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 5 Apr 2019 08:22:05 +0200 Subject: [PATCH] [IMP] runbot_merge: more reliable blocked attribute Use the proper / actual "is there any stageable PR" query to check if a PR is blocked as well, that way they shoudn't be diverging all the time even if it might make PR.blocked a bit more expensive. fixes #111 --- runbot_merge/models/pull_requests.py | 77 +++++++++++++-------------- runbot_merge/tests/local.py | 4 -- runbot_merge/tests/remote.py | 3 ++ runbot_merge/tests/test_multirepo.py | 78 ++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 42 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index d12aa67d..828a1449 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -258,24 +258,7 @@ class Branch(models.Model): for b in self: b.active_staging_id = b.staging_ids - def try_staging(self): - """ Tries to create a staging if the current branch does not already - have one. Returns None if the branch already has a staging or there - is nothing to stage, the newly created staging otherwise. - """ - logger = _logger.getChild('cron') - - logger.info( - "Checking %s (%s) for staging: %s, skip? %s", - self, self.name, - self.active_staging_id, - bool(self.active_staging_id) - ) - if self.active_staging_id: - return - - PRs = self.env['runbot_merge.pull_requests'] - + def _stageable(self): # noinspection SqlResolve self.env.cr.execute(""" SELECT @@ -283,7 +266,7 @@ class Branch(models.Model): array_agg(pr.id) AS match FROM runbot_merge_pull_requests pr LEFT JOIN runbot_merge_batch batch ON pr.batch_id = batch.id AND batch.active - WHERE pr.target = %s + WHERE pr.target = any(%s) -- exclude terminal states (so there's no issue when -- deleting branches & reusing labels) AND pr.state != 'merged' @@ -302,9 +285,29 @@ class Branch(models.Model): OR bool_and(pr.state = 'ready') ) ORDER BY min(pr.priority), min(pr.id) - """, [self.id]) - # result: [(priority, [(repo_id, pr_id) for repo in repos] - rows = self.env.cr.fetchall() + """, [self.ids]) + # result: [(priority, [pr_id for repo in repos])] + return self.env.cr.fetchall() + + def try_staging(self): + """ Tries to create a staging if the current branch does not already + have one. Returns None if the branch already has a staging or there + is nothing to stage, the newly created staging otherwise. + """ + logger = _logger.getChild('cron') + + logger.info( + "Checking %s (%s) for staging: %s, skip? %s", + self, self.name, + self.active_staging_id, + bool(self.active_staging_id) + ) + if self.active_staging_id: + return + + PRs = self.env['runbot_merge.pull_requests'] + + rows = self._stageable() priority = rows[0][0] if rows else -1 if priority == 0: # p=0 take precedence over all else @@ -440,23 +443,21 @@ class PullRequests(models.Model): " PR is linked to an other non-ready PR" ) - @property - def blocked(self): - if self.state not in ('ready', 'merged', 'closed'): - return True - if not (self.squash or self.merge_method): - return True + blocked = fields.Boolean( + compute='_compute_is_blocked', + help="PR is not currently stageable for some reason (mostly an issue if status is ready)" + ) - # can't be blocked on a co-dependent PR if it's a patch-* - if re.search(r':patch-\d+$', self.label): - return False - - return bool(self.search_count([ - ('id', '!=', self.id), - ('label', '=', self.label), - ('state', '!=', 'ready'), - ('priority', '!=', 0), - ])) + # missing link to other PRs + @api.depends('priority', 'state', 'squash', 'merge_method', 'batch_id.active', 'label') + def _compute_is_blocked(self): + stageable = { + pr_id + for _, pr_ids in self.mapped('target')._stageable() + for pr_id in pr_ids + } + for pr in self: + pr.blocked = pr.id not in stageable @api.depends('head') def _compute_statuses(self): diff --git a/runbot_merge/tests/local.py b/runbot_merge/tests/local.py index 24661427..6e215da9 100644 --- a/runbot_merge/tests/local.py +++ b/runbot_merge/tests/local.py @@ -9,10 +9,6 @@ import odoo import fake_github -@pytest.fixture(autouse=True) -def debuglog(caplog): - caplog.set_level(logging.DEBUG, logger='odoo') - @pytest.fixture(scope='session') def remote_p(): return False diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index f46086a1..94184350 100644 --- a/runbot_merge/tests/remote.py +++ b/runbot_merge/tests/remote.py @@ -438,6 +438,9 @@ class Model: return Model(self._env, self._model, {*self._ids, *other._ids}, fields=self._fields) + def invalidate_cache(self, fnames=None, ids=None): + pass # not a concern when every access is an RPC call + class Repo: __slots__ = ['name', '_session', '_tokens'] def __init__(self, session, name, user_tokens): diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 85695812..25019487 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -462,3 +462,81 @@ def test_urgent(env, repo_a, repo_b): 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 + +class TestBlocked: + def test_merge_method(self, env, repo_a): + make_branch(repo_a, 'master', 'initial', {'a0': 'a'}) + + pr = make_pr(repo_a, 'A', [{'a1': 'a'}, {'a2': 'a'}]) + + run_crons(env) + + p = to_pr(env, pr) + assert p.state == 'ready' + print(p.id, p.squash, p.merge_method) + assert p.blocked + + pr.post_comment('hansen rebase-merge', 'reviewer') + assert not p.blocked + + def test_linked_closed(self, env, repo_a, repo_b): + make_branch(repo_a, 'master', 'initial', {'a0': 'a'}) + make_branch(repo_b, 'master', 'initial', {'b0': 'b'}) + + pr = make_pr(repo_a, 'A', [{'a1': 'a'}], label='xxx') + b = make_pr(repo_b, 'B', [{'b1': 'b'}], label='xxx', statuses=[]) + run_crons(env) + + p = to_pr(env, pr) + assert p.blocked + b.close() + # FIXME: find a way for PR.blocked to depend on linked PR somehow so this isn't needed + p.invalidate_cache(['blocked'], [p.id]) + assert not p.blocked + + def test_linked_merged(self, env, repo_a, repo_b): + make_branch(repo_a, 'master', 'initial', {'a0': 'a'}) + make_branch(repo_b, 'master', 'initial', {'b0': 'b'}) + + b = make_pr(repo_b, 'B', [{'b1': 'b'}], label='xxx') + + run_crons(env) # stage b and c + + repo_a.post_status('heads/staging.master', 'success', 'legal/cla') + repo_a.post_status('heads/staging.master', 'success', 'ci/runbot') + repo_b.post_status('heads/staging.master', 'success', 'legal/cla') + repo_b.post_status('heads/staging.master', 'success', 'ci/runbot') + + run_crons(env) # merge b and c + assert to_pr(env, b).state == 'merged' + + pr = make_pr(repo_a, 'A', [{'a1': 'a'}], label='xxx') + run_crons(env) # merge b and c + + p = to_pr(env, pr) + assert not p.blocked + + def test_linked_unready(self, env, repo_a, repo_b): + """ Create a PR A linked to a non-ready PR B, + * A is blocked by default + * A is not blocked if A.p=0 + * A is not blocked if B.p=0 + """ + make_branch(repo_a, 'master', 'initial', {'a0': 'a'}) + make_branch(repo_b, 'master', 'initial', {'b0': 'b'}) + + a = make_pr(repo_a, 'A', [{'a1': 'a'}], label='xxx') + b = make_pr(repo_b, 'B', [{'b1': 'b'}], label='xxx', statuses=[]) + run_crons(env) + + pr_a = to_pr(env, a) + assert pr_a.blocked + + a.post_comment('hansen p=0', 'reviewer') + assert not pr_a.blocked + + a.post_comment('hansen p=2', 'reviewer') + assert pr_a.blocked + + b.post_comment('hansen p=0', 'reviewer') + assert not pr_a.blocked