mirror of
https://github.com/odoo/runbot.git
synced 2025-03-15 23:45:44 +07:00
[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
This commit is contained in:
parent
c61c1c74a0
commit
dd0905f8d3
@ -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)
|
||||
|
@ -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),
|
||||
])
|
||||
|
@ -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."),
|
||||
]
|
||||
|
||||
|
175
runbot_merge/tests/test_disabled_branch.py
Normal file
175
runbot_merge/tests/test_disabled_branch.py
Normal file
@ -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')
|
@ -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
|
||||
|
Loading…
Reference in New Issue
Block a user