From 0c882fc0df3f342851ed2399d34565c4477c3b55 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 29 Jul 2022 12:37:23 +0200 Subject: [PATCH] [IMP] runbot_merge: automation around branch deactivation Currently deactivating a branch kinda leaves users in the dark, with little way to know what has happened aside from inferring it from the branch having disappeared from the main dashboard. - surface the state of the branch in the PR dashboard (also surface the target branch at all so users can see if their PR is targeted as they expect as far as the mergebot is concerned) - close & notify every PR to a branch being deactivated - cancel any current staging to the branch (as a consequence of the above) Closes #632 --- runbot_merge/changelog/2022-06/branch.md | 4 + runbot_merge/controllers/__init__.py | 4 +- runbot_merge/models/pull_requests.py | 17 ++++ runbot_merge/tests/test_basic.py | 29 ++++-- runbot_merge/tests/test_disabled_branch.py | 105 ++++++++------------- runbot_merge/views/templates.xml | 3 + 6 files changed, 88 insertions(+), 74 deletions(-) create mode 100644 runbot_merge/changelog/2022-06/branch.md diff --git a/runbot_merge/changelog/2022-06/branch.md b/runbot_merge/changelog/2022-06/branch.md new file mode 100644 index 00000000..6c7c302b --- /dev/null +++ b/runbot_merge/changelog/2022-06/branch.md @@ -0,0 +1,4 @@ +IMP: automatically close PRs when their target branch is deactivated + +Leave a message on the PRs to explain, such PRs should also be reopen-able if +the users wants to retarget them. diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 3b0b02c0..7e134af3 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -145,11 +145,11 @@ def handle_pr(env, event): message = None if not branch: - message = f"This PR targets the un-managed branch {r}:{b}, it can not be merged." + message = f"This PR targets the un-managed branch {r}:{b}, it needs to be retargeted before it can be merged." _logger.info("Ignoring event %s on PR %s#%d for un-managed branch %s", event['action'], r, pr['number'], b) elif not branch.active: - message = f"This PR targets the disabled branch {r}:{b}, it can not be merged." + message = f"This PR targets the disabled branch {r}:{b}, it needs to be retargeted before it can be merged." if message and event['action'] not in ('synchronize', 'closed'): feedback(message=message) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 5ced85e1..33a087b6 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -249,6 +249,23 @@ class Branch(models.Model): self._table, ['name', 'project_id']) return res + @api.depends('active') + def _compute_display_name(self): + super()._compute_display_name() + for b in self.filtered(lambda b: not b.active): + b.display_name += ' (inactive)' + + def write(self, vals): + super().write(vals) + if vals.get('active') is False: + self.env['runbot_merge.pull_requests.feedback'].create([{ + 'repository': pr.repository.id, + 'pull_request': pr.number, + 'close': True, + 'message': f'{pr.ping()}the target branch {pr.target.name!r} has been disabled, closing this PR.', + } for pr in self.prs]) + return True + @api.depends('staging_ids.active') def _compute_active_staging(self): for b in self: diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 5049fcda..f039b606 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -6,7 +6,7 @@ import time from unittest import mock import pytest -from lxml import html +from lxml import html, etree import odoo from utils import _simple_init, seen, re_matches, get_partner, Commit, pr_page, to_pr, part_of @@ -26,21 +26,34 @@ def repo(env, project, make_repo, users, setreviewers): def test_trivial_flow(env, repo, page, users, config): # create base branch with repo: - m = repo.make_commit(None, "initial", None, tree={'a': 'some content'}) - repo.make_ref('heads/master', m) + [m] = repo.make_commits(None, Commit("initial", tree={'a': 'some content'}), ref='heads/master') # create PR with 2 commits - c0 = repo.make_commit(m, 'replace file contents', None, tree={'a': 'some other content'}) - c1 = repo.make_commit(c0, 'add file', None, tree={'a': 'some other content', 'b': 'a second file'}) - pr = repo.make_pr(title="gibberish", body="blahblah", target='master', head=c1,) + _, c1 = repo.make_commits( + m, + Commit('replace file contents', tree={'a': 'some other content'}), + Commit('add file', tree={'b': 'a second file'}), + ref='heads/other' + ) + pr = repo.make_pr(title="gibberish", body="blahblah", target='master', head='other') pr_id = to_pr(env, pr) assert pr_id.state == 'opened' env.run_crons() assert pr.comments == [seen(env, pr, users)] - s = pr_page(page, pr).cssselect('.alert-info > ul > li') + + pr_dashboard = pr_page(page, pr) + s = pr_dashboard.cssselect('.alert-info > ul > li') assert [it.get('class') for it in s] == ['fail', 'fail', ''],\ "merge method unset, review missing, no CI" + assert dict(zip( + [e.text_content() for e in pr_dashboard.cssselect('dl.runbot-merge-fields dt')], + [e.text_content() for e in pr_dashboard.cssselect('dl.runbot-merge-fields dd')], + )) == { + 'label': f"{config['github']['owner']}:other", + 'head': c1, + 'target': 'master', + } with repo: repo.post_status(c1, 'success', 'legal/cla') @@ -3190,7 +3203,7 @@ class TestUnknownPR: assert prx.comments == [ (users['reviewer'], 'hansen r+'), - (users['user'], "This PR targets the un-managed branch %s:branch, it can not be merged." % repo.name), + (users['user'], "This PR targets the un-managed branch %s:branch, it needs to be retargeted before it can be merged." % repo.name), (users['user'], "Branch `branch` is not within my remit, imma just ignore it."), ] diff --git a/runbot_merge/tests/test_disabled_branch.py b/runbot_merge/tests/test_disabled_branch.py index eaaca7c4..1d6be201 100644 --- a/runbot_merge/tests/test_disabled_branch.py +++ b/runbot_merge/tests/test_disabled_branch.py @@ -1,6 +1,6 @@ -from utils import seen, Commit +from utils import seen, Commit, pr_page -def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, config, users): +def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, config, users, page): """ PRs to disabled branches are ignored, but what if the PR exists *before* the branch is disabled? """ @@ -13,7 +13,8 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf repo_id = env['runbot_merge.repository'].create({ 'project_id': project.id, 'name': repo.name, - 'status_ids': [(0, 0, {'context': 'status'})] + 'status_ids': [(0, 0, {'context': 'status'})], + 'group_id': False, }) setreviewers(*project.repo_ids) @@ -25,42 +26,57 @@ def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, conf [c] = repo.make_commits(ot, Commit('wheee', tree={'b': '2'})) pr = repo.make_pr(title="title", body='body', target='other', head=c) repo.post_status(c, 'success', 'status') + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() pr_id = env['runbot_merge.pull_requests'].search([ ('repository', '=', repo_id.id), ('number', '=', pr.number), ]) + branch_id = pr_id.target + assert pr_id.staging_id + staging_id = branch_id.active_staging_id + assert staging_id == pr_id.staging_id # disable branch "other" - project.branch_ids.filtered(lambda b: b.name == 'other').active = False - - # r+ the PR - with repo: - pr.post_comment('hansen r+', config['role_reviewer']['token']) + branch_id.active = False env.run_crons() - # nothing should happen, the PR should be unstaged forever, maybe? - assert pr_id.state == 'ready' + assert not branch_id.active_staging_id + assert staging_id.state == 'cancelled', \ + "closing the PRs should have canceled the staging" + + p = pr_page(page, pr) + target = dict(zip( + (e.text for e in p.cssselect('dl.runbot-merge-fields dt')), + (p.cssselect('dl.runbot-merge-fields dd')) + ))['target'] + assert target.text_content() == 'other (inactive)' + assert target.get('class') == 'text-muted bg-warning' + + # the PR should have been closed implicitly + assert pr_id.state == 'closed' assert not pr_id.staging_id + with repo: + pr.open() + pr.post_comment('hansen r+', config['role_reviewer']['token']) + assert pr_id.state == 'ready', "pr should be reopenable" + env.run_crons() + + assert pr.comments == [ + (users['reviewer'], "hansen r+"), + seen(env, pr, users), + (users['user'], "@%(user)s @%(reviewer)s the target branch 'other' has been disabled, closing this PR." % users), + (users['reviewer'], "hansen r+"), + (users['user'], "This PR targets the disabled branch %s:other, it needs to be retargeted before it can be merged." % repo.name), + ] + with repo: [c2] = repo.make_commits(ot, Commit('wheee', tree={'b': '3'})) repo.update_ref(pr.ref, c2, force=True) assert pr_id.head == c2, "pr should be aware of its update" - with repo: - pr.close() - assert pr_id.state == 'closed', "pr should be closeable" - with repo: - pr.open() - assert pr_id.state == 'opened', "pr should be reopenable (state reset due to force push" - env.run_crons() - assert pr.comments == [ - (users['reviewer'], "hansen r+"), - seen(env, pr, users), - (users['user'], "This PR targets the disabled branch %s:other, it can not be merged." % repo.name), - ], "reopening a PR to an inactive branch should send feedback, but not closing it" - with repo: pr.base = 'other2' repo.post_status(c2, 'success', 'status') @@ -96,7 +112,7 @@ def test_new_pr_no_branch(env, project, make_repo, setreviewers, users): ('number', '=', pr.number), ]), "the PR should not have been created in the backend" assert pr.comments == [ - (users['user'], "This PR targets the un-managed branch %s:other, it can not be merged." % repo.name), + (users['user'], "This PR targets the un-managed branch %s:other, it needs to be retargeted before it can be merged." % repo.name), ] def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users): @@ -131,45 +147,6 @@ def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users): assert pr_id, "the PR should have been created in the backend" assert pr_id.state == 'opened' assert pr.comments == [ - (users['user'], "This PR targets the disabled branch %s:other, it can not be merged." % repo.name), + (users['user'], "This PR targets the disabled branch %s:other, it needs to be retargeted before it can be merged." % repo.name), seen(env, pr, users), ] - -def test_retarget_from_disabled(env, make_repo, project, setreviewers): - """ Retargeting a PR from a disabled branch should not duplicate the PR - """ - repo = make_repo('repo') - project.write({'branch_ids': [(0, 0, {'name': '1.0'}), (0, 0, {'name': '2.0'})]}) - repo_id = env['runbot_merge.repository'].create({ - 'project_id': project.id, - 'name': repo.name, - 'required_statuses': 'legal/cla,ci/runbot', - }) - setreviewers(repo_id) - - with repo: - [c0] = repo.make_commits(None, Commit('0', tree={'a': '0'}), ref='heads/1.0') - [c1] = repo.make_commits(c0, Commit('1', tree={'a': '1'}), ref='heads/2.0') - repo.make_commits(c1, Commit('2', tree={'a': '2'}), ref='heads/master') - - # create PR on 1.0 - repo.make_commits(c0, Commit('c', tree={'a': '0', 'b': '0'}), ref='heads/changes') - prx = repo.make_pr(head='changes', target='1.0') - branch_1 = project.branch_ids.filtered(lambda b: b.name == '1.0') - # there should only be a single PR in the system at this point - [pr] = env['runbot_merge.pull_requests'].search([]) - assert pr.target == branch_1 - - # branch 1 is EOL, disable it - branch_1.active = False - - with repo: - # we forgot we had active PRs for it, and we may want to merge them - # still, retarget them! - prx.base = '2.0' - - # check that we still only have one PR in the system - [pr_] = env['runbot_merge.pull_requests'].search([]) - # and that it's the same as the old one, just edited with a new target - assert pr_ == pr - assert pr.target == project.branch_ids.filtered(lambda b: b.name == '2.0') diff --git a/runbot_merge/views/templates.xml b/runbot_merge/views/templates.xml index 52ca2eed..0e87428d 100644 --- a/runbot_merge/views/templates.xml +++ b/runbot_merge/views/templates.xml @@ -404,11 +404,14 @@ open +
label
head
+
target
+