diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 5aaf3a20..eb2b36b0 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -55,55 +55,6 @@ class Project(models.Model): id: int github_prefix: str - fp_github_token = fields.Char() - fp_github_name = fields.Char(store=True, compute="_compute_git_identity") - fp_github_email = fields.Char(store=True, compute="_compute_git_identity") - - def _find_commands(self, comment): - if self.env.context.get('without_forward_port'): - return super()._find_commands(comment) - - return re.findall( - '^\s*[@|#]?{}:? (.*)$'.format(self.fp_github_name), - comment, re.MULTILINE | re.IGNORECASE - ) + super()._find_commands(comment) - - # technically the email could change at any moment... - @api.depends('fp_github_token') - def _compute_git_identity(self): - s = requests.Session() - for project in self: - if not project.fp_github_token or (project.fp_github_name and project.fp_github_email): - continue - - r0 = s.get('https://api.github.com/user', headers={ - 'Authorization': 'token %s' % project.fp_github_token - }) - if not r0.ok: - _logger.error("Failed to fetch forward bot information for project %s: %s", project.name, r0.text or r0.content) - continue - - user = r0.json() - project.fp_github_name = user['name'] or user['login'] - if email := user['email']: - project.fp_github_email = email - continue - - if 'user:email' not in set(re.split(r',\s*', r0.headers['x-oauth-scopes'])): - raise UserError("The forward-port github token needs the user:email scope to fetch the bot's identity.") - r1 = s.get('https://api.github.com/user/emails', headers={ - 'Authorization': 'token %s' % project.fp_github_token - }) - if not r1.ok: - _logger.error("Failed to fetch forward bot emails for project %s: %s", project.name, r1.text or r1.content) - continue - project.fp_github_email = next(( - entry['email'] - for entry in r1.json() - if entry['primary'] - ), None) - if not project.fp_github_email: - raise UserError("The forward-port bot needs a public or primary email set up.") def write(self, vals): # check on branches both active and inactive so disabling branches doesn't @@ -404,90 +355,6 @@ class PullRequests(models.Model): }) return r - def _parse_commands(self, author, comment, login): - super(PullRequests, self.with_context(without_forward_port=True))._parse_commands(author, comment, login) - - tokens = [ - token - for line in re.findall('^\s*[@|#]?{}:? (.*)$'.format(self.repository.project_id.fp_github_name), comment['body'] or '', re.MULTILINE | re.IGNORECASE) - for token in line.split() - ] - if not tokens: - _logger.info("found no commands in comment of %s (%s) (%s)", author.github_login, author.display_name, - utils.shorten(comment['body'] or '', 50) - ) - return - - # TODO: don't use a mutable tokens iterator - tokens = iter(tokens) - while True: - token = next(tokens, None) - if token is None: - break - - ping = False - close = False - msg = None - if token in ('ci', 'skipci'): - pr = (self.source_id or self) - if pr._pr_acl(author).is_reviewer: - 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: - ping = True - msg = "you can't configure ci." - - if token == 'ignore': # replace 'ignore' by 'up to ' - token = 'up' - tokens = itertools.chain(['to', self.target.name], tokens) - - if token in ('r+', 'review+'): - if not self.source_id: - 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 - ) - elif not self.parent_id: - ping = True - msg = "I can only do this on unmodified forward-port PRs, ask {}.".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': - if self.source_id._pr_acl(author).is_reviewer: - close = True - 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: - msg = "you can't set a forward-port limit." - elif not limit: - msg = "please provide a branch to forward-port to." - else: - ping, msg = self._maybe_update_limit(limit) - - if msg or close: - if msg: - _logger.info("%s [%s]: %s", self.display_name, login, msg) - else: - _logger.info("%s [%s]: closing", self.display_name, login) - self.env['runbot_merge.pull_requests.feedback'].create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'message': f'@{author.github_login} {msg}' if msg and ping else msg, - 'close': close, - 'token_field': 'fp_github_token', - }) - def _maybe_update_limit(self, limit: str) -> typing.Tuple[bool, str]: limit_id = self.env['runbot_merge.branch'].with_context(active_test=False).search([ ('project_id', '=', self.repository.project_id.id), diff --git a/forwardport/tests/test_limit.py b/forwardport/tests/test_limit.py index c96fa853..a2c174c5 100644 --- a/forwardport/tests/test_limit.py +++ b/forwardport/tests/test_limit.py @@ -101,22 +101,22 @@ def test_disable(env, config, make_repo, users): pr = prod.make_pr(target='a', head='branch2') prod.post_status(c, 'success', 'legal/cla') prod.post_status(c, 'success', 'ci/runbot') - pr.post_comment('hansen r+\n%s up to' % bot_name, config['role_reviewer']['token']) - pr.post_comment('%s up to b' % bot_name, config['role_reviewer']['token']) - pr.post_comment('%s up to foo' % bot_name, config['role_reviewer']['token']) - pr.post_comment('%s up to c' % bot_name, config['role_reviewer']['token']) + pr.post_comment(f'hansen r+\n{bot_name} up to', config['role_reviewer']['token']) + pr.post_comment(f'{bot_name} up to b', config['role_reviewer']['token']) + pr.post_comment(f'{bot_name} up to foo', config['role_reviewer']['token']) + pr.post_comment(f'{bot_name} up to c', config['role_reviewer']['token']) env.run_crons() # use a set because git webhooks delays might lead to mis-ordered # 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['reviewer'], f"hansen r+\n{bot_name} up to"), + (users['other'], "@{reviewer} please provide a branch to forward-port to.".format_map(users)), + (users['reviewer'], f"{bot_name} up to b"), + (users['other'], "@{reviewer} branch 'b' is disabled, it can't be used as a forward port target.".format_map(users)), + (users['reviewer'], f"{bot_name} up to foo"), + (users['other'], "@{reviewer} there is no branch 'foo', it can't be used as a forward port target.".format_map(users)), + (users['reviewer'], f"{bot_name} up to c"), (users['other'], "Forward-porting to 'c'."), seen(env, pr, users), } diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 32bcf0db..1c1fc4ab 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -809,7 +809,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"@{users['reviewer']} draft PRs can not be approved."), ] with prod: diff --git a/runbot_merge/models/commands.py b/runbot_merge/models/commands.py new file mode 100644 index 00000000..aa597bb8 --- /dev/null +++ b/runbot_merge/models/commands.py @@ -0,0 +1,231 @@ +import enum +from collections import abc +from dataclasses import dataclass, field +from typing import Iterator, List, Optional, Union + + +def tokenize(line: str) -> Iterator[str]: + cur = '' + for c in line: + if c == '-' and not cur: + yield '-' + elif c in ' \t+=,': + if cur: + yield cur + cur = '' + if not c.isspace(): + yield c + else: + cur += c + + if cur: + yield cur + +def normalize(it: Iterator[str]) -> Iterator[str]: + """Converts shorthand tokens to expanded version + """ + for t in it: + match t: + case 'r': + yield 'review' + case 'r-': + yield 'review' + yield '-' + case 'p': + yield 'priority' + case _: + yield t + + +@dataclass +class Peekable(abc.Iterator[str]): + it: Iterator[str] + memo: Optional[str] = None + + def __iter__(self) -> Iterator[str]: + return self + + def __next__(self) -> str: + if self.memo is not None: + v, self.memo = self.memo, None + return v + return next(self.it) + + def peek(self) -> Optional[str]: + if self.memo is None: + self.memo = next(self.it, None) + return self.memo + + +def assert_next(it: Iterator[str], val: str): + if (actual := next(it, None)) != val: + raise CommandError(f"expected {val!r}, got {actual!r}") + + +class CommandError(Exception): + pass + + +class Approve: + def __str__(self): + return 'review+' + + +class Reject: + def __str__(self): + return 'review-' + + +class MergeMethod(enum.Enum): + SQUASH = 'squash' + REBASE_FF = 'rebase-ff' + REBASE_MERGE = 'rebase-merge' + MERGE = 'merge' + + def __str__(self): + return self.value + + +class Retry: + def __str__(self): + return 'retry' + + +class Check: + def __str__(self): + return 'check' + + +@dataclass +class Override: + statuses: List[str] = field(default_factory=list) + + def __str__(self): + return f"override={','.join(self.statuses)}" + + +@dataclass +class Delegate: + users: List[str] = field(default_factory=list) + + def __str__(self): + if not self.users: + return 'delegate+' + return f"delegate={','.join(self.users)}" + + +class Priority(enum.IntEnum): + NUKE = 0 + PRIORITIZE = 1 + DEFAULT = 2 + + +Command = Union[Approve, Reject, MergeMethod, Retry, Check, Override, Delegate, Priority] + + +def parse_mergebot(line: str) -> Iterator[Command]: + it = Peekable(normalize(tokenize(line))) + for token in it: + match token: + case 'review': + match next(it): + case '+': + yield Approve() + case '-': + yield Reject() + case t: + raise CommandError(f"unknown review {t!r}") + case 'squash': + yield MergeMethod.SQUASH + case 'rebase-ff': + yield MergeMethod.REBASE_FF + case 'rebase-merge': + yield MergeMethod.REBASE_MERGE + case 'merge': + yield MergeMethod.MERGE + case 'retry': + yield Retry() + case 'check': + yield Check() + case 'override': + assert_next(it, '=') + ci = [next(it)] + while it.peek() == ',': + next(it) + ci.append(next(it)) + yield Override(ci) + case 'delegate': + match next(it): + case '+': + yield Delegate() + case '=': + delegates = [next(it).lstrip('#@')] + while it.peek() == ',': + next(it) + delegates.append(next(it).lstrip('#@')) + yield Delegate(delegates) + case d: + raise CommandError(f"unknown delegation {d!r}") + case 'priority': + assert_next(it, '=') + yield Priority(int(next(it))) + case c: + raise CommandError(f"unknown command {c!r}") + + +class FWApprove: + def __str__(self): + return 'review+' + + +@dataclass +class CI: + run: bool + def __str__(self): + return 'ci' if self.run else 'skipci' + + +@dataclass +class Limit: + branch: Optional[str] + + def __str__(self): + if self.branch is None: + return 'ignore' + return f'up to {self.branch}' + + +class Close: + def __str__(self): + return 'close' + + +FWCommand = Union[FWApprove, CI, Limit, Close] + + +def parse_forwardbot(line: str) -> Iterator[FWCommand]: + it = Peekable(normalize(tokenize(line))) + for token in it: + match token: + case 'review': + match next(it): + case '+': + yield FWApprove() + case t: + raise CommandError(f"unknown review {t!r}", True) + case 'ci': + yield CI(True) + case 'skipci': + yield CI(False) + case 'ignore': + yield Limit(None) + case 'up': + assert_next(it, 'to') + if limit := next(it, None): + yield Limit(limit) + else: + raise CommandError("please provide a branch to forward-port to.", True) + case 'close': + yield Close() + case c: + raise CommandError(f"unknown command {c!r}", True) diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index b686137c..9e335267 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -1,5 +1,6 @@ import logging import re +from typing import List, Tuple import requests import sentry_sdk @@ -38,6 +39,9 @@ class Project(models.Model): help="Prefix (~bot name) used when sending commands from PR " "comments e.g. [hanson retry] or [hanson r+ p=1]", ) + fp_github_token = fields.Char() + fp_github_name = fields.Char(store=True, compute="_compute_git_identity") + fp_github_email = fields.Char(store=True, compute="_compute_git_identity") batch_limit = fields.Integer( default=8, group_operator=None, help="Maximum number of PRs staged together") @@ -96,6 +100,43 @@ class Project(models.Model): if not project.github_email: raise UserError("The merge bot needs a public or accessible primary email set up.") + # technically the email could change at any moment... + @api.depends('fp_github_token') + def _compute_git_identity(self): + s = requests.Session() + for project in self: + if not project.fp_github_token or (project.fp_github_name and project.fp_github_email): + continue + + r0 = s.get('https://api.github.com/user', headers={ + 'Authorization': 'token %s' % project.fp_github_token + }) + if not r0.ok: + _logger.error("Failed to fetch forward bot information for project %s: %s", project.name, r0.text or r0.content) + continue + + user = r0.json() + project.fp_github_name = user['name'] or user['login'] + if email := user['email']: + project.fp_github_email = email + continue + + if 'user:email' not in set(re.split(r',\s*', r0.headers['x-oauth-scopes'])): + raise UserError("The forward-port github token needs the user:email scope to fetch the bot's identity.") + r1 = s.get('https://api.github.com/user/emails', headers={ + 'Authorization': 'token %s' % project.fp_github_token + }) + if not r1.ok: + _logger.error("Failed to fetch forward bot emails for project %s: %s", project.name, r1.text or r1.content) + continue + project.fp_github_email = next(( + entry['email'] + for entry in r1.json() + if entry['primary'] + ), None) + if not project.fp_github_email: + raise UserError("The forward-port bot needs a public or primary email set up.") + def _check_stagings(self, commit=False): # check branches with an active staging for branch in self.env['runbot_merge.branch']\ @@ -132,9 +173,18 @@ class Project(models.Model): if commit: self.env.cr.commit() - def _find_commands(self, comment): + def _find_commands(self, comment: str) -> List[Tuple[str, str]]: + """Tries to find all the lines starting (ignoring leading whitespace) + with either the merge or the forward port bot identifiers. + + For convenience, the identifier *can* be prefixed with an ``@`` or + ``#``, and suffixed with a ``:``. + """ + logins = '|'.join(map(re.escape, filter(None, [self.github_prefix, self.fp_github_name]))) + # horizontal whitespace (\s - {\n, \r}), but Python doesn't have \h or \p{Blank} + h = r'[^\S\r\n]' return re.findall( - '^\s*[@|#]?{}:? (.*)$'.format(self.github_prefix), + fr'^{h}*[@|#]?({logins})(?:{h}+|:{h}*)(.*)$', comment, re.MULTILINE | re.IGNORECASE) def _has_branch(self, name): diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 1b7a120b..6e4d63db 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -8,7 +8,7 @@ import logging import pprint import re import time -from typing import Optional, Union +from typing import Optional, Union, List, Literal import sentry_sdk import werkzeug @@ -16,6 +16,7 @@ import werkzeug from odoo import api, fields, models, tools from odoo.exceptions import ValidationError from odoo.osv import expression +from . import commands from .. import github, exceptions, controllers, utils @@ -561,71 +562,46 @@ class PullRequests(models.Model): 'closing': closing, }) - def _parse_command(self, commandstring): - for m in re.finditer( - r'(\S+?)(?:([+-])|=(\S*))?(?=\s|$)', - commandstring, - ): - name, flag, param = m.groups() - if name == 'r': - name = 'review' - if flag in ('+', '-'): - yield name, flag == '+' - elif name == 'delegate': - if param: - for p in param.split(','): - yield 'delegate', p.lstrip('#@') - elif name == 'override': - if param: - for p in param.split(','): - yield 'override', p - elif name in ('p', 'priority'): - if param in ('0', '1', '2'): - yield ('priority', int(param)) - elif any(name == k for k, _ in type(self).merge_method.selection): - yield ('method', name) - else: - yield name, param - def _parse_commands(self, author, comment, login): - """Parses a command string prefixed by Project::github_prefix. - - A command string can contain any number of space-separated commands: - - retry - resets a PR in error mode to ready for staging - r(eview)+/- - approves or disapproves a PR (disapproving just cancels an approval) - delegate+/delegate= - adds either PR author or the specified (github) users as - authorised reviewers for this PR. ```` is a - comma-separated list of github usernames (no @) - p(riority)=2|1|0 - sets the priority to normal (2), pressing (1) or urgent (0). - Lower-priority PRs are selected first and batched together. - rebase+/- - Whether the PR should be rebased-and-merged (the default) or just - merged normally. - """ assert self, "parsing commands must be executed in an actual PR" (login, name) = (author.github_login, author.display_name) if author else (login, 'not in system') - is_admin, is_reviewer, is_author = self._pr_acl(author) - - commands = [ - ps - for m in self.repository.project_id._find_commands(comment['body'] or '') - for ps in self._parse_command(m) - ] - - if not commands: - _logger.info("found no commands in comment of %s (%s) (%s)", author.github_login, author.display_name, + commandlines = self.repository.project_id._find_commands(comment['body'] or '') + if not commandlines: + _logger.info("found no commands in comment of %s (%s) (%s)", login, name, utils.shorten(comment['body'] or '', 50) ) return 'ok' - if not (is_author or any(cmd == 'override' for cmd, _ in commands)): + def feedback(message: Optional[str] = None, close: bool = False, token: Literal['github_token', 'fp_github_token'] = 'github_token'): + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': self.repository.id, + 'pull_request': self.number, + 'message': message, + 'close': close, + 'token_field': token, + }) + try: + cmds: List[Union[commands.Command, commands.FWCommand]] = [ + ps + for bot, line in commandlines + for ps in (commands.parse_mergebot(line) if bot.casefold() == self.repository.project_id.github_prefix.casefold() else commands.parse_forwardbot(line)) + ] + except Exception as e: + _logger.info( + "error %s while parsing comment of %s (%s): %s", + e, + author.github_login, author.display_name, + utils.shorten(comment['body'] or '', 50), + exc_info=True + ) + feedback(message=f"@{login} {e.args[0]}", token='fp_github_token' if len(e.args) >= 2 and e.args[1] else 'github_token') + return 'error' + + is_admin, is_reviewer, is_author = self._pr_acl(author) + + if not (is_author or any(isinstance(cmd, commands.Override) for cmd in cmds)): # no point even parsing commands _logger.info("ignoring comment of %s (%s): no ACL to %s", login, name, self.display_name) @@ -636,40 +612,13 @@ class PullRequests(models.Model): ) return 'ignored' - applied, ignored = [], [] - def reformat(command, param): - if param is None: - pstr = '' - elif isinstance(param, bool): - pstr = '+' if param else '-' - elif isinstance(param, list): - pstr = '=' + ','.join(param) - else: - pstr = '={}'.format(param) - - return '%s%s' % (command, pstr) - msgs = [] - for command, param in commands: - ok = False - 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." - elif command == 'check': - if is_author: - self.env['runbot_merge.fetch_job'].create({ - 'repository': self.repository.id, - 'number': self.number, - }) - ok = True - elif command == 'review': - if self.draft: + rejections = [] + for command in cmds: + fwbot, msg = False, None + match command: + case commands.Approve() if self.draft: msg = "draft PRs can not be approved." - elif param and is_reviewer: + case commands.Approve() if is_reviewer: oldstate = self.state newstate = RPLUS.get(self.state) if not author.email: @@ -679,7 +628,6 @@ class PullRequests(models.Model): else: self.state = newstate self.reviewed_by = author - ok = True _logger.debug( "r+ on %s by %s (%s->%s) status=%s message? %s", self.display_name, author.github_login, @@ -694,7 +642,7 @@ class PullRequests(models.Model): pull_request=self.number, format_args={'user': login, 'pr': self}, ) - elif not param and is_author: + case commands.Reject() if is_author: newstate = RMINUS.get(self.state) if self.priority == 0 or newstate: if newstate: @@ -707,94 +655,128 @@ class PullRequests(models.Model): format_args={'user': login, 'pr': self}, ) self.unstage("unreviewed (r-) by %s", login) - ok = True else: msg = "r- makes no sense in the current PR state." - elif command == 'delegate': - if is_reviewer: - ok = True - Partners = self.env['res.partner'] - if param is True: - delegate = self.author - else: - delegate = Partners.search([('github_login', '=', param)]) or Partners.create({ - 'name': param, - 'github_login': param, - }) - delegate.write({'delegate_reviewer': [(4, self.id, 0)]}) - elif command == 'priority': - if is_admin: - ok = True - self.priority = param - if param == 0: - self.target.active_staging_id.cancel( - "P=0 on %s by %s, unstaging target %s", - self.display_name, - author.github_login, self.target.name, - ) - elif command == 'method': - if is_reviewer: - self.merge_method = param - ok = True - explanation = next(label for value, label in type(self).merge_method.selection if value == param) + case commands.MergeMethod() as command if is_reviewer: + self.merge_method = command.value + explanation = next(label for value, label in type(self).merge_method.selection if value == command.value) self.env.ref("runbot_merge.command.method")._send( repository=self.repository, pull_request=self.number, format_args={'new_method': explanation, 'pr': self, 'user': login}, ) - elif command == 'override': - overridable = author.override_rights\ - .filtered(lambda r: not r.repository_id or (r.repository_id == self.repository))\ - .mapped('context') - if param in overridable: - self.overrides = json.dumps({ - **json.loads(self.overrides), - param: { - 'state': 'success', - 'target_url': comment['html_url'], - 'description': f"Overridden by @{author.github_login}", - }, - }) - c = self.env['runbot_merge.commit'].search([('sha', '=', self.head)]) - if c: - c.to_check = True + case commands.Retry() if is_author: + if self.state == 'error': + self.state = 'ready' else: - c.create({'sha': self.head, 'statuses': '{}'}) - ok = True - else: - msg = "you are not allowed to override this status." - else: - # ignore unknown commands - continue + msg = "retry makes no sense when the PR is not in error." + case commands.Check() if is_author: + self.env['runbot_merge.fetch_job'].create({ + 'repository': self.repository.id, + 'number': self.number, + }) + case commands.Delegate(users) if is_reviewer: + if not users: + delegates = self.author + else: + delegates = self.env['res.partner'] + for login in users: + delegates |= delegates.search([('github_login', '=', login)]) or delegates.create({ + 'name': login, + 'github_login': login, + }) + delegates.write({'delegate_reviewer': [(4, self.id, 0)]}) + case commands.Priority() if is_admin: + self.priority = int(command) + if command is commands.Priority.NUKE: + self.target.active_staging_id.cancel( + "P=0 on %s by %s, unstaging target %s", + self.display_name, + author.github_login, self.target.name, + ) + case commands.Override(statuses): + for status in statuses: + overridable = author.override_rights\ + .filtered(lambda r: not r.repository_id or (r.repository_id == self.repository))\ + .mapped('context') + if status in overridable: + self.overrides = json.dumps({ + **json.loads(self.overrides), + status: { + 'state': 'success', + 'target_url': comment['html_url'], + 'description': f"Overridden by @{author.github_login}", + }, + }) + c = self.env['runbot_merge.commit'].search([('sha', '=', self.head)]) + if c: + c.to_check = True + else: + c.create({'sha': self.head, 'statuses': '{}'}) + else: + msg = f"you are not allowed to override {status!r}." + # FW + case commands.FWApprove(): + fwbot = True + if not self.source_id: + msg = "I can only do this on forward-port PRs and this is not one, see {}.".format( + self.repository.project_id.github_prefix + ) + elif not self.parent_id: + msg = "I can only do this on unmodified forward-port PRs, ask {}.".format( + self.repository.project_id.github_prefix + ) + else: + merge_bot = self.repository.project_id.github_prefix + # FIXME: classification of messages from child PR :( + # 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) + case commands.Close() if self.source_id._pr_acl(author).is_reviewer: + feedback(close=True, token='fp_github_token') + case commands.CI(run): + pr = (self.source_id or self) + if pr._pr_acl(author).is_reviewer: + pr.fw_policy = 'ci' if run else 'skipci' + feedback( + message="Waiting for CI to create followup forward-ports." if run else "Not waiting for CI to create followup forward-ports.", + token='fp_github_token', + ) + else: + fwbot = True + msg = "you can't configure ci." + case commands.Limit(branch): + fwbot = True + if is_author: + ping, msg = self._maybe_update_limit(branch or self.target.name) + if not ping: + feedback(message=msg, token='fp_github_token') + msg = None + else: + msg = "you can't set a forward-port limit." + # NO! + case _: + msg = f"you can't {command}. Skill issue." + if msg is not None: + rejections.append((fwbot, msg)) - _logger.info( - "%s %s(%s) on %s by %s (%s)", - "applied" if ok else "ignored", - command, param, self.display_name, - author.github_login, author.display_name, - ) - if ok: - applied.append(reformat(command, param)) - else: - ignored.append(reformat(command, param)) - msgs.append(msg or "you can't {}.".format(reformat(command, param))) + if not rejections: + _logger.info("%s (%s) applied %s", login, name, cmds) + return 'applied ' + ', '.join(map(str, cmds)) - if msgs: - joiner = ' ' if len(msgs) == 1 else '\n- ' - msgs.insert(0, "I'm sorry, @{}:".format(login)) - self.env['runbot_merge.pull_requests.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) - return '\n'.join(msg) + self.env.cr.rollback() + rejections_list = ''.join(f'\n- {r}' for fw, r in rejections if not fw) + fw_rejections_list = ''.join(f'\n- {r}' for fw, r in rejections if fw) + _logger.info("%s (%s) tried to apply %s%s", login, name, cmds, rejections_list + fw_rejections_list) + footer = '' if len(cmds) == len(rejections) else "\n\nFor your own safety I've ignored everything in your comment." + if rejections_list: + rejections = ' ' + rejections_list.removeprefix("\n- ") if rejections_list.count('\n- ') == 1 else rejections_list + feedback(message=f"@{login}{rejections}{footer}") + if fw_rejections_list: + rejections = ' ' + fw_rejections_list.removeprefix("\n- ") if fw_rejections_list.count('\n- ') else fw_rejections_list + feedback(message=f"@{login}{rejections}{footer}", token='fp_github_token') + return 'rejected' def _pr_acl(self, user): if not self: diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 1244bda2..5ddff161 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -1222,7 +1222,7 @@ class TestRetry: (users['reviewer'], 'hansen r+'), (users['reviewer'], 'hansen retry'), seen(env, prx, users), - (users['user'], "I'm sorry, @{reviewer}: retry makes no sense when the PR is not in error.".format_map(users)), + (users['user'], "@{reviewer} retry makes no sense when the PR is not in error.".format_map(users)), ] @pytest.mark.parametrize('disabler', ['user', 'other', 'reviewer']) @@ -2909,7 +2909,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'], "@{} this PR is already reviewed, reviewing it again is useless.".format( users['reviewer'])), ] @@ -2937,7 +2937,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'], "@{} you can't review+. Skill issue.".format(users['reviewer'])), ] def test_self_review_success(self, env, repo, users, config): @@ -3092,7 +3092,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"@{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 @@ -3394,6 +3394,8 @@ class TestRecognizeCommands: (users['reviewer'], "hansen do the thing"), (users['reviewer'], "hansen @bobby-b r+ :+1:"), seen(env, pr, users), + (users['user'], "@{reviewer} unknown command 'do'".format_map(users)), + (users['user'], "@{reviewer} unknown command '@bobby-b'".format_map(users)), ] class TestRMinus: diff --git a/runbot_merge/tests/test_status_overrides.py b/runbot_merge/tests/test_status_overrides.py index 9ffe7604..1bdc4877 100644 --- a/runbot_merge/tests/test_status_overrides.py +++ b/runbot_merge/tests/test_status_overrides.py @@ -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'], "@{} you are not allowed to override 'l/int'.".format(users['reviewer'])), (users['other'], "hansen override=l/int"), ] assert pr_id.statuses == '{}'