diff --git a/forwardport/models/forwardport.py b/forwardport/models/forwardport.py index 78e75bac..3a0daab8 100644 --- a/forwardport/models/forwardport.py +++ b/forwardport/models/forwardport.py @@ -91,6 +91,27 @@ class UpdateQueue(models.Model, Queue): previous = self.new_root with ExitStack() as s: for child in self.new_root._iter_descendants(): + _logger.info( + "Re-port %s from %s (changed root %s -> %s)", + child.display_name, + previous.display_name, + self.original_root.display_name, + self.new_root.display_name + ) + if child.state in ('closed', 'merged'): + self.env['runbot_merge.pull_requests.feedback'].create({ + 'repository': child.repository.id, + 'pull_request': child.number, + 'message': "Ancestor PR %s has been updated but this PR" + " is %s and can't be updated to match." + "\n\n" + "You may want or need to manually update any" + " followup PR." % ( + self.new_root.display_name, + child.state, + ) + }) + return # QUESTION: update PR to draft if there are conflicts? _, working_copy = previous._create_fp_branch( child.target, child.refname, s) diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 5d1a4ce2..61ed69e0 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- import collections +import sys import time from datetime import datetime, timedelta from operator import itemgetter @@ -352,6 +353,98 @@ a '''), } +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) + child. + + Turns out the issue is the followup: given a PR a and forward port targets + B -> C -> D. When a is merged we get b, c and d. If c gets merged *then* + b gets updated, the fwbot will update c in turn, then it will look for the + head of the updated c in order to create d. + + However it *will not* find that head, as update events don't get propagated + on closed PRs (this is generally a good thing). As a result, the sanity + check when trying to port c to d will fail. + + After checking with nim, the safest behaviour seems to be: + + * stop at the update of the first closed or merged PR + * signal on that PR that something fucky happened + * also maybe disable or exponentially backoff the update job after some + number of attempts? + """ + prod, other = make_basic(env, config, make_repo) + # add a 4th branch + with prod: + prod.make_ref('heads/d', prod.commit('c').id) + env['runbot_merge.project'].search([]).write({ + 'branch_ids': [(0, 0, { + 'name': 'd', 'fp_sequence': -1, 'fp_target': True, + })] + }) + + with prod: + [c] = prod.make_commits('a', Commit('p_0', tree={'0': '0'}), ref='heads/hugechange') + pr = prod.make_pr(target='a', head='hugechange') + prod.post_status(c, 'success', 'legal/cla') + prod.post_status(c, 'success', 'ci/runbot') + pr.post_comment('hansen r+', config['role_reviewer']['token']) + env.run_crons() + with prod: + prod.post_status('staging.a', 'success', 'legal/cla') + prod.post_status('staging.a', 'success', 'ci/runbot') + env.run_crons() + + pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number') + with prod: + prod.post_status(pr1.head, 'success', 'legal/cla') + prod.post_status(pr1.head, 'success', 'ci/runbot') + env.run_crons() + + pr0, pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number') + with prod: + prod.get_pr(pr2.number).post_comment('hansen r+', config['role_reviewer']['token']) + prod.post_status(pr2.head, 'success', 'legal/cla') + prod.post_status(pr2.head, 'success', 'ci/runbot') + env.run_crons() + + assert pr2.staging_id + with prod: + prod.post_status('staging.c', 'success', 'legal/cla') + prod.post_status('staging.c', 'success', 'ci/runbot') + env.run_crons() + assert pr2.state == 'merged' + assert prod.get_pr(pr2.number).state == 'closed' + + # now we can try updating pr1 and see what happens + repo, ref = prod.get_pr(pr1.number).branch + with repo: + repo.make_commits( + pr1.target.name, + Commit('2', tree={'0': '0', '1': '1'}), + ref='heads/%s' % ref, + make=False + ) + updates = env['forwardport.updates'].search([]) + assert updates + assert updates.original_root == pr0 + assert updates.new_root == pr1 + env.run_crons() + assert not pr1.parent_id + assert not env['forwardport.updates'].search([]) + + assert prod.get_pr(pr2.number).comments == [ + (users['user'], '''This PR targets c and is part of the forward-port chain. Further PRs will be created up to d. + +More info at https://github.com/odoo/odoo/wiki/Mergebot-and-Forwardbot#forward-port +'''), + (users['reviewer'], 'hansen r+'), + (users['user'], """Ancestor PR %s has been updated but this PR is merged and can't be updated to match. + +You may want or need to manually update any followup PR.""" % pr1.display_name) + ] + def test_conflict(env, config, make_repo): prod, other = make_basic(env, config, make_repo) # reset b to b~1 (g=a) parent so there's no b -> c conflict