From dd0905f8d395cddf8026fddfe70cb7658808dd92 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 6 Oct 2020 12:43:57 +0200 Subject: [PATCH] [IMP] runbot_merge: handling of PRs on disabled branches Historically PRs to disabled branches were treated like PRs to un-managed branches: ignored. However because they cay *already exist* when the branch is disabled, the effects can be subtly different, and problematically so e.g. ignoring all PR events on PRs targeting disabled branches means we can't close them anymore, which is less than great. So don't ignore events on PRs to disabled branches (creation, sync, closing, and reopening) but also send feedback on PRs to disabled or un-managed branches to indicate that they're not merge-able. Fixes #410 --- runbot_merge/controllers/__init__.py | 33 ++-- runbot_merge/models/pull_requests.py | 2 +- runbot_merge/tests/test_basic.py | 42 +---- runbot_merge/tests/test_disabled_branch.py | 175 +++++++++++++++++++++ runbot_merge/tests/test_oddities.py | 42 ----- 5 files changed, 199 insertions(+), 95 deletions(-) create mode 100644 runbot_merge/tests/test_disabled_branch.py diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 5d11313e..c0e51088 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -84,12 +84,17 @@ def handle_pr(env, event): # PRs to unmanaged branches are not necessarily abnormal and # we don't care - # note: deactivated ~= unmanaged for new PRs - branch = env['runbot_merge.branch'].search([ + branch = env['runbot_merge.branch'].with_context(active_test=False).search([ ('name', '=', b), ('project_id', '=', repo.project_id.id), ]) + def feedback(**info): + return env['runbot_merge.pull_requests.feedback'].create({ + 'repository': repo.id, + 'pull_request': pr['number'], + **info, + }) def find(target): return env['runbot_merge.pull_requests'].search([ ('repository', '=', repo.id), @@ -101,10 +106,9 @@ def handle_pr(env, event): # handling must occur before the rest of the steps if event['action'] == 'edited': source = event['changes'].get('base', {'ref': {'from': b}})['ref']['from'] - source_branch = env['runbot_merge.branch'].search([ + source_branch = env['runbot_merge.branch'].with_context(active_test=False).search([ ('name', '=', source), ('project_id', '=', repo.project_id.id), - '|', ('active', '=', True), ('active', '=', False) ]) # retargeting to un-managed => delete if not branch: @@ -128,8 +132,17 @@ def handle_pr(env, event): return 'Updated {}'.format(pr_obj.id) return "Nothing to update ({})".format(event['changes'].keys()) + message = None + if not branch: + message = f"This PR targets the un-managed branch {r}:{b}, it can not 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." + if message and event['action'] not in ('synchronize', 'closed'): + feedback(message=message) + if not branch: - _logger.info("Ignoring PR for un-managed branch %s:%s", r, b) return "Not set up to care about {}:{}".format(r, b) author_name = pr['user']['login'] @@ -194,12 +207,10 @@ def handle_pr(env, event): if event['action'] == 'reopened' : if pr_obj.state == 'merged': - env['runbot_merge.pull_requests.feedback'].create({ - 'repository': pr_obj.repository.id, - 'pull_request': pr_obj.number, - 'close': True, - 'message': "@%s ya silly goose you can't reopen a PR that's been merged PR." % event['sender']['login'] - }) + feedback( + close=True, + message="@%s ya silly goose you can't reopen a PR that's been merged PR." % event['sender']['login'] + ) if pr_obj.state == 'closed': _logger.info('%s reopening %s', event['sender']['login'], pr_obj.display_name) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 0a431dd9..38c71db5 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1132,7 +1132,7 @@ class PullRequests(models.Model): ('name', '=', description['base']['repo']['full_name']), ]) if branch is None: - branch = self.env['runbot_merge.branch'].search([ + branch = self.env['runbot_merge.branch'].with_context(active_test=False).search([ ('name', '=', description['base']['ref']), ('project_id', '=', repo.project_id.id), ]) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 235dc248..b096641b 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -789,47 +789,6 @@ class TestPREdition: with repo: prx.base = 'master' - def test_retarget_from_disabled(self, env, repo): - """ Retargeting a PR from a disabled branch should not duplicate the PR - """ - branch_1 = env['runbot_merge.branch'].create({ - 'name': '1.0', - 'project_id': env['runbot_merge.project'].search([]).id, - }) - branch_2 = env['runbot_merge.branch'].create({ - 'name': '2.0', - 'project_id': env['runbot_merge.project'].search([]).id, - }) - - with repo: - c0 = repo.make_commit(None, '0', None, tree={'a': '0'}) - repo.make_ref('heads/1.0', c0) - c1 = repo.make_commit(c0, '1', None, tree={'a': '1'}) - repo.make_ref('heads/2.0', c1) - c2 = repo.make_commit(c1, '2', None, tree={'a': '2'}) - repo.make_ref('heads/master', c2) - - # create PR on 1.0 - c = repo.make_commit(c0, 'c', None, tree={'a': '0', 'b': '0'}) - prx = repo.make_pr(title='t', body='b', target='1.0', head=c) - # 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 == branch_2 - @pytest.mark.skip(reason="What do?") def test_edit_staged(env, repo): """ @@ -2835,6 +2794,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'], "I'm sorry. Branch `branch` is not within my remit."), ] diff --git a/runbot_merge/tests/test_disabled_branch.py b/runbot_merge/tests/test_disabled_branch.py new file mode 100644 index 00000000..132ddff4 --- /dev/null +++ b/runbot_merge/tests/test_disabled_branch.py @@ -0,0 +1,175 @@ +import pytest + +from utils import Commit + +def test_existing_pr_disabled_branch(env, project, make_repo, setreviewers, config, users): + """ PRs to disabled branches are ignored, but what if the PR exists *before* + the branch is disabled? + """ + repo = make_repo('repo') + project.branch_ids.sequence = 0 + project.write({'branch_ids': [ + (0, 0, {'name': 'other', 'sequence': 1}), + (0, 0, {'name': 'other2', 'sequence': 2}), + ]}) + repo_id = env['runbot_merge.repository'].create({ + 'project_id': project.id, + 'name': repo.name, + 'status_ids': [(0, 0, {'context': 'status'})] + }) + setreviewers(*project.repo_ids) + + with repo: + [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') + [ot] = repo.make_commits(m, Commit('other', tree={'b': '1'}), ref='heads/other') + repo.make_commits(m, Commit('other2', tree={'c': '1'}), ref='heads/other2') + + [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_id = env['runbot_merge.pull_requests'].search([ + ('repository', '=', repo_id.id), + ('number', '=', pr.number), + ]) + + # 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']) + env.run_crons() + + # nothing should happen, the PR should be unstaged forever, maybe? + assert pr_id.state == 'ready' + assert not pr_id.staging_id + + 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+"), + (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') + pr.post_comment('hansen rebase-ff r+', config['role_reviewer']['token']) + env.run_crons() + + assert pr_id.state == 'ready' + assert pr_id.target == env['runbot_merge.branch'].search([('name', '=', 'other2')]) + assert pr_id.staging_id + + +def test_new_pr_no_branch(env, project, make_repo, setreviewers, users): + """ A new PR to an *unknown* branch should be ignored and warn + """ + repo = make_repo('repo') + repo_id = env['runbot_merge.repository'].create({ + 'project_id': project.id, + 'name': repo.name, + 'status_ids': [(0, 0, {'context': 'status'})] + }) + setreviewers(*project.repo_ids) + + with repo: + [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') + [ot] = repo.make_commits(m, Commit('other', tree={'b': '1'}), ref='heads/other') + + [c] = repo.make_commits(ot, Commit('wheee', tree={'b': '2'})) + pr = repo.make_pr(title="title", body='body', target='other', head=c) + env.run_crons() + + assert not env['runbot_merge.pull_requests'].search([ + ('repository', '=', repo_id.id), + ('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), + ] + +def test_new_pr_disabled_branch(env, project, make_repo, setreviewers, users): + """ A new PR to a *disabled* branch should be accepted (rather than ignored) + but should warn + """ + repo = make_repo('repo') + repo_id = env['runbot_merge.repository'].create({ + 'project_id': project.id, + 'name': repo.name, + 'status_ids': [(0, 0, {'context': 'status'})] + }) + env['runbot_merge.branch'].create({ + 'project_id': project.id, + 'name': 'other', + 'active': False, + }) + setreviewers(*project.repo_ids) + + with repo: + [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') + [ot] = repo.make_commits(m, Commit('other', tree={'b': '1'}), ref='heads/other') + + [c] = repo.make_commits(ot, Commit('wheee', tree={'b': '2'})) + pr = repo.make_pr(title="title", body='body', target='other', head=c) + env.run_crons() + + pr_id = env['runbot_merge.pull_requests'].search([ + ('repository', '=', repo_id.id), + ('number', '=', pr.number), + ]) + 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), + ] + +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/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index 4a19065c..6d6e6b98 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -89,45 +89,3 @@ def test_override(env, project, make_repo, users, setreviewers, config): 'description': 'Overridden by @{}'.format(users['other']), }} -def test_disabled_branch(env, project, make_repo, users, setreviewers, config): - """ PRs to disabled branches are ignored, but what if the PR exists *before* - the branch is disabled? - """ - repo = make_repo('repo') - project.branch_ids.sequence = 0 - project.write({'branch_ids': [ - (0, 0, {'name': 'other', 'sequence': 1}), - (0, 0, {'name': 'other2', 'sequence': 2}), - ]}) - repo_id = env['runbot_merge.repository'].create({ - 'project_id': project.id, - 'name': repo.name, - 'status_ids': [(0, 0, {'context': 'status'})] - }) - setreviewers(*project.repo_ids) - - with repo: - [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') - [ot] = repo.make_commits(m, Commit('other', tree={'b': '1'}), ref='heads/other') - repo.make_commits(m, Commit('other2', tree={'c': '1'}), ref='heads/other2') - - [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_id = env['runbot_merge.pull_requests'].search([ - ('repository', '=', repo_id.id), - ('number', '=', pr.number), - ]) - - # 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']) - env.run_crons() - - # nothing should happen, the PR should be unstaged forever, maybe? - assert pr_id.state == 'ready' - assert not pr_id.staging_id