diff --git a/conftest.py b/conftest.py index 498605b3..b5e3da6c 100644 --- a/conftest.py +++ b/conftest.py @@ -777,6 +777,15 @@ class Repo: self.reset = reset MakeCommit = Repo.Commit ct = itertools.count() + +class Comment(tuple): + def __new__(cls, c): + self = super(Comment, cls).__new__(cls, (c['user']['login'], c['body'])) + self._c = c + return self + def __getitem__(self, item): + return self._c[item] + class PR: def __init__(self, repo, number): self.repo = repo @@ -815,10 +824,7 @@ class PR: def comments(self): r = self.repo._session.get('https://api.github.com/repos/{}/issues/{}/comments'.format(self.repo.name, self.number)) assert 200 <= r.status_code < 300, r.json() - return [ - (c['user']['login'], c['body']) - for c in r.json() - ] + return [Comment(c) for c in r.json()] @property def ref(self): diff --git a/forwardport/models/project.py b/forwardport/models/project.py index a1454d3c..d9528423 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -280,12 +280,12 @@ class PullRequests(models.Model): tokens = [ token - for line in re.findall('^\s*[@|#]?{}:? (.*)$'.format(self.repository.project_id.fp_github_name), comment, re.MULTILINE | re.IGNORECASE) + 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, 50) + utils.shorten(comment['body'] or '', 50) ) return @@ -323,7 +323,7 @@ class PullRequests(models.Model): # don't update the root ever for pr in filter(lambda p: p.parent_id, self._iter_ancestors()): # only the author is delegated explicitely on the - pr._parse_commands(author, merge_bot + ' r+', login) + 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." if self.source_id._pr_acl(author).is_reviewer: diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index ab52d7ea..e41fc869 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -259,7 +259,7 @@ def handle_comment(env, event): if event['action'] != 'created': return "Ignored: action (%r) is not 'created'" % event['action'] - return _handle_comment(env, repo, issue, author, comment) + return _handle_comment(env, repo, issue, event['comment']) def handle_review(env, event): repo = event['repository']['full_name'] @@ -272,7 +272,7 @@ def handle_review(env, event): return "Ignored: action (%r) is not 'submitted'" % event['action'] return _handle_comment( - env, repo, pr, author, comment, + env, repo, pr, event['review'], target=event['pull_request']['base']['ref']) def handle_ping(env, event): @@ -287,14 +287,14 @@ EVENTS = { 'ping': handle_ping, } -def _handle_comment(env, repo, issue, author, comment, target=None): +def _handle_comment(env, repo, issue, comment, target=None): repository = env['runbot_merge.repository'].search([('name', '=', repo)]) - if not repository.project_id._find_commands(comment): + if not repository.project_id._find_commands(comment['body'] or ''): return "No commands, ignoring" pr = env['runbot_merge.pull_requests']._get_or_schedule(repo, issue, target=target) if not pr: return "Unknown PR, scheduling fetch" - partner = env['res.partner'].search([('github_login', '=', author), ]) - return pr._parse_commands(partner, comment, author) + partner = env['res.partner'].search([('github_login', '=', comment['user']['login'])]) + return pr._parse_commands(partner, comment, comment['user']['login']) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 19a6dbd7..b0ecdbf0 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -623,6 +623,7 @@ class PullRequests(models.Model): delegates = fields.Many2many('res.partner', help="Delegate reviewers, not intrinsically reviewers but can review this PR") priority = fields.Integer(default=2, index=True) + overrides = fields.Char(required=True, default='{}') statuses = fields.Text(compute='_compute_statuses') status = fields.Char(compute='_compute_statuses') previous_failure = fields.Char(default='{}') @@ -702,12 +703,13 @@ class PullRequests(models.Model): Commits = self.env['runbot_merge.commit'] for pr in self: c = Commits.search([('sha', '=', pr.head)]) - if not (c and c.statuses): + st = json.loads(c.statuses or '{}') + statuses = {**st, **json.loads(pr.overrides)} + if not statuses: pr.status = pr.statuses = False continue - statuses = json.loads(c.statuses) - pr.statuses = pprint.pformat(statuses) + pr.statuses = pprint.pformat(st) st = 'success' for ci in pr.repository.status_ids._for_pr(pr): @@ -754,21 +756,16 @@ class PullRequests(models.Model): def _parse_command(self, commandstring): for m in re.finditer( - r'(\S+?)(?:([+-])|=(\S*))?(?:\s|$)', + r'(\S+?)(?:([+-])|=(\S*))?(?=\s|$)', commandstring, ): name, flag, param = m.groups() - if name in ('retry', 'check'): - yield (name, None) - elif name in ('r', 'review'): - if flag == '+': - yield ('review', True) - elif flag == '-': - yield ('review', False) + if name == 'r': + name = 'review' + if flag in ('+', '-'): + yield name, flag == '+' elif name == 'delegate': - if flag == '+': - yield ('delegate', True) - elif param: + if param: yield ('delegate', [ p.lstrip('#@') for p in param.split(',') @@ -778,6 +775,8 @@ class PullRequests(models.Model): 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. @@ -807,18 +806,18 @@ class PullRequests(models.Model): commands = dict( ps - for m in self.repository.project_id._find_commands(comment) + 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, - utils.shorten(comment, 50) + utils.shorten(comment['body'] or '', 50) ) return 'ok' Feedback = self.env['runbot_merge.pull_requests.feedback'] - if not is_author: + if not (is_author or 'override' in commands): # no point even parsing commands _logger.info("ignoring comment of %s (%s): no ACL to %s", login, name, self.display_name) @@ -931,6 +930,27 @@ class PullRequests(models.Model): 'pull_request': self.number, 'message':"Merge method set to %s" % explanation }) + elif command == 'override': + overridable = author.override_rights\ + .filtered(lambda r: 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 + else: + c.create({'sha': self.head, 'statuses': '{}'}) + ok = True + else: + msg = "You are not allowed to do that." _logger.info( "%s %s(%s) on %s by %s (%s)", @@ -980,17 +1000,18 @@ class PullRequests(models.Model): failed = self.browse(()) for pr in self: required = pr.repository.status_ids._for_pr(pr).mapped('context') + sts = {**statuses, **json.loads(pr.overrides)} success = True for ci in required: - st = state_(statuses, ci) or 'pending' + st = state_(sts, ci) or 'pending' if st == 'success': continue success = False if st in ('error', 'failure'): failed |= pr - pr._notify_ci_new_failure(ci, to_status(statuses.get(ci.strip(), 'pending'))) + pr._notify_ci_new_failure(ci, to_status(sts.get(ci.strip(), 'pending'))) if success: oldstate = pr.state if oldstate == 'opened': diff --git a/runbot_merge/models/res_partner.py b/runbot_merge/models/res_partner.py index ec11f920..70ec79ad 100644 --- a/runbot_merge/models/res_partner.py +++ b/runbot_merge/models/res_partner.py @@ -13,6 +13,7 @@ class Partner(models.Model): delegate_reviewer = fields.Many2many('runbot_merge.pull_requests') formatted_email = fields.Char(string="commit email", compute='_rfc5322_formatted') review_rights = fields.One2many('res.partner.review', 'partner_id') + override_rights = fields.One2many('res.partner.override', 'partner_id') def _auto_init(self): res = super(Partner, self)._auto_init() @@ -72,3 +73,17 @@ class ReviewRights(models.Model): def name_search(self, name='', args=None, operator='ilike', limit=100): return self.search(args + [('repository_id.name', operator, name)], limit=limit).name_get() + +class OverrideRights(models.Model): + _name = 'res.partner.override' + _description = 'lints which the partner can override' + + partner_id = fields.Many2one('res.partner', required=True, ondelete='cascade') + repository_id = fields.Many2one('runbot_merge.repository', required=True) + context = fields.Char(required=True) + + def name_get(self): + return [ + (r.id, f'{r.repository.name}: {r.context}') + for r in self + ] diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index cc5a4668..42dc408c 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/runbot_merge/security/ir.model.access.csv @@ -12,8 +12,11 @@ access_runbot_merge_batch_admin,Admin access to batches,model_runbot_merge_batch access_runbot_merge_fetch_job_admin,Admin access to fetch jobs,model_runbot_merge_fetch_job,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_pull_requests_feedback_admin,Admin access to feedback,model_runbot_merge_pull_requests_feedback,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_review_rights,Admin access to review permissions,model_res_partner_review,runbot_merge.group_admin,1,1,1,1 +access_runbot_merge_review_override,Admin access to override permissions,model_res_partner_override,runbot_merge.group_admin,1,1,1,1 access_runbot_merge_project,User access to project,model_runbot_merge_project,base.group_user,1,0,0,0 access_runbot_merge_repository,User access to repo,model_runbot_merge_repository,base.group_user,1,0,0,0 access_runbot_merge_branch,User access to branches,model_runbot_merge_branch,base.group_user,1,0,0,0 access_runbot_merge_pull_requests,User access to PR,model_runbot_merge_pull_requests,base.group_user,1,0,0,0 access_runbot_merge_pull_requests_feedback,Users have no reason to access feedback,model_runbot_merge_pull_requests_feedback,,0,0,0,0 +access_runbot_merge_review_rights_2,Users can see partners,model_res_partner_review,base.group_user,1,0,0,0 +access_runbot_merge_review_override_2,Users can see partners,model_res_partner_override,base.group_user,1,0,0,0 diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index d071be84..fdce2444 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -1,3 +1,8 @@ +import json + +from utils import Commit + + def test_partner_merge(env): p_src = env['res.partner'].create({ 'name': 'kfhsf', @@ -20,3 +25,66 @@ def test_partner_merge(env): assert not p_src.exists() assert p_dest.name == 'Partner P. Partnersson' assert p_dest.github_login == 'xxx' + +def test_override(env, project, make_repo, users, setreviewers, config): + """ + Test that we can override a status on a PR: + + * @mergebot override context=status + * target url should be the comment (?) + * description should be overridden by + """ + repo = make_repo('repo') + repo_id = env['runbot_merge.repository'].create({ + 'project_id': project.id, + 'name': repo.name, + 'status_ids': [(0, 0, {'context': 'l/int'})] + }) + setreviewers(*project.repo_ids) + # "other" can override the lint + env['res.partner'].create({ + 'name': config['role_other'].get('name', 'Other'), + 'github_login': users['other'], + 'override_rights': [(0, 0, { + 'repository_id': repo_id.id, + 'context': 'l/int', + })] + }) + + with repo: + m = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') + + repo.make_commits(m, Commit('pr', tree={'a': '2'}), ref='heads/change') + pr = repo.make_pr(target='master', title='super change', head='change') + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + + pr_id = env['runbot_merge.pull_requests'].search([ + ('repository.name', '=', repo.name), + ('number', '=', pr.number) + ]) + assert pr_id.state == 'approved' + + with repo: + pr.post_comment('hansen override=l/int', config['role_reviewer']['token']) + env.run_crons() + assert pr_id.state == 'approved' + + with repo: + pr.post_comment('hansen override=l/int', config['role_other']['token']) + env.run_crons() + assert pr_id.state == 'ready' + + comments = pr.comments + assert comments == [ + (users['reviewer'], 'hansen r+'), + (users['reviewer'], 'hansen override=l/int'), + (users['user'], "I'm sorry, @{}. You are not allowed to do that.".format(users['reviewer'])), + (users['other'], "hansen override=l/int"), + ] + assert pr_id.statuses == '{}' + assert json.loads(pr_id.overrides) == {'l/int': { + 'state': 'success', + 'target_url': comments[-1]['html_url'], + 'description': 'Overridden by @{}'.format(users['other']), + }} diff --git a/runbot_merge/views/mergebot.xml b/runbot_merge/views/mergebot.xml index b6d27b55..a4624709 100644 --- a/runbot_merge/views/mergebot.xml +++ b/runbot_merge/views/mergebot.xml @@ -151,6 +151,7 @@ + diff --git a/runbot_merge/views/res_partner.xml b/runbot_merge/views/res_partner.xml index f00032e5..50ac7b6d 100644 --- a/runbot_merge/views/res_partner.xml +++ b/runbot_merge/views/res_partner.xml @@ -35,6 +35,9 @@ + + +