diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index a0e36ac8..d5e63dbb 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -241,17 +241,15 @@ class ForwardPortTasks(models.Model, Queue): _logger.warning("Deleting %s:%s=%s", remote_target, ref, d.text) raise RuntimeError(f"Forwardport failure: {pr.display_name} ({r.text})") - new_pr = PullRequests._from_gh(r.json()) - _logger.info("Created forward-port PR %s", new_pr) - new_pr.write({ - 'batch_id': descendant.id, # should already be set correctly but... - 'merge_method': pr.merge_method, - 'source_id': source.id, - # only link to previous PR of sequence if cherrypick passed - # FIXME: apply parenting of siblings? Apply parenting *to* siblings? - 'parent_id': pr.id if not conflict else False, - 'detach_reason': "{1}\n{2}".format(*conflict).strip() if conflict else None, - }) + new_pr = PullRequests._from_gh( + r.json(), + batch_id=descendant.id, + merge_method=pr.merge_method, + source_id=source.id, + parent_id=False if conflict else pr.id, + detach_reason="{1}\n{2}".format(*conflict).strip() if conflict else None + ) + _logger.info("Created forward-port PR %s", new_pr.display_name) if conflict: self.env.ref('runbot_merge.forwardport.failure.conflict')._send( diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 0e9aed09..ebb63e80 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -213,10 +213,11 @@ class PullRequests(models.Model): new = super().create(to_create) for pr in new: - # added a new PR to an already forward-ported batch: port the PR - if self.env['runbot_merge.batch'].search_count([ + # added a new PR to an already forward-ported batch: immediately + # port forward to complete the genealogy + if not pr.source_id and self.env['runbot_merge.batch'].search_count([ ('parent_id', '=', pr.batch_id.id), - ]): + ], limit=1): self.env['forwardport.batches'].create({ 'batch_id': pr.batch_id.id, 'source': 'complete', diff --git a/forwardport/tests/test_batches.py b/forwardport/tests/test_batches.py index 7a53bfcf..13e14e0b 100644 --- a/forwardport/tests/test_batches.py +++ b/forwardport/tests/test_batches.py @@ -227,6 +227,13 @@ def test_add_to_forward_ported(env, config, make_repo, users): # region setup r1, _ = make_basic(env, config, make_repo, statuses="default") r2, fork2 = make_basic(env, config, make_repo, statuses="default") + project = env['runbot_merge.project'].search([]) + project.write({ + 'branch_ids': [(0, 0, {'name': 'd', 'sequence': 40})], + }) + with r1, r2: + r1.make_commits('c', Commit('1111', tree={'g': 'a'}), ref='heads/d') + r2.make_commits('c', Commit('1111', tree={'g': 'a'}), ref='heads/d') with r1: r1.make_commits('a', Commit('a', tree={'a': 'a'}), ref="heads/pr1") @@ -241,59 +248,60 @@ def test_add_to_forward_ported(env, config, make_repo, users): # region port forward pr1_a_id = to_pr(env, pr1_a) - pr1_b_id = pr1_a_id.forwardport_ids - assert pr1_b_id - with r1: - r1.post_status(pr1_b_id.head, 'success') - env.run_crons() - pr1_c_id = pr1_a_id.forwardport_ids - pr1_b_id - assert pr1_c_id + for branch in 'bcd': + next_pr = env['runbot_merge.pull_requests'].search([ + ('source_id', '=', pr1_a_id.id), + ('target.name', '=', branch), + ]) + assert next_pr + with r1: + r1.post_status(next_pr.head, 'success') + env.run_crons() # endregion # endregion + pr1_b_id, pr1_c_id, pr1_d_id = pr1_a_id.forwardport_ids[::-1] # new PR must be in fork for labels to actually match with r2, fork2: # branch in fork has no owner prefix, but HEAD for cross-repo PR does fork2.make_commits(r2.commit("b").id, Commit('b', tree={'b': 'b'}), ref=f'heads/{pr1_b_id.refname}') pr2_b = r2.make_pr(title="b", target="b", head=pr1_b_id.label) - r2.post_status(pr2_b.head, 'success') env.run_crons() pr2_b_id = to_pr(env, pr2_b) assert pr2_b_id.batch_id == pr1_b_id.batch_id - assert len(pr2_b_id.forwardport_ids) == 1, \ + assert len(pr2_b_id.forwardport_ids) == 2, \ "since the batch is already forward ported, the new PR should" \ " immediately be forward ported to match" - assert pr2_b_id.forwardport_ids.label == pr1_c_id.label - pr2_a = r1.get_pr(pr1_b_id.number) + assert pr2_b_id.forwardport_ids.mapped('label') == (pr1_d_id | pr1_c_id).mapped('label') + + assert not (b := env['forwardport.batches'].search([])),\ + f"there should not be any more forward porting, found {b.read(['source', 'pr_id'])}" + known_set = pr1_a_id | pr1_a_id.forwardport_ids | pr2_b_id | pr2_b_id.forwardport_ids + assert not (p := (env['runbot_merge.pull_requests'].search([]) - known_set)), \ + "there should not be any PR outside of the sequences we created, "\ + f"found {p.read(['source_id', 'parent_id', 'display_name'])}" + + with r2: + for pr_id in pr2_b_id | pr2_b_id.forwardport_ids: + r2.post_status(pr_id.head, 'success') + with r1, r2: - pr2_a.post_comment('hansen r+', config['role_reviewer']['token']) - pr2_b.post_comment("hansen r+", config['role_reviewer']['token']) + r1.get_pr(pr1_a_id.forwardport_ids[0].number)\ + .post_comment('hansen r+', config['role_reviewer']['token']) + r2.get_pr(pr2_b_id.forwardport_ids[0].number) \ + .post_comment('hansen r+', config['role_reviewer']['token']) env.run_crons() with r1, r2: - r1.post_status('staging.b', 'success') - r2.post_status('staging.b', 'success') + for branch in 'bcd': + r1.post_status(f'staging.{branch}', 'success') + r2.post_status(f'staging.{branch}', 'success') env.run_crons() - assert pr1_b_id.state == 'merged' - assert pr2_b_id.state == 'merged' - - assert len(pr2_b_id.forwardport_ids) == 1,\ - "verify that pr2_b did not get forward ported again on merge" - pr2_c = r2.get_pr(pr2_b_id.forwardport_ids.number) - assert pr2_c.comments == [ - seen(env, pr2_c, users), - (users['user'], '''\ -@{user} this PR targets c 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)), - ] + assert not env['runbot_merge.pull_requests'].search([]) - known_set, \ + "there should not be any PR outside of the sequences we created" def test_add_to_forward_port_conflict(env, config, make_repo, users): """If a PR is added to an existing forward port sequence, and it causes diff --git a/runbot_merge/models/backport/__init__.py b/runbot_merge/models/backport/__init__.py index 1812c222..d92a70f2 100644 --- a/runbot_merge/models/backport/__init__.py +++ b/runbot_merge/models/backport/__init__.py @@ -116,15 +116,15 @@ class PullRequestBackport(models.TransientModel): 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, + backport = self.env['runbot_merge.pull_requests']._from_gh( + r.json(), + 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], - }) + limit_id=branches[source_idx - 1], + ) + _logger.info("Created backport %s for %s", backport.display_name, self.pr_id.display_name) + self.env['runbot_merge.pull_requests.tagging'].create({ 'repository': repo_id.id, 'pull_request': backport.number, diff --git a/runbot_merge/models/batch.py b/runbot_merge/models/batch.py index 1480eaf0..458ba1a7 100644 --- a/runbot_merge/models/batch.py +++ b/runbot_merge/models/batch.py @@ -364,23 +364,21 @@ class Batch(models.Model): _logger.warning("Deleting %s:%s=%s", repo.fp_remote_target, new_branch, d.text) raise RuntimeError(f"Forwardport failure: {pr.display_name} ({r.text})") - new_pr = PRs._from_gh(r.json()) + report_conflicts = has_conflicts and self.fw_policy != 'skipmerge' + new_pr = PRs._from_gh( + r.json(), + merge_method=pr.merge_method, + source_id=source.id, + parent_id=False if report_conflicts else pr.id, + detach_reason=report_conflicts and "conflicts:\n{}".format( + '\n\n'.join( + f"{out}\n{err}".strip() + for _, out, err, _ in filter(None, conflicts.values()) + ) + ), + ) _logger.info("Created forward-port PR %s", new_pr) new_batch |= new_pr - - report_conflicts = has_conflicts and self.fw_policy != 'skipmerge' - - # allows PR author to close or skipci - new_pr.write({ - 'merge_method': pr.merge_method, - 'source_id': source.id, - # only link to previous PR of sequence if cherrypick passed - 'parent_id': pr.id if not report_conflicts else False, - 'detach_reason': "conflicts:\n{}".format('\n\n'.join( - f"{out}\n{err}".strip() - for _, out, err, _ in filter(None, conflicts.values()) - )) if report_conflicts else None, - }) if report_conflicts and pr.parent_id and pr.state not in ('merged', 'closed'): self.env.ref('runbot_merge.forwardport.failure.conflict')._send( repository=pr.repository, diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 081015a7..c9a092e1 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1397,7 +1397,7 @@ For your own safety I've ignored *everything in your entire comment*. ) return prs - def _from_gh(self, description, author=None, branch=None, repo=None): + def _from_gh(self, description, author=None, branch=None, repo=None, **kwargs): if repo is None: repo = self.env['runbot_merge.repository'].search([ ('name', '=', description['base']['repo']['full_name']), @@ -1423,6 +1423,7 @@ For your own safety I've ignored *everything in your entire comment*. 'squash': description['commits'] == 1, 'message': utils.make_message(description), 'draft': description['draft'], + **kwargs, }) def write(self, vals):