From cf4d1629074eb9d976f4bed1feb546d75964d290 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 22 Oct 2024 10:53:45 +0200 Subject: [PATCH] [ADD] *: PR backport wizard This is not a full user-driven backport thingie for now, just one admins can use to facilitate thing and debug issues with the system. May eventually graduate to a frontend feature. Fixes #925 --- forwardport/models/forwardport.py | 4 +- forwardport/models/project.py | 22 +++- forwardport/tests/test_backport.py | 117 ++++++++++++++++++ runbot_merge/models/__init__.py | 1 + runbot_merge/models/backport/__init__.py | 140 ++++++++++++++++++++++ runbot_merge/models/batch.py | 2 +- runbot_merge/security/ir.model.access.csv | 1 + 7 files changed, 280 insertions(+), 7 deletions(-) create mode 100644 forwardport/tests/test_backport.py create mode 100644 runbot_merge/models/backport/__init__.py diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index 6f9320e18..5e28e5e07 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -202,7 +202,7 @@ def _complete_batches(self): # NOTE: ports the new source everywhere instead of porting each # PR to the next step as it does not *stop* on conflict repo = git.get_local(source.repository) - conflict, head = source._create_fp_branch(repo, target) + conflict, head = source._create_port_branch(repo, target, forward=True) repo.push(git.fw_url(pr.repository), f'{head}:refs/heads/{ref}') remote_target = repository.fp_remote_target @@ -302,7 +302,7 @@ def _process_item(self): return repo = git.get_local(previous.repository) - conflicts, new_head = previous._create_fp_branch(repo, child.target) + conflicts, new_head = previous._create_port_branch(repo, child.target, forward=True) if conflicts: _, out, err, _ = conflicts diff --git a/forwardport/models/project.py b/forwardport/models/project.py index f26ebfd5d..00faa4690 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -34,6 +34,8 @@ from odoo.addons.runbot_merge.models.pull_requests import Branch from odoo.addons.runbot_merge.models.stagings_create import Message +Conflict = tuple[str, str, str, list[str]] + DEFAULT_DELTA = dateutil.relativedelta.relativedelta(days=3) _logger = logging.getLogger('odoo.addons.forwardport') @@ -287,17 +289,29 @@ def commits(self): return sorted(commits, key=lambda c: idx[c['sha']]) - def _create_fp_branch(self, source, target_branch): + def _create_port_branch( + self, + source: git.Repo, + target_branch: Branch, + *, + forward: bool, + ) -> tuple[typing.Optional[Conflict], str]: """ Creates a forward-port for the current PR to ``target_branch`` under ``fp_branch_name``. - :param target_branch: the branch to port forward to - :rtype: (None | (str, str, str, list[commit]), Repo) + :param source: the git repository to work with / in + :param target_branch: the branch to port ``self`` to + :param forward: whether this is a forward (``True``) or a back + (``False``) port + :returns: a conflict if one happened, and the head of the port branch + (either a succcessful port of the entire `self`, or a conflict + commit) """ logger = _logger.getChild(str(self.id)) root = self.root_id logger.info( - "Forward-porting %s (%s) to %s", + "%s %s (%s) to %s", + "Forward-porting" if forward else "Back-porting", self.display_name, root.display_name, target_branch.name ) fetch = source.with_config(stdout=subprocess.PIPE, stderr=subprocess.STDOUT).fetch() diff --git a/forwardport/tests/test_backport.py b/forwardport/tests/test_backport.py new file mode 100644 index 000000000..dbfd84080 --- /dev/null +++ b/forwardport/tests/test_backport.py @@ -0,0 +1,117 @@ +from xmlrpc.client import Fault + +import pytest + +from utils import make_basic, Commit, to_pr, seen + + +@pytest.fixture +def repo(env, config, make_repo): + repo, _ = make_basic(env, config, make_repo, statuses="default") + return repo + +@pytest.fixture +def pr_id(env, repo, config): + with repo: + repo.make_commits('c', Commit("c", tree={'x': '1'}), ref='heads/aref') + pr = repo.make_pr(target='c', head='aref') + repo.post_status('aref', 'success') + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + with repo: + repo.post_status('staging.c', 'success') + env.run_crons() + pr_id = to_pr(env, pr) + assert pr_id.merge_date + return pr_id + +@pytest.fixture +def backport_id(env, pr_id): + action = pr_id.backport() + backport_id = env[action['res_model']].browse([action['res_id']]) + assert backport_id._name == 'runbot_merge.pull_requests.backport' + assert backport_id + return backport_id + +def test_golden_path(env, repo, config, pr_id, backport_id, users): + branch_a, branch_b, _branch_c = env['runbot_merge.branch'].search([], order='name') + backport_id.target = branch_a.id + act2 = backport_id.action_apply() + env.run_crons() # run cron to update labels + + _, bp_id = env['runbot_merge.pull_requests'].search([], order='number') + assert bp_id.limit_id == branch_b + assert bp_id._name == act2['res_model'] + assert bp_id.id == act2['res_id'] + bp_head = repo.commit(bp_id.head) + assert repo.read_tree(bp_head) == { + 'f': 'e', + 'x': '1', + } + assert bp_head.message == f"""c + +X-original-commit: {pr_id.head}\ +""" + assert bp_id.message == f"[Backport] c\n\nBackport of {pr_id.display_name}" + assert repo.get_pr(bp_id.number).labels == {"backport"} + + # check that the backport can actually be merged and forward-ports successfully... + with repo: + repo.post_status(bp_id.head, 'success') + repo.get_pr(bp_id.number).post_comment("hansen r+", config['role_reviewer']['token']) + env.run_crons() + with repo: + repo.post_status('staging.a', 'success') + env.run_crons() + _pr, _backport, fw_id = env['runbot_merge.pull_requests'].search([], order='number') + fw_pr = repo.get_pr(fw_id.number) + assert fw_pr.comments == [ + seen(env, fw_pr, users), + (users['user'], '''\ +@{user} @{reviewer} this PR targets b and is the last of the forward-port chain. + +To merge the full chain, use +> @hansen r+ + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +'''.format_map(users)), + ] + +def test_conflict(env, repo, config, backport_id): + with repo: + repo.make_commits('a', Commit('conflict', tree={'x': '-1'}), ref='heads/a', make=False) + + branch_a, _branch_b, _branch_c = env['runbot_merge.branch'].search([], order='name') + backport_id.target = branch_a.id + with pytest.raises(Fault) as exc: + backport_id.action_apply() + assert exc.value.faultString == """\ +backport conflict: + +Auto-merging x +CONFLICT (add/add): Merge conflict in x +""" + +def test_target_error(env, config, backport_id): + branch_a, _branch_b, branch_c = env['runbot_merge.branch'].search([], order='name') + with pytest.raises(Fault) as exc: + backport_id.action_apply() + assert exc.value.faultString == "A backport needs a backport target" + + backport_id.target = branch_c.id + with pytest.raises(Fault) as exc: + backport_id.action_apply() + assert exc.value.faultString == "The backport branch needs to be before the source's branch (got 'c' and 'c')" + + backport_id.target = branch_a.id + backport_id.action_apply() + +@pytest.mark.skip( + reason="Currently no way to make just the PR creation fail, swapping the " + "fp_github_token for an invalid one breaks git itself" +) +def test_pr_fail(env, config, repo, pr_id, backport_id): + backport_id.target = env['runbot_merge.branch'].search([], order='name', limit=1).id + with pytest.raises(Fault) as exc: + backport_id.action_apply() + assert exc.value.faultString == 'Backport PR creation failure: ' diff --git a/runbot_merge/models/__init__.py b/runbot_merge/models/__init__.py index 4a25ecd44..54fd25d5f 100644 --- a/runbot_merge/models/__init__.py +++ b/runbot_merge/models/__init__.py @@ -8,5 +8,6 @@ from . import project_freeze from . import stagings_create from . import staging_cancel +from . import backport from . import events_sources from . import crons diff --git a/runbot_merge/models/backport/__init__.py b/runbot_merge/models/backport/__init__.py new file mode 100644 index 000000000..ac58ce29d --- /dev/null +++ b/runbot_merge/models/backport/__init__.py @@ -0,0 +1,140 @@ +import logging +import re +import secrets +import subprocess + +import requests + +from odoo import models, fields +from odoo.exceptions import UserError + +from ..batch import Batch +from ..project import Project +from ..pull_requests import Repository +from ... import git + +_logger = logging.getLogger(__name__) + + +class PullRequest(models.Model): + _inherit = 'runbot_merge.pull_requests' + + id: int + display_name: str + project: Project + repository: Repository + batch_id: Batch + + def backport(self) -> dict: + if len(self) != 1: + raise UserError(f"Backporting works one PR at a time, got {len(self)}") + + if len(self.batch_id.prs) > 1: + raise UserError("Automatic backport of multi-pr batches is not currently supported") + + if not self.project.fp_github_token: + raise UserError(f"Can not backport {self.display_name}: no token on project {self.project.display_name}") + + if not self.repository.fp_remote_target: + raise UserError(f"Can not backport {self.display_name}: no remote on {self.project.display_name}") + + w = self.env['runbot_merge.pull_requests.backport'].create({ + 'pr_id': self.id, + }) + return { + 'type': 'ir.actions.act_window', + 'name': f"Backport of {self.display_name}", + 'views': [('form', False)], + 'target': 'new', + 'res_model': w._name, + 'res_id': w.id, + } + +class PullRequestBackport(models.TransientModel): + _name = 'runbot_merge.pull_requests.backport' + _rec_name = 'pr_id' + + pr_id = fields.Many2one('runbot_merge.pull_requests', required=True) + target = fields.Many2one('runbot_merge.branch') + + def action_apply(self) -> dict: + if not self.target: + raise UserError("A backport needs a backport target") + + project = self.pr_id.project + branches = project._forward_port_ordered().ids + source = self.pr_id.source_id or self.pr_id + source_idx = branches.index(source.target.id) + if branches.index(self.target.id) >= source_idx: + raise UserError( + "The backport branch needs to be before the source's branch " + f"(got {self.target.name!r} and {source.target.name!r})" + ) + + _logger.info( + "backporting %s (on %s) to %s", + self.pr_id.display_name, + self.pr_id.target.name, + self.target.name, + ) + + bp_branch = "%s-%s-%s-bp" % ( + self.target.name, + self.pr_id.refname, + secrets.token_urlsafe(3), + ) + repo_id = self.pr_id.repository + repo = git.get_local(repo_id) + + old_map = self.pr_id.commits_map + self.pr_id.commits_map = "{}" + conflict, head = self.pr_id._create_port_branch(repo, self.target, forward=False) + self.pr_id.commits_map = old_map + + if conflict: + feedback = "\n".join(filter(None, conflict[1:3])) + raise UserError(f"backport conflict:\n\n{feedback}") + repo.push(git.fw_url(repo_id), f"{head}:refs/heads/{bp_branch}") + + self.env.cr.execute('LOCK runbot_merge_pull_requests IN SHARE MODE') + + owner, _repo = repo_id.fp_remote_target.split('/', 1) + message = source.message + f"\n\nBackport of {self.pr_id.display_name}" + title, body = re.fullmatch(r'(?P[^\n]+)\n*(?P<body>.*)', message, flags=re.DOTALL).groups() + + r = requests.post( + f'https://api.github.com/repos/{repo_id.name}/pulls', + headers={'Authorization': f'token {project.fp_github_token}'}, + json={ + 'base': self.target.name, + 'head': f'{owner}:{bp_branch}', + 'title': '[Backport]' + ('' if title[0] == '[' else ' ') + title, + 'body': body + } + ) + if not r.ok: + raise UserError(f"Backport PR creation failure: {r.text}") + + backport = self.env['runbot_merge.pull_requests']._from_gh(r.json()) + _logger.info("Created backport %s for %s", backport.display_name, self.pr_id.display_name) + + backport.write({ + 'merge_method': self.pr_id.merge_method, + # the backport's own forwardport should stop right before the + # original PR by default + 'limit_id': branches[source_idx - 1], + }) + self.env['runbot_merge.pull_requests.tagging'].create({ + 'repository': repo_id.id, + 'pull_request': backport.number, + 'tags_add': ['backport'], + }) + # scheduling fp followup probably doesn't make sense since we don't copy the fw_policy... + + return { + 'type': 'ir.actions.act_window', + 'name': "new backport", + 'views': [('form', False)], + 'res_model': backport._name, + 'res_id': backport.id, + } diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py index 5e7dbbfb6..df5cff036 100644 --- a/runbot_merge/models/batch.py +++ b/runbot_merge/models/batch.py @@ -326,7 +326,7 @@ def _port_forward(self): conflicts = {} for pr in prs: repo = git.get_local(pr.repository) - conflicts[pr], head = pr._create_fp_branch(repo, target) + conflicts[pr], head = pr._create_port_branch(repo, target, forward=True) repo.push(git.fw_url(pr.repository), f"{head}:refs/heads/{new_branch}") diff --git a/runbot_merge/security/ir.model.access.csv b/runbot_merge/security/ir.model.access.csv index 0e8b0e8dc..13949597f 100644 --- a/runbot_merge/security/ir.model.access.csv +++ b/runbot_merge/security/ir.model.access.csv @@ -32,3 +32,4 @@ access_runbot_merge_review_rights_2,Users can see partners,model_res_partner_rev access_runbot_merge_review_override_2,Users can see partners,model_res_partner_override,base.group_user,1,0,0,0 access_runbot_merge_pull_requests_feedback_template,access_runbot_merge_pull_requests_feedback_template,runbot_merge.model_runbot_merge_pull_requests_feedback_template,base.group_system,1,1,0,0 access_runbot_merge_patch,Patcher access,runbot_merge.model_runbot_merge_patch,runbot_merge.group_patcher,1,1,1,0 +access_runbot_merge_backport_admin,Admin access to backport wizard,model_runbot_merge_pull_requests_backport,runbot_merge.group_admin,1,1,1,0