From 6b1f698c235bb721e1921fcfe76df7f53b52b8ab Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 27 Jul 2021 09:17:18 +0200 Subject: [PATCH] [IMP] forwardport: handling of updates causing conflicts on followups If a PR is updated and has extent forward-ports, those forwardports get updated automatically ("followup"). However there is an issue if the udpate causes a conflict in the followup: the conflict gets silently pushed, and may fairly easily get merged if it occurs in an area which the CI doesn't cover. It's unclear what the policy really should be for this issue, and there is no real way to *block* a pull request at the moment (save by putting it in error at the mergebot level I guess?), so for now clearly notify the user on both the modified PR and the followup, with a comment on both. We may want to revisit this eventually. Fixes #467 --- forwardport/models/forwardport.py | 37 ++++++-- forwardport/tests/test_updates.py | 144 +++++++++++++++++++----------- 2 files changed, 124 insertions(+), 57 deletions(-) diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index be81981d..8caaca33 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -79,6 +79,15 @@ class BatchQueue(models.Model, Queue): ) batch.active = False + +CONFLICT_TEMPLATE = "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 " \ + "{previous.head} has caused a conflict in this pull request, " \ + "data may have been lost." class UpdateQueue(models.Model, Queue): _name = 'forwardport.updates' _description = 'if a forward-port PR gets updated & has followups (cherrypick succeeded) the followups need to be updated as well' @@ -89,6 +98,7 @@ class UpdateQueue(models.Model, Queue): new_root = fields.Many2one('runbot_merge.pull_requests') def _process_item(self): + Feedback = self.env['runbot_merge.pull_requests.feedback'] previous = self.new_root with ExitStack() as s: for child in self.new_root._iter_descendants(): @@ -100,7 +110,7 @@ class UpdateQueue(models.Model, Queue): self.new_root.display_name ) if child.state in ('closed', 'merged'): - self.env['runbot_merge.pull_requests.feedback'].create({ + Feedback.create({ 'repository': child.repository.id, 'pull_request': child.number, 'message': "Ancestor PR %s has been updated but this PR" @@ -108,14 +118,31 @@ class UpdateQueue(models.Model, Queue): "\n\n" "You may want or need to manually update any" " followup PR." % ( - self.new_root.display_name, - child.state, - ) + self.new_root.display_name, + child.state, + ) }) return # QUESTION: update PR to draft if there are conflicts? - _, working_copy = previous._create_fp_branch( + conflicts, working_copy = previous._create_fp_branch( child.target, child.refname, s) + if conflicts: + _, out, err = conflicts + Feedback.create({ + 'repository': previous.repository.id, + 'pull_request': previous.number, + 'message': CONFLICT_TEMPLATE.format( + previous=previous, + next=child + ) + }) + Feedback.create({ + 'repository': child.repository.id, + 'pull_request': child.number, + 'message': CHILD_CONFLICT.format(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 '') + }) new_head = working_copy.stdout().rev_parse(child.refname).stdout.decode().strip() commits_count = int(working_copy.stdout().rev_list( diff --git a/forwardport/tests/test_updates.py b/forwardport/tests/test_updates.py index bea30fa8..5c5f60d9 100644 --- a/forwardport/tests/test_updates.py +++ b/forwardport/tests/test_updates.py @@ -2,11 +2,13 @@ Test cases for updating PRs during after the forward-porting process after the initial merge has succeeded (and forward-porting has started) """ +import re import sys import pytest -from utils import seen, re_matches, Commit, make_basic +from utils import seen, re_matches, Commit, make_basic, to_pr + def test_update_pr(env, config, make_repo, users): """ Even for successful cherrypicks, it's possible that e.g. CI doesn't @@ -119,57 +121,6 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port 'x': '5' }, "the followup FP should also have the update" - with pr_repo: - pr_repo.make_commits( - pr1_id.target.name, - Commit('fire!', tree={'h': '0'}), - ref='heads/%s' % pr_ref, - ) - env.run_crons() - # since there are PRs, this is going to update pr2 as broken - assert prod.read_tree(prod.commit(pr1_id.head)) == { - 'f': 'c', - 'g': 'b', - 'h': '0' - } - assert prod.read_tree(prod.commit(pr2_id.head)) == { - 'f': 'c', - 'g': 'a', - 'h': re_matches(r'''<<<\x3c<<< HEAD -a -======= -0 ->>>\x3e>>> [0-9a-f]{7,}.* -'''), - } - [project] = env['runbot_merge.project'].search([]) - pr2 = prod.get_pr(pr2_id.number) - # fail pr2 then fwbot r+ to check that we get a warning - with prod: - prod.post_status(pr2_id.head, 'failure', 'ci/runbot') - env.run_crons() # parse commit statuses - with prod: - pr2.post_comment(project.fp_github_name + ' r+', config['role_reviewer']['token']) - env.run_crons() # send feedback - - assert pr2.comments == [ - seen(env, pr2, users), - (users['user'], """Ping @{}, @{} -This PR targets c and is the last of the forward-port chain containing: -* {} - -To merge the full chain, say -> @{} r+ - -More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port -""".format(users['user'], users['reviewer'], pr1_id.display_name, project.fp_github_name)), - (users['user'], 'Ping @{}, @{}\n\nci/runbot failed on this forward-port PR'.format( - users['user'], users['reviewer'] - )), - (users['reviewer'], project.fp_github_name + ' r+'), - (users['user'], '@{}, you may want to rebuild or fix this PR as it has failed CI.'.format(users['reviewer'])), - ] - def test_update_merged(env, make_repo, config, users): """ Strange things happen when an FP gets closed / merged but then its parent is modified and the forwardport tries to update the (now merged) @@ -365,3 +316,92 @@ def test_duplicate_fw(env, make_repo, setreviewers, config, users): env.run_crons() # env.run_crons() assert PRs.search([], order='number') == pr_ids + +def test_subsequent_conflict(env, make_repo, config, users): + """ Test for updating an fw PR in the case where it produces a conflict in + the followup. Cf #467. + """ + repo, fork = make_basic(env, config, make_repo) + + # create a PR in branch A which adds a new file + with repo: + repo.make_commits('a', Commit('newfile', tree={'x': '0'}), ref='heads/pr1') + pr_1 = repo.make_pr(target='a', head='pr1') + repo.post_status('pr1', 'success', 'legal/cla') + repo.post_status('pr1', 'success', 'ci/runbot') + pr_1.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + with repo: + repo.post_status('staging.a', 'success', 'legal/cla') + repo.post_status('staging.a', 'success', 'ci/runbot') + env.run_crons() + pr1_id = to_pr(env, pr_1) + assert pr1_id.state == 'merged' + + pr2_id = env['runbot_merge.pull_requests'].search([('source_id', '=', pr1_id.id)]) + assert pr2_id + with repo: + repo.post_status(pr2_id.head, 'success', 'legal/cla') + repo.post_status(pr2_id.head, 'success', 'ci/runbot') + env.run_crons() + + pr3_id = env['runbot_merge.pull_requests'].search([('parent_id', '=', pr2_id.id)]) + assert pr3_id + assert repo.read_tree(repo.commit(pr3_id.head)) == { + 'f': 'c', + 'g': 'a', + 'h': 'a', + 'x': '0', + } + + # update pr2: add a file "h" + pr2 = repo.get_pr(pr2_id.number) + t = {**repo.read_tree(repo.commit(pr2_id.head)), 'h': 'conflict!'} + with fork: + fork.make_commits(pr2_id.target.name, Commit('newfiles', tree=t), ref=pr2.ref, make=False) + env.run_crons() + + assert repo.read_tree(repo.commit(pr3_id.head)) == { + 'f': 'c', + 'g': 'a', + 'h': re_matches(r'''<<<\x3c<<< HEAD +a +======= +conflict! +>>>\x3e>>> [0-9a-f]{7,}.* +'''), + 'x': '0', + } + # skip comments: + # 1. link to mergebot status page + # 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 " + 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 " + f"differently, or fix the issue by hand on " + f"{pr3_id.display_name}.") + ] + # skip comments: + # 1. link to status page + # 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 \ +conflict in this pull request, data may have been lost. + +stdout: +``` +CONFLICT \(add/add\): Merge conflict in h +Auto-merging h +``` + +stderr: +``` +\\d{{2}}:\\d{{2}}:\\d{{2}}.\\d+ .* {pr2_id.head} +error: could not apply [0-9a-f]+\\.\\.\\. newfiles +hint: after resolving the conflicts, mark the corrected paths +.*''', re.DOTALL)) + ]