From 0b629a32bc2ebddddaa6860234dc827b3c776f22 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 15 Oct 2018 16:19:29 +0200 Subject: [PATCH] [ADD] runbot_merge: warning that a ready PR is linked to a non-ready one People get surprised/worried that their ready PR never gets picked up, but it's because there is a non-ready (either unreviewed or failing CI) pull request linked to it. This aims to at least warn them of the issue. --- runbot_merge/data/merge_cron.xml | 10 +++ runbot_merge/models/pull_requests.py | 51 +++++++++++- runbot_merge/tests/remote.py | 4 + runbot_merge/tests/test_multirepo.py | 117 ++++++++++++++++++++------- 4 files changed, 152 insertions(+), 30 deletions(-) diff --git a/runbot_merge/data/merge_cron.xml b/runbot_merge/data/merge_cron.xml index af230f24..c3e225a3 100644 --- a/runbot_merge/data/merge_cron.xml +++ b/runbot_merge/data/merge_cron.xml @@ -29,4 +29,14 @@ -1 + + Warn on linked PRs where only one is ready + + code + model._check_linked_prs_statuses(True) + 1 + hours + -1 + + diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index b0adb3db..89597e07 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -352,7 +352,7 @@ class PullRequests(models.Model): # staged? ('merged', 'Merged'), ('error', 'Error'), - ], default='opened') + ], default='opened', index=True) number = fields.Integer(required=True, index=True) author = fields.Many2one('res.partner') @@ -379,6 +379,11 @@ class PullRequests(models.Model): batch_ids = fields.Many2many('runbot_merge.batch') staging_id = fields.Many2one(related='batch_id.staging_id', store=True) + link_warned = fields.Boolean( + default=False, help="Whether we've already warned that this (ready)" + " PR is linked to an other non-ready PR" + ) + @api.depends('head') def _compute_statuses(self): Commits = self.env['runbot_merge.commit'] @@ -635,6 +640,50 @@ class PullRequests(models.Model): }) return super().unlink() + def _check_linked_prs_statuses(self, commit=False): + """ Looks for linked PRs where at least one of the PRs is in a ready + state and the others are not, notifies the other PRs. + + :param bool commit: whether to commit the tnx after each comment + """ + # similar to Branch.try_staging's query as it's a subset of that + # other query's behaviour + self.env.cr.execute(""" + SELECT + array_agg(pr.id) AS match + FROM runbot_merge_pull_requests pr + WHERE + -- exclude terminal states (so there's no issue when + -- deleting branches & reusing labels) + pr.state != 'merged' + AND pr.state != 'closed' + GROUP BY pr.label + HAVING + -- one of the batch's PRs should be ready & not marked + bool_or(pr.state = 'ready' AND NOT pr.link_warned) + -- one of the others should be unready + AND bool_or(pr.state != 'ready') + -- but ignore batches with one of the prs at p- + AND bool_and(pr.priority != 0) + """) + for [ids] in self.env.cr.fetchall(): + prs = self.browse(ids) + ready = prs.filtered(lambda p: p.state == 'ready') + unready = prs - ready + + for r in ready: + r.repository.github().comment( + r.number, "Linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format( + ', '.join(map( + '{0.repository.name}#{0.number}'.format, + unready + )) + ) + ) + r.link_warned = True + if commit: + self.env.cr.commit() + # state changes on reviews RPLUS = { 'opened': 'approved', diff --git a/runbot_merge/tests/remote.py b/runbot_merge/tests/remote.py index ba50a557..dff70c0b 100644 --- a/runbot_merge/tests/remote.py +++ b/runbot_merge/tests/remote.py @@ -348,6 +348,10 @@ class Model: assert self._model == 'runbot_merge.project' self._run_cron('runbot_merge.fetch_prs_cron') + def _check_linked_prs_statuses(self): + assert self._model == 'runbot_merge.pull_requests' + self._run_cron('runbot_merge.check_linked_prs_status') + def _run_cron(self, xid): _, model, cron_id = self._env('ir.model.data', 'xmlid_lookup', xid) assert model == 'ir.cron', "Expected {} to be a cron, got {}".format(xid, model) diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index 37856e72..91ed8e36 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -216,39 +216,98 @@ def test_ff_fail(env, project, repo_a, repo_b): assert len(st) == 1 assert len(st.batch_ids.prs) == 2 -def test_one_failed(env, project, repo_a, repo_b, owner): - """ If the companion of a ready branch-matched PR is not ready, - they should not get staged - """ - project.batch_limit = 1 - c_a = repo_a.make_commit(None, 'initial', None, tree={'a': 'a_0'}) - repo_a.make_ref('heads/master', c_a) - repo_a.protect('master') - # pr_a is born ready - pr_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing') +class TestCompanionsNotReady: + def test_one_pair(self, env, project, repo_a, repo_b, owner, users): + """ If the companion of a ready branch-matched PR is not ready, + they should not get staged + """ + project.batch_limit = 1 + make_branch(repo_a, 'master', 'initial', {'a': 'a_0'}) + # pr_a is born ready + p_a = make_pr(repo_a, 'A', [{'a': 'a_1'}], label='do-a-thing') - c_b = repo_b.make_commit(None, 'initial', None, tree={'a': 'b_0'}) - repo_b.make_ref('heads/master', c_b) - repo_b.protect('master') - c_pr = repo_b.make_commit(c_b, 'pr', None, tree={'a': 'b_1'}) - pr_b = repo_b.make_pr( - 'title', 'body', target='master', ctid=c_pr, - user='user', label='do-a-thing', - ) - repo_b.post_status(c_pr, 'success', 'ci/runbot') - repo_b.post_status(c_pr, 'success', 'legal/cla') + make_branch(repo_b, 'master', 'initial', {'a': 'b_0'}) + p_b = make_pr(repo_b, 'B', [{'a': 'b_1'}], label='do-a-thing', reviewer=None) - pr_a = to_pr(env, pr_a) - pr_b = to_pr(env, pr_b) - assert pr_a.state == 'ready' - assert pr_b.state == 'validated' - assert pr_a.label == pr_b.label == '{}:do-a-thing'.format(owner) + pr_a = to_pr(env, p_a) + pr_b = to_pr(env, p_b) + assert pr_a.state == 'ready' + assert pr_b.state == 'validated' + assert pr_a.label == pr_b.label == '{}:do-a-thing'.format(owner) - env['runbot_merge.project']._check_progress() + env['runbot_merge.project']._check_progress() - assert not pr_b.staging_id - assert not pr_a.staging_id, \ - "pr_a should not have been staged as companion is not ready" + assert not pr_b.staging_id + assert not pr_a.staging_id, \ + "pr_a should not have been staged as companion is not ready" + + env['runbot_merge.pull_requests']._check_linked_prs_statuses() + assert p_a.comments == [ + (users['reviewer'], 'hansen r+'), + (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)), + ] + # ensure the message is only sent once per PR + env['runbot_merge.pull_requests']._check_linked_prs_statuses() + assert p_a.comments == [ + (users['reviewer'], 'hansen r+'), + (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)), + ] + assert p_b.comments == [] + + def test_two_of_three_unready(self, env, project, repo_a, repo_b, repo_c, owner, users): + """ In a 3-batch, if two of the PRs are not ready both should be + linked by the first one + """ + project.batch_limit = 1 + make_branch(repo_a, 'master', 'initial', {'f': 'a0'}) + pr_a = make_pr(repo_a, 'A', [{'f': 'a1'}], label='a-thing', reviewer=None) + + make_branch(repo_b, 'master', 'initial', {'f': 'b0'}) + pr_b = make_pr(repo_b, 'B', [{'f': 'b1'}], label='a-thing') + + make_branch(repo_c, 'master', 'initial', {'f': 'c0'}) + pr_c = make_pr(repo_c, 'C', [{'f': 'c1'}], label='a-thing', reviewer=None) + + env['runbot_merge.pull_requests']._check_linked_prs_statuses() + assert pr_a.comments == [] + assert pr_b.comments == [ + (users['reviewer'], 'hansen r+'), + (users['user'], "Linked pull request(s) %s#%d, %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( + repo_a.name, pr_a.number, + repo_c.name, pr_c.number + )) + ] + assert pr_c.comments == [] + + def test_one_of_three_unready(self, env, project, repo_a, repo_b, repo_c, owner, users): + """ In a 3-batch, if one PR is not ready it should be linked on the + other two + """ + project.batch_limit = 1 + make_branch(repo_a, 'master', 'initial', {'f': 'a0'}) + pr_a = make_pr(repo_a, 'A', [{'f': 'a1'}], label='a-thing', reviewer=None) + + make_branch(repo_b, 'master', 'initial', {'f': 'b0'}) + pr_b = make_pr(repo_b, 'B', [{'f': 'b1'}], label='a-thing') + + make_branch(repo_c, 'master', 'initial', {'f': 'c0'}) + pr_c = make_pr(repo_c, 'C', [{'f': 'c1'}], label='a-thing') + + env['runbot_merge.pull_requests']._check_linked_prs_statuses() + assert pr_a.comments == [] + assert pr_b.comments == [ + (users['reviewer'], 'hansen r+'), + (users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( + repo_a.name, pr_a.number + )) + ] + assert pr_c.comments == [ + (users['reviewer'], 'hansen r+'), + (users['user'], + "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % ( + repo_a.name, pr_a.number + )) + ] def test_other_failed(env, project, repo_a, repo_b, owner, users): """ In a non-matched-branch scenario, if the companion staging (copy of