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

[IMP] *: review mergebot & forwardbot messages for pinging

Browse Source 
Old messages were quite inconsistent in their pinging of the PR author
and reviewer.

Reviewed messages (probably missed some but...) and try to more
consistently ping when the feedback requires some sort of action in
order to proceed.

Fixes #592
This commit is contained in:
Xavier Morel 2022-06-23 14:25:07 +02:00
parent 4a3cde2faa
commit f430c014c1
14 changed files with 245 additions and 194 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
conftest.py
View File

@ -582,17 +582,6 @@ class Repo:
parents=[p['sha'] for p in gh_commit['parents']],
)
def log(self, ref_or_sha):
for page in itertools.count(1):
r = self._session.get(
'https://api.github.com/repos/{}/commits'.format(self.name),
params={'sha': ref_or_sha, 'page': page}
)
assert 200 <= r.status_code < 300, r.json()
yield from map(self._commit_from_gh, r.json())
if not r.links.get('next'):
return
def read_tree(self, commit):
""" read tree object from commit

16
forwardport/models/forwardport.py
View File

@ -79,12 +79,12 @@ class ForwardPortTasks(models.Model, Queue):
batch.active = False
CONFLICT_TEMPLATE = "WARNING: the latest change ({previous.head}) triggered " \
CONFLICT_TEMPLATE = "{ping}WARNING: the latest change ({previous.head}) triggered " \
"a conflict when updating the next forward-port " \
"({next.display_name}), and has been ignored.\n\n" \
"You will need to update this pull request differently, " \
"or fix the issue by hand on {next.display_name}."
CHILD_CONFLICT = "WARNING: the update of {previous.display_name} to " \
CHILD_CONFLICT = "{ping}WARNING: the update of {previous.display_name} to " \
"{previous.head} has caused a conflict in this pull request, " \
"data may have been lost."
class UpdateQueue(models.Model, Queue):
@ -118,14 +118,15 @@ class UpdateQueue(models.Model, Queue):
Feedback.create({
'repository': child.repository.id,
'pull_request': child.number,
'message': "Ancestor PR %s has been updated but this PR"
'message': "%sancestor PR %s has been updated but this PR"
" is %s and can't be updated to match."
"\n\n"
"You may want or need to manually update any"
" followup PR." % (
self.new_root.display_name,
child.state,
)
child.ping(),
self.new_root.display_name,
child.state,
)
})
return
@ -137,6 +138,7 @@ class UpdateQueue(models.Model, Queue):
'repository': previous.repository.id,
'pull_request': previous.number,
'message': CONFLICT_TEMPLATE.format(
ping=previous.ping(),
previous=previous,
next=child
)
@ -144,7 +146,7 @@ class UpdateQueue(models.Model, Queue):
Feedback.create({
'repository': child.repository.id,
'pull_request': child.number,
'message': CHILD_CONFLICT.format(previous=previous, next=child)\
'message': CHILD_CONFLICT.format(ping=child.ping(), previous=previous, next=child)\
+ (f'\n\nstdout:\n```\n{out.strip()}\n```' if out.strip() else '')
+ (f'\n\nstderr:\n```\n{err.strip()}\n```' if err.strip() else '')
})

107
forwardport/models/project.py
View File

@ -261,8 +261,11 @@ class PullRequests(models.Model):
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': p.repository.id,
'pull_request': p.number,
'message': "This PR was modified / updated and has become a normal PR. "
"It should be merged the normal way (via @%s)" % p.repository.project_id.github_prefix,
'message': "%sthis PR was modified / updated and has become a normal PR. "
"It should be merged the normal way (via @%s)" % (
p.source_id.ping(),
p.repository.project_id.github_prefix,
),
'token_field': 'fp_github_token',
})
if vals.get('state') == 'merged':
@ -298,7 +301,6 @@ class PullRequests(models.Model):
)
return
Feedback = self.env['runbot_merge.pull_requests.feedback']
# TODO: don't use a mutable tokens iterator
tokens = iter(tokens)
while True:
@ -306,6 +308,7 @@ class PullRequests(models.Model):
if token is None:
break
ping = False
close = False
msg = None
if token in ('ci', 'skipci'):
@ -314,7 +317,8 @@ class PullRequests(models.Model):
pr.fw_policy = token
msg = "Not waiting for CI to create followup forward-ports." if token == 'skipci' else "Waiting for CI to create followup forward-ports."
else:
msg = "I don't trust you enough to do that @{}.".format(login)
ping = True
msg = "you can't configure ci."
if token == 'ignore': # replace 'ignore' by 'up to <pr_branch>'
token = 'up'
@ -322,55 +326,51 @@ class PullRequests(models.Model):
if token in ('r+', 'review+'):
if not self.source_id:
Feedback.create({
'repository': self.repository.id,
'pull_request': self.number,
'message': "I'm sorry, @{}. I can only do this on forward-port PRs and this ain't one.".format(login),
'token_field': 'fp_github_token',
})
continue
merge_bot = self.repository.project_id.github_prefix
# don't update the root ever
for pr in (p for p in self._iter_ancestors() if p.parent_id if p.state in RPLUS):
# only the author is delegated explicitely on the
pr._parse_commands(author, {**comment, 'body': merge_bot + ' r+'}, login)
ping = True
msg = "I can only do this on forward-port PRs and this is not one, see {}.".format(
self.repository.project_id.github_prefix
)
else:
merge_bot = self.repository.project_id.github_prefix
# don't update the root ever
for pr in (p for p in self._iter_ancestors() if p.parent_id if p.state in RPLUS):
# only the author is delegated explicitely on the
pr._parse_commands(author, {**comment, 'body': merge_bot + ' r+'}, login)
elif token == 'close':
msg = "I'm sorry, @{}. I can't close this PR for you.".format(
login)
if self.source_id._pr_acl(author).is_reviewer:
close = True
msg = None
else:
ping = True
msg = "you can't close PRs."
elif token == 'up' and next(tokens, None) == 'to':
limit = next(tokens, None)
ping = True
if not self._pr_acl(author).is_author:
Feedback.create({
'repository': self.repository.id,
'pull_request': self.number,
'message': "I'm sorry, @{}. You can't set a forward-port limit.".format(login),
'token_field': 'fp_github_token',
})
continue
if not limit:
msg = "Please provide a branch to forward-port to."
msg = "you can't set a forward-port limit.".format(login)
elif not limit:
msg = "please provide a branch to forward-port to."
else:
limit_id = self.env['runbot_merge.branch'].with_context(active_test=False).search([
('project_id', '=', self.repository.project_id.id),
('name', '=', limit),
])
if self.source_id:
msg = "Sorry, forward-port limit can only be set on " \
msg = "forward-port limit can only be set on " \
f"an origin PR ({self.source_id.display_name} " \
"here) before it's merged and forward-ported."
elif self.state in ['merged', 'closed']:
msg = "Sorry, forward-port limit can only be set before the PR is merged."
msg = "forward-port limit can only be set before the PR is merged."
elif not limit_id:
msg = "There is no branch %r, it can't be used as a forward port target." % limit
msg = "there is no branch %r, it can't be used as a forward port target." % limit
elif limit_id == self.target:
ping = False
msg = "Forward-port disabled."
self.limit_id = limit_id
elif not limit_id.fp_enabled:
msg = "Branch %r is disabled, it can't be used as a forward port target." % limit_id.name
msg = "branch %r is disabled, it can't be used as a forward port target." % limit_id.name
else:
ping = False
msg = "Forward-porting to %r." % limit_id.name
self.limit_id = limit_id
@ -382,7 +382,7 @@ class PullRequests(models.Model):
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': self.repository.id,
'pull_request': self.number,
'message': msg,
'message': f'@{author.github_login} {msg}' if msg and ping else msg,
'close': close,
'token_field': 'fp_github_token',
})
@ -397,8 +397,8 @@ class PullRequests(models.Model):
'repository': self.repository.id,
'pull_request': self.number,
'token_field': 'fp_github_token',
'message': '%s\n\n%s failed on this forward-port PR' % (
self.source_id._pingline(),
'message': '%s%s failed on this forward-port PR' % (
self.source_id.ping(),
ci,
)
})
@ -578,10 +578,10 @@ class PullRequests(models.Model):
'repository': pr.repository.id,
'pull_request': pr.number,
'token_field': 'fp_github_token',
'message': "This pull request can not be forward ported: "
'message': "%sthis pull request can not be forward ported: "
"next branch is %r but linked pull request %s "
"has a next branch %r." % (
t.name, linked.display_name, other.name
pr.ping(), t.name, linked.display_name, other.name
)
})
_logger.warning(
@ -678,8 +678,8 @@ class PullRequests(models.Model):
'delegates': [(6, False, (source.delegates | pr.delegates).ids)]
})
if has_conflicts and pr.parent_id and pr.state not in ('merged', 'closed'):
message = source._pingline() + """
The next pull request (%s) is in conflict. You can merge the chain up to here by saying
message = source.ping() + """\
the next pull request (%s) is in conflict. You can merge the chain up to here by saying
> @%s r+
%s""" % (new_pr.display_name, pr.repository.project_id.fp_github_name, footer)
self.env['runbot_merge.pull_requests.feedback'].create({
@ -710,19 +710,21 @@ The next pull request (%s) is in conflict. You can merge the chain up to here by
'* %s%s\n' % (sha, ' <- on this commit' if sha == h else '')
for sha in hh
)
message = f"""{source._pingline()} cherrypicking of pull request {source.display_name} failed.
message = f"""{source.ping()}cherrypicking of pull request {source.display_name} failed.
{lines}{sout}{serr}
Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).
In the former case, you may want to edit this PR message as well.
"""
elif has_conflicts:
message = """%s
While this was properly forward-ported, at least one co-dependent PR (%s) did not succeed. You will need to fix it before this can be merged.
message = """%s\
while this was properly forward-ported, at least one co-dependent PR (%s) did \
not succeed. You will need to fix it before this can be merged.
Both this PR and the others will need to be approved via `@%s r+` as they are all considered "in conflict".
Both this PR and the others will need to be approved via `@%s r+` as they are \
all considered "in conflict".
%s""" % (
source._pingline(),
source.ping(),
', '.join(p.display_name for p in (new_batch - new_pr)),
proj.github_prefix,
footer
@ -733,8 +735,8 @@ Both this PR and the others will need to be approved via `@%s r+` as they are al
for p in pr._iter_ancestors()
if p.parent_id
)
message = source._pingline() + """
This PR targets %s and is the last of the forward-port chain%s
message = source.ping() + """\
this PR targets %s and is the last of the forward-port chain%s
%s
To merge the full chain, say
> @%s r+
@ -772,14 +774,6 @@ This PR targets %s and is part of the forward-port chain. Further PRs will be cr
b.prs[0]._schedule_fp_followup()
return b
def _pingline(self):
assignees = (self.author | self.reviewed_by).mapped('github_login')
return "Ping %s" % ', '.join(
'@' + login
for login in assignees
if login
)
def _create_fp_branch(self, target_branch, fp_branch_name, cleanup):
""" Creates a forward-port for the current PR to ``target_branch`` under
``fp_branch_name``.
@ -1084,8 +1078,9 @@ stderr:
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': source.repository.id,
'pull_request': source.number,
'message': "This pull request has forward-port PRs awaiting action (not merged or closed): %s" % ', '.join(
pr.display_name for pr in sorted(prs, key=lambda p: p.number)
'message': "%sthis pull request has forward-port PRs awaiting action (not merged or closed):\n%s" % (
source.ping(),
'\n- '.join(pr.display_name for pr in sorted(prs, key=lambda p: p.number))
),
'token_field': 'fp_github_token',
})

11
forwardport/tests/test_conflicts.py
View File

@ -73,8 +73,8 @@ This PR targets b and is part of the forward-port chain. Further PRs will be cre
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
'''),
(users['user'], """Ping @%s, @%s
The next pull request (%s) is in conflict. You can merge the chain up to here by saying
(users['user'], """@%s @%s the next pull request (%s) is in conflict. \
You can merge the chain up to here by saying
> @%s r+
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
@ -343,11 +343,12 @@ b
assert pr2.comments == [
seen(env, pr2, users),
(users['user'], re_matches(r'Ping.*CONFLICT', re.DOTALL)),
(users['user'], re_matches(r'@%s @%s .*CONFLICT' % (users['user'], users['reviewer']), re.DOTALL)),
(users['reviewer'], 'hansen r+'),
(users['user'], f"All commits must have author and committer email, "
(users['user'], f"@{users['user']} @{users['reviewer']} unable to stage: "
"All commits must have author and committer email, "
f"missing email on {pr2_id.head} indicates the "
f"authorship is most likely incorrect."),
"authorship is most likely incorrect."),
]
assert pr2_id.state == 'error'
assert not pr2_id.staging_id, "staging should have been rejected"

33
forwardport/tests/test_limit.py
View File

@ -1,5 +1,6 @@
# -*- coding: utf-8 -*-
import collections
import time
import pytest
@ -157,12 +158,12 @@ def test_disable(env, config, make_repo, users, enabled):
# responses and we don't care that much
assert set(pr.comments) == {
(users['reviewer'], "hansen r+\n%s up to" % bot_name),
(users['other'], "@%s please provide a branch to forward-port to." % users['reviewer']),
(users['reviewer'], "%s up to b" % bot_name),
(users['other'], "@%s branch 'b' is disabled, it can't be used as a forward port target." % users['reviewer']),
(users['reviewer'], "%s up to foo" % bot_name),
(users['other'], "@%s there is no branch 'foo', it can't be used as a forward port target." % users['reviewer']),
(users['reviewer'], "%s up to c" % bot_name),
(users['other'], "Please provide a branch to forward-port to."),
(users['other'], "Branch 'b' is disabled, it can't be used as a forward port target."),
(users['other'], "There is no branch 'foo', it can't be used as a forward port target."),
(users['other'], "Forward-porting to 'c'."),
seen(env, pr, users),
}
@ -201,14 +202,13 @@ def test_default_disabled(env, config, make_repo, users):
assert pr2.comments == [
seen(env, pr2, users),
(users['user'], """\
Ping @%s, @%s
This PR targets b and is the last of the forward-port chain.
@%(user)s @%(reviewer)s this PR targets b and is the last of the forward-port chain.
To merge the full chain, say
> @%s r+
> @%(user)s r+
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
""" % (users['user'], users['reviewer'], users['user'])),
""" % users)
]
def test_limit_after_merge(env, config, make_repo, users):
@ -247,7 +247,7 @@ def test_limit_after_merge(env, config, make_repo, users):
(users['reviewer'], "hansen r+"),
seen(env, pr1, users),
(users['reviewer'], bot_name + ' up to b'),
(bot_name, "Sorry, forward-port limit can only be set before the PR is merged."),
(bot_name, "@%s forward-port limit can only be set before the PR is merged." % users['reviewer']),
]
assert pr2.comments == [
seen(env, pr2, users),
@ -257,9 +257,11 @@ This PR targets b and is part of the forward-port chain. Further PRs will be cre
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
"""),
(users['reviewer'], bot_name + ' up to b'),
(bot_name, "Sorry, forward-port limit can only be set on an origin PR"
" (%s here) before it's merged and forward-ported." % p1.display_name
),
(bot_name, "@%s forward-port limit can only be set on an origin PR"
" (%s here) before it's merged and forward-ported." % (
users['reviewer'],
p1.display_name,
)),
]
# update pr2 to detach it from pr1
@ -279,10 +281,13 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
env.run_crons()
assert pr2.comments[4:] == [
(bot_name, "This PR was modified / updated and has become a normal PR. "
"It should be merged the normal way (via @hansen)"),
(bot_name, "@%s @%s this PR was modified / updated and has become a normal PR. "
"It should be merged the normal way (via @%s)" % (
users['user'], users['reviewer'],
p2.repository.project_id.github_prefix
)),
(users['reviewer'], bot_name + ' up to b'),
(bot_name, "Sorry, forward-port limit can only be set on an origin PR "
(bot_name, f"@{users['reviewer']} forward-port limit can only be set on an origin PR "
f"({p1.display_name} here) before it's merged and forward-ported."
),
]

28
forwardport/tests/test_simple.py
View File

@ -125,8 +125,11 @@ def test_straightforward_flow(env, config, make_repo, users):
assert pr.comments == [
(users['reviewer'], 'hansen r+ rebase-ff'),
seen(env, pr, users),
(users['user'], 'Merge method set to rebase and fast-forward'),
(users['user'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + ', '.join((pr1 | pr2).mapped('display_name'))),
(users['user'], 'Merge method set to rebase and fast-forward.'),
(users['user'], '@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n%s' % (
users['other'], users['reviewer'],
'\n- '.join((pr1 | pr2).mapped('display_name'))
)),
]
assert pr0_ == pr0
@ -148,8 +151,7 @@ def test_straightforward_flow(env, config, make_repo, users):
assert pr2_remote.comments == [
seen(env, pr2_remote, users),
(users['user'], """\
Ping @%s, @%s
This PR targets c and is the last of the forward-port chain containing:
@%s @%s this PR targets c and is the last of the forward-port chain containing:
* %s
To merge the full chain, say
@ -314,11 +316,18 @@ def test_empty(env, config, make_repo, users):
env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK})
env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK})
awaiting = (
users['other'],
'@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n%s' % (
users['user'], users['reviewer'],
fail_id.display_name
)
)
assert pr1.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, pr1, users),
(users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name),
(users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name),
awaiting,
awaiting,
], "each cron run should trigger a new message on the ancestor"
# check that this stops if we close the PR
with prod:
@ -327,8 +336,8 @@ def test_empty(env, config, make_repo, users):
assert pr1.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, pr1, users),
(users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name),
(users['other'], 'This pull request has forward-port PRs awaiting action (not merged or closed): ' + fail_id.display_name),
awaiting,
awaiting,
]
def test_partially_empty(env, config, make_repo):
@ -562,8 +571,7 @@ def test_delegate_fw(env, config, make_repo, users):
assert pr2.comments == [
seen(env, pr2, users),
(users['user'], '''Ping @{self_reviewer}, @{reviewer}
This PR targets c and is the last of the forward-port chain.
(users['user'], '''@{self_reviewer} @{reviewer} this PR targets c and is the last of the forward-port chain.
To merge the full chain, say
> @{user} r+

20
forwardport/tests/test_updates.py
View File

@ -44,7 +44,7 @@ This PR targets b and is part of the forward-port chain. Further PRs will be cre
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
''')
ci_warning = (users['user'], 'Ping @%(user)s, @%(reviewer)s\n\nci/runbot failed on this forward-port PR' % users)
ci_warning = (users['user'], '@%(user)s @%(reviewer)s ci/runbot failed on this forward-port PR' % users)
# oh no CI of the first FP PR failed!
# simulate status being sent multiple times (e.g. on multiple repos) with
@ -103,7 +103,11 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
assert pr1_remote.comments == [
seen(env, pr1_remote, users),
fp_intermediate, ci_warning, ci_warning,
(users['user'], "This PR was modified / updated and has become a normal PR. It should be merged the normal way (via @%s)" % pr1_id.repository.project_id.github_prefix),
(users['user'], "@%s @%s this PR was modified / updated and has become a normal PR. "
"It should be merged the normal way (via @%s)" % (
users['user'], users['reviewer'],
pr1_id.repository.project_id.github_prefix
)),
], "users should be warned that the PR has become non-FP"
# NOTE: should the followup PR wait for pr1 CI or not?
assert pr2_id.head != pr2_head
@ -210,9 +214,13 @@ def test_update_merged(env, make_repo, config, users):
More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port
'''),
(users['reviewer'], 'hansen r+'),
(users['user'], """Ancestor PR %s has been updated but this PR is merged and can't be updated to match.
(users['user'], """@%s @%s ancestor PR %s has been updated but this PR is merged and can't be updated to match.
You may want or need to manually update any followup PR.""" % pr1_id.display_name)
You may want or need to manually update any followup PR.""" % (
users['user'],
users['reviewer'],
pr1_id.display_name,
))
]
def test_duplicate_fw(env, make_repo, setreviewers, config, users):
@ -377,7 +385,7 @@ conflict!
# 2. "forward port chain" bit
# 3. updated / modified & got detached
assert pr2.comments[3:] == [
(users['user'], f"WARNING: the latest change ({pr2_id.head}) triggered "
(users['user'], f"@{users['user']} WARNING: the latest change ({pr2_id.head}) triggered "
f"a conflict when updating the next forward-port "
f"({pr3_id.display_name}), and has been ignored.\n\n"
f"You will need to update this pull request "
@ -389,7 +397,7 @@ conflict!
# 2. forward-port chain thing
assert repo.get_pr(pr3_id.number).comments[2:] == [
(users['user'], re_matches(f'''\
WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \
@{users['user']} WARNING: the update of {pr2_id.display_name} to {pr2_id.head} has caused a \
conflict in this pull request, data may have been lost.
stdout:

20
forwardport/tests/test_weird.py
View File

@ -399,18 +399,20 @@ class TestNotAllBranches:
assert pr_a.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, pr_a, users),
(users['user'], "This pull request can not be forward ported: next "
"branch is 'b' but linked pull request %s#%d has a"
" next branch 'c'." % (b.name, pr_b.number)
)
(users['user'], "@%s @%s this pull request can not be forward ported:"
" next branch is 'b' but linked pull request %s "
"has a next branch 'c'." % (
users['user'], users['reviewer'], pr_b_id.display_name,
)),
]
assert pr_b.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, pr_b, users),
(users['user'], "This pull request can not be forward ported: next "
"branch is 'c' but linked pull request %s#%d has a"
" next branch 'b'." % (a.name, pr_a.number)
)
(users['user'], "@%s @%s this pull request can not be forward ported:"
" next branch is 'c' but linked pull request %s "
"has a next branch 'b'." % (
users['user'], users['reviewer'], pr_a_id.display_name,
)),
]
def test_new_intermediate_branch(env, config, make_repo):
@ -755,7 +757,7 @@ def test_approve_draft(env, config, make_repo, users):
assert pr.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, pr, users),
(users['user'], f"I'm sorry, @{users['reviewer']}. Draft PRs can not be approved."),
(users['user'], f"I'm sorry, @{users['reviewer']}: draft PRs can not be approved."),
]
with prod:

3
runbot_merge/changelog/2022-06/pinging.md Normal file
View File

@ -0,0 +1,3 @@
IMP: review pinging (`@`-notification) of users by the mergebot and forwardbot
The bots should more consistently ping users when they need some sort of action to proceed.

2
runbot_merge/controllers/__init__.py
View File

@ -218,7 +218,7 @@ def handle_pr(env, event):
if pr_obj.state == 'merged':
feedback(
close=True,
message="@%s ya silly goose you can't reopen a PR that's been merged PR." % event['sender']['login']
message="@%s ya silly goose you can't reopen a merged PR." % event['sender']['login']
)
if pr_obj.state == 'closed':

97
runbot_merge/models/pull_requests.py
View File

@ -3,6 +3,7 @@
import ast
import base64
import collections
import contextlib
import datetime
import io
import itertools
@ -116,7 +117,7 @@ All substitutions are tentatively applied sequentially to the input.
feedback({
'repository': self.id,
'pull_request': number,
'message': "I'm sorry. Branch `{}` is not within my remit.".format(pr['base']['ref']),
'message': "Branch `{}` is not within my remit, imma just ignore it.".format(pr['base']['ref']),
})
return
@ -142,8 +143,9 @@ All substitutions are tentatively applied sequentially to the input.
feedback({
'repository': self.id,
'pull_request': number,
'message': "Sorry, I didn't know about this PR and had to retrieve "
"its information, you may have to re-approve it."
'message': "%sI didn't know about this PR and had to retrieve "
"its information, you may have to re-approve it as "
"I didn't see previous commands." % pr_id.ping()
})
# init the PR to the null commit so we can later synchronise it back
# back to the "proper" head while resetting reviews
@ -338,14 +340,20 @@ class Branch(models.Model):
_logger.exception("Failed to merge %s into staging branch", pr.display_name)
if first or isinstance(e, exceptions.Unmergeable):
if len(e.args) > 1 and e.args[1]:
message = e.args[1]
reason = e.args[1]
else:
message = "Unable to stage PR (%s)" % e.__context__
reason = e.__context__
# if the reason is a json document, assume it's a github
# error and try to extract the error message to give it to
# the user
with contextlib.suppress(Exception):
reason = json.loads(str(reason))['message'].lower()
pr.state = 'error'
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': pr.repository.id,
'pull_request': pr.number,
'message': message,
'message': f'{pr.ping()}unable to stage: {reason}',
})
else:
first = False
@ -534,6 +542,17 @@ class PullRequests(models.Model):
repo_name = fields.Char(related='repository.name')
message_title = fields.Char(compute='_compute_message_title')
def ping(self, author=True, reviewer=True):
P = self.env['res.partner']
s = ' '.join(
f'@{p.github_login}'
for p in (self.author if author else P) | (self.reviewed_by if reviewer else P)
if p
)
if s:
s += ' '
return s
@api.depends('repository.name', 'number')
def _compute_url(self):
base = werkzeug.urls.url_parse(self.env['ir.config_parameter'].sudo().get_param('web.base.url', 'http://localhost:8069'))
@ -791,14 +810,14 @@ class PullRequests(models.Model):
msgs = []
for command, param in commands:
ok = False
msg = []
msg = None
if command == 'retry':
if is_author:
if self.state == 'error':
ok = True
self.state = 'ready'
else:
msg = "Retry makes no sense when the PR is not in error."
msg = "retry makes no sense when the PR is not in error."
elif command == 'check':
if is_author:
self.env['runbot_merge.fetch_job'].create({
@ -808,14 +827,14 @@ class PullRequests(models.Model):
ok = True
elif command == 'review':
if self.draft:
msg = "Draft PRs can not be approved."
msg = "draft PRs can not be approved."
elif param and is_reviewer:
oldstate = self.state
newstate = RPLUS.get(self.state)
if not author.email:
msg = "I must know your email before you can review PRs. Please contact an administrator."
elif not newstate:
msg = "This PR is already reviewed, reviewing it again is useless."
msg = "this PR is already reviewed, reviewing it again is useless."
else:
self.state = newstate
self.reviewed_by = author
@ -832,7 +851,7 @@ class PullRequests(models.Model):
Feedback.create({
'repository': self.repository.id,
'pull_request': self.number,
'message': "@{}, you may want to rebuild or fix this PR as it has failed CI.".format(author.github_login),
'message': "@{} you may want to rebuild or fix this PR as it has failed CI.".format(login),
})
elif not param and is_author:
newstate = RMINUS.get(self.state)
@ -846,7 +865,7 @@ class PullRequests(models.Model):
'pull_request': self.number,
'message': "PR priority reset to 1, as pull requests with priority 0 ignore review state.",
})
self.unstage("unreviewed (r-) by %s", author.github_login)
self.unstage("unreviewed (r-) by %s", login)
ok = True
else:
msg = "r- makes no sense in the current PR state."
@ -875,7 +894,7 @@ class PullRequests(models.Model):
elif command == 'method':
if is_reviewer:
if param == 'squash' and not self.squash:
msg = "Squash can only be used with a single commit at this time."
msg = "squash can only be used with a single commit at this time."
else:
self.merge_method = param
ok = True
@ -883,7 +902,7 @@ class PullRequests(models.Model):
Feedback.create({
'repository': self.repository.id,
'pull_request': self.number,
'message':"Merge method set to %s" % explanation
'message':"Merge method set to %s." % explanation
})
elif command == 'override':
overridable = author.override_rights\
@ -905,7 +924,7 @@ class PullRequests(models.Model):
c.create({'sha': self.head, 'statuses': '{}'})
ok = True
else:
msg = f"You are not allowed to override this status."
msg = "you are not allowed to override this status."
else:
# ignore unknown commands
continue
@ -920,21 +939,23 @@ class PullRequests(models.Model):
applied.append(reformat(command, param))
else:
ignored.append(reformat(command, param))
msgs.append(msg or "You can't {}.".format(reformat(command, param)))
msgs.append(msg or "you can't {}.".format(reformat(command, param)))
if msgs:
joiner = ' ' if len(msgs) == 1 else '\n- '
msgs.insert(0, "I'm sorry, @{}:".format(login))
Feedback.create({
'repository': self.repository.id,
'pull_request': self.number,
'message': joiner.join(msgs),
})
msg = []
if applied:
msg.append('applied ' + ' '.join(applied))
if ignored:
ignoredstr = ' '.join(ignored)
msg.append('ignored ' + ignoredstr)
if msgs:
msgs.insert(0, "I'm sorry, @{}.".format(login))
Feedback.create({
'repository': self.repository.id,
'pull_request': self.number,
'message': ' '.join(msgs),
})
return '\n'.join(msg)
def _pr_acl(self, user):
@ -1021,7 +1042,7 @@ class PullRequests(models.Model):
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': self.repository.id,
'pull_request': self.number,
'message': "%r failed on this reviewed PR." % ci,
'message': "%s%r failed on this reviewed PR." % (self.ping(), ci),
})
def _auto_init(self):
@ -1158,11 +1179,9 @@ class PullRequests(models.Model):
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': r.repository.id,
'pull_request': r.number,
'message': "Linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format(
', '.join(map(
'{0.display_name}'.format,
unready
))
'message': "{}linked pull request(s) {} not ready. Linked PRs are not staged until all of them are ready.".format(
r.ping(),
', '.join(map('{0.display_name}'.format, unready))
)
})
r.link_warned = True
@ -1171,6 +1190,11 @@ class PullRequests(models.Model):
# send feedback for multi-commit PRs without a merge_method (which
# we've not warned yet)
methods = ''.join(
'* `%s` to %s\n' % pair
for pair in type(self).merge_method.selection
if pair[0] != 'squash'
)
for r in self.search([
('state', '=', 'ready'),
('squash', '=', False),
@ -1180,10 +1204,9 @@ class PullRequests(models.Model):
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': r.repository.id,
'pull_request': r.number,
'message': "Because this PR has multiple commits, I need to know how to merge it:\n\n" + ''.join(
'* `%s` to %s\n' % pair
for pair in type(self).merge_method.selection
if pair[0] != 'squash'
'message': "%sbecause this PR has multiple commits, I need to know how to merge it:\n\n%s" % (
r.ping(),
methods,
)
})
r.method_warned = True
@ -1740,7 +1763,7 @@ class Stagings(models.Model):
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': pr.repository.id,
'pull_request': pr.number,
'message':"Staging failed: %s" % message
'message': "%sstaging failed: %s" % (pr.ping(), message),
})
self.batch_ids.write({'active': False})
@ -2042,11 +2065,11 @@ class Batch(models.Model):
self.env['runbot_merge.pull_requests.feedback'].create({
'repository': pr.repository.id,
'pull_request': pr.number,
'message': "We apparently missed an update to this PR "
'message': "%swe apparently missed an update to this PR "
"and tried to stage it in a state which "
"might not have been approved. PR has been "
"updated to %s, please check and approve or "
"re-approve." % new_head
"re-approve." % (pr.ping(), new_head)
})
return self.env['runbot_merge.batch']

61
runbot_merge/tests/test_basic.py
View File

@ -63,7 +63,6 @@ def test_trivial_flow(env, repo, page, users, config):
]
assert statuses == [('legal/cla', 'ok'), ('ci/runbot', 'ok')]
with repo:
pr.post_comment('hansen r+ rebase-merge', config['role_reviewer']['token'])
assert pr_id.state == 'ready'
@ -443,8 +442,8 @@ def test_staging_conflict_first(env, repo, users, config, page):
assert pr.comments == [
(users['reviewer'], 'hansen r+ rebase-merge'),
seen(env, pr, users),
(users['user'], 'Merge method set to rebase and merge, using the PR as merge commit message'),
(users['user'], re_matches('^Unable to stage PR')),
(users['user'], 'Merge method set to rebase and merge, using the PR as merge commit message.'),
(users['user'], '@%(user)s @%(reviewer)s unable to stage: merge conflict' % users),
]
dangerbox = pr_page(page, pr).cssselect('.alert-danger span')
@ -566,8 +565,8 @@ def test_staging_ci_failure_single(env, repo, users, config, page):
assert pr.comments == [
(users['reviewer'], 'hansen r+ rebase-merge'),
seen(env, pr, users),
(users['user'], "Merge method set to rebase and merge, using the PR as merge commit message"),
(users['user'], 'Staging failed: ci/runbot')
(users['user'], "Merge method set to rebase and merge, using the PR as merge commit message."),
(users['user'], '@%(user)s @%(reviewer)s staging failed: ci/runbot' % users)
]
dangerbox = pr_page(page, pr).cssselect('.alert-danger span')
@ -970,9 +969,9 @@ def test_ci_failure_after_review(env, repo, users, config):
assert prx.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, prx, users),
(users['user'], "'ci/runbot' failed on this reviewed PR.".format_map(users)),
(users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)),
(users['user'], "'legal/cla' failed on this reviewed PR.".format_map(users)),
(users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)),
(users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)),
(users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)),
]
def test_reopen_merged_pr(env, repo, config, users):
@ -1016,7 +1015,7 @@ def test_reopen_merged_pr(env, repo, config, users):
assert prx.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, prx, users),
(users['user'], "@%s ya silly goose you can't reopen a PR that's been merged PR." % users['other'])
(users['user'], "@%s ya silly goose you can't reopen a merged PR." % users['other'])
]
class TestNoRequiredStatus:
@ -1212,7 +1211,7 @@ class TestRetry:
(users['reviewer'], 'hansen r+'),
(users['reviewer'], 'hansen retry'),
seen(env, prx, users),
(users['user'], "I'm sorry, @{}. Retry makes no sense when the PR is not in error.".format(users['reviewer'])),
(users['user'], "I'm sorry, @{reviewer}: retry makes no sense when the PR is not in error.".format_map(users)),
]
@pytest.mark.parametrize('disabler', ['user', 'other', 'reviewer'])
@ -1408,12 +1407,13 @@ class TestMergeMethod:
assert prx.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, prx, users),
(users['user'], """Because this PR has multiple commits, I need to know how to merge it:
(users['user'], """@{user} @{reviewer} because this PR has multiple \
commits, I need to know how to merge it:
* `merge` to merge directly, using the PR as merge commit message
* `rebase-merge` to rebase and merge, using the PR as merge commit message
* `rebase-ff` to rebase and fast-forward
"""),
""".format_map(users)),
]
def test_pr_method_no_review(self, repo, env, users, config):
@ -1453,11 +1453,11 @@ class TestMergeMethod:
assert prx.comments == [
(users['reviewer'], 'hansen rebase-merge'),
seen(env, prx, users),
(users['user'], "Merge method set to rebase and merge, using the PR as merge commit message"),
(users['user'], "Merge method set to rebase and merge, using the PR as merge commit message."),
(users['reviewer'], 'hansen merge'),
(users['user'], "Merge method set to merge directly, using the PR as merge commit message"),
(users['user'], "Merge method set to merge directly, using the PR as merge commit message."),
(users['reviewer'], 'hansen rebase-ff'),
(users['user'], "Merge method set to rebase and fast-forward"),
(users['user'], "Merge method set to rebase and fast-forward."),
]
def test_pr_rebase_merge(self, repo, env, users, config):
@ -2025,7 +2025,7 @@ Part-of: {pr_id.display_name}"""
assert pr1.comments == [
seen(env, pr1, users),
(users['reviewer'], 'hansen r+ squash'),
(users['user'], 'Merge method set to squash')
(users['user'], 'Merge method set to squash.')
]
merged_head = repo.commit('master')
assert merged_head.message == f"""first pr
@ -2049,13 +2049,13 @@ Signed-off-by: {get_partner(env, users["reviewer"]).formatted_email}\
assert pr2.comments == [
seen(env, pr2, users),
(users['reviewer'], 'hansen r+ squash'),
(users['user'], f"I'm sorry, @{users['reviewer']}. Squash can only be used with a single commit at this time."),
(users['user'], """Because this PR has multiple commits, I need to know how to merge it:
(users['user'], f"I'm sorry, @{users['reviewer']}: squash can only be used with a single commit at this time."),
(users['user'], """@{user} @{reviewer} because this PR has multiple commits, I need to know how to merge it:
* `merge` to merge directly, using the PR as merge commit message
* `rebase-merge` to rebase and merge, using the PR as merge commit message
* `rebase-ff` to rebase and fast-forward
""")
""".format_map(users))
]
@pytest.mark.xfail(reason="removed support for squash- command")
@ -2859,7 +2859,7 @@ class TestReviewing(object):
(users['user'], "I'm sorry, @{}. I'm afraid I can't do that.".format(users['other'])),
(users['reviewer'], 'hansen r+'),
(users['reviewer'], 'hansen r+'),
(users['user'], "I'm sorry, @{}. This PR is already reviewed, reviewing it again is useless.".format(
(users['user'], "I'm sorry, @{}: this PR is already reviewed, reviewing it again is useless.".format(
users['reviewer'])),
]
@ -2888,7 +2888,7 @@ class TestReviewing(object):
assert prx.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, prx, users),
(users['user'], "I'm sorry, @{}. You can't review+.".format(users['reviewer'])),
(users['user'], "I'm sorry, @{}: you can't review+.".format(users['reviewer'])),
]
def test_self_review_success(self, env, repo, users, config):
@ -3044,7 +3044,7 @@ class TestReviewing(object):
seen(env, pr, users),
(users['reviewer'], 'hansen delegate+'),
(users['user'], 'hansen r+'),
(users['user'], f"I'm sorry, @{users['user']}. I must know your email before you can review PRs. Please contact an administrator."),
(users['user'], f"I'm sorry, @{users['user']}: I must know your email before you can review PRs. Please contact an administrator."),
]
user_partner.fetch_github_email()
assert user_partner.email
@ -3108,9 +3108,9 @@ class TestUnknownPR:
seen(env, prx, users),
(users['reviewer'], 'hansen r+'),
(users['reviewer'], 'hansen r+'),
(users['user'], "Sorry, I didn't know about this PR and had to "
(users['user'], "I didn't know about this PR and had to "
"retrieve its information, you may have to "
"re-approve it."),
"re-approve it as I didn't see previous commands."),
seen(env, prx, users),
]
@ -3161,9 +3161,9 @@ class TestUnknownPR:
assert pr.comments == [
seen(env, pr, users),
(users['reviewer'], 'hansen r+'),
(users['user'], "Sorry, I didn't know about this PR and had to "
"retrieve its information, you may have to "
"re-approve it."),
(users['user'], "I didn't know about this PR and had to retrieve "
"its information, you may have to re-approve it "
"as I didn't see previous commands."),
seen(env, pr, users),
]
@ -3189,7 +3189,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."),
(users['user'], "Branch `branch` is not within my remit, imma just ignore it."),
]
def test_rplus_review_unmanaged(self, env, repo, users, config):
@ -3586,7 +3586,7 @@ class TestFeedback:
assert prx.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, prx, users),
(users['user'], "'ci/runbot' failed on this reviewed PR.")
(users['user'], "@%(user)s @%(reviewer)s 'ci/runbot' failed on this reviewed PR." % users)
]
def test_review_failed(self, repo, env, users, config):
@ -3616,8 +3616,9 @@ class TestFeedback:
assert prx.comments == [
seen(env, prx, users),
(users['reviewer'], 'hansen r+'),
(users['user'], "@%s, you may want to rebuild or fix this PR as it has failed CI." % users['reviewer'])
(users['user'], "@%s you may want to rebuild or fix this PR as it has failed CI." % users['reviewer'])
]
class TestInfrastructure:
def test_protection(self, repo):
""" force-pushing on a protected ref should fail

28
runbot_merge/tests/test_multirepo.py
View File

@ -429,7 +429,7 @@ def test_merge_fail(env, project, repo_a, repo_b, users, config):
assert pr1b.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, pr1b, users),
(users['user'], re_matches('^Unable to stage PR')),
(users['user'], '@%(user)s @%(reviewer)s unable to stage: merge conflict' % users),
]
other = to_pr(env, pr1a)
reviewer = get_partner(env, users["reviewer"]).formatted_email
@ -527,14 +527,22 @@ class TestCompanionsNotReady:
assert p_a.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, p_a, users),
(users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)),
(users['user'], "@%s @%s linked pull request(s) %s not ready. Linked PRs are not staged until all of them are ready." % (
users['user'],
users['reviewer'],
pr_b.display_name,
)),
]
# ensure the message is only sent once per PR
env.run_crons('runbot_merge.check_linked_prs_status')
assert p_a.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, p_a, users),
(users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (repo_b.name, p_b.number)),
(users['user'], "@%s @%s linked pull request(s) %s not ready. Linked PRs are not staged until all of them are ready." % (
users['user'],
users['reviewer'],
pr_b.display_name,
)),
]
assert p_b.comments == [seen(env, p_b, users)]
@ -570,7 +578,8 @@ class TestCompanionsNotReady:
assert pr_b.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, pr_b, users),
(users['user'], "Linked pull request(s) %s#%d, %s#%d not ready. Linked PRs are not staged until all of them are ready." % (
(users['user'], "@%s @%s linked pull request(s) %s#%d, %s#%d not ready. Linked PRs are not staged until all of them are ready." % (
users['user'], users['reviewer'],
repo_a.name, pr_a.number,
repo_c.name, pr_c.number
))
@ -609,7 +618,8 @@ class TestCompanionsNotReady:
assert pr_b.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, pr_b, users),
(users['user'], "Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (
(users['user'], "@%s @%s linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (
users['user'], users['reviewer'],
repo_a.name, pr_a.number
))
]
@ -617,7 +627,8 @@ class TestCompanionsNotReady:
(users['reviewer'], 'hansen r+'),
seen(env, pr_c, users),
(users['user'],
"Linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (
"@%s @%s linked pull request(s) %s#%d not ready. Linked PRs are not staged until all of them are ready." % (
users['user'], users['reviewer'],
repo_a.name, pr_a.number
))
]
@ -655,7 +666,10 @@ def test_other_failed(env, project, repo_a, repo_b, users, config):
assert pr_a.comments == [
(users['reviewer'], 'hansen r+'),
seen(env, pr_a, users),
(users['user'], 'Staging failed: ci/runbot on %s (view more at http://example.org/b)' % sth)
(users['user'], '@%s @%s staging failed: ci/runbot on %s (view more at http://example.org/b)' % (
users['user'], users['reviewer'],
sth
))
]
class TestMultiBatches:

2
runbot_merge/tests/test_status_overrides.py
View File

@ -89,7 +89,7 @@ def test_basic(env, project, make_repo, users, setreviewers, config):
(users['reviewer'], 'hansen r+'),
seen(env, pr, users),
(users['reviewer'], 'hansen override=l/int'),
(users['user'], "I'm sorry, @{}. You are not allowed to override this status.".format(users['reviewer'])),
(users['user'], "I'm sorry, @{}: you are not allowed to override this status.".format(users['reviewer'])),
(users['other'], "hansen override=l/int"),
]
assert pr_id.statuses == '{}'