NextERP/runbot
5
0
Fork 0
mirror of https://github.com/odoo/runbot.git synced 2025-03-15 15:35:46 +07:00
Code Issues Actions Packages Projects Releases Wiki Activity

[IMP] runbot_merge: automation around branch deactivation

Browse Source 
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
This commit is contained in:
Xavier Morel 2022-07-29 12:37:23 +02:00
parent e9278c021d
commit 0c882fc0df
6 changed files with 88 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
runbot_merge/changelog/2022-06/branch.md Normal file
View File

@ -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.

4
runbot_merge/controllers/__init__.py
View File

@ -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)

17
runbot_merge/models/pull_requests.py
View File

@ -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:

29
runbot_merge/tests/test_basic.py
View File

@ -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."),
]

105
runbot_merge/tests/test_disabled_branch.py
View File

@ -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')

3
runbot_merge/views/templates.xml
View File

@ -404,11 +404,14 @@
<t t-else="">open</t>
</t>
<t t-call="runbot_merge.view_pull_request_info_{{tmpl.strip()}}"/>
<t t-set="target_cls" t-value="None if pr.target.active else 'text-muted bg-warning'"/>
<dl class="runbot-merge-fields">
<dt>label</dt>
<dd><span t-field="pr.label"/></dd>
<dt>head</dt>
<dd><a t-attf-href="{{pr.github_url}}/commits/{{pr.head}}"><span t-field="pr.head"/></a></dd>
<dt t-att-class="target_cls">target</dt>
<dd t-att-class="target_cls"><span t-field="pr.target"/></dd>
</dl>
<p t-field="pr.message"/>
</div></div>