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