diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 0c2cd329..2a780e27 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -317,13 +317,11 @@ class PullRequests(models.Model): 'message': "I'm sorry, @{}. You can't review+.".format(login), }) continue + merge_bot = self.repository.project_id.github_prefix # don't update the root ever for pr in filter(lambda p: p.parent_id, self._iter_ancestors()): - newstate = RPLUS.get(pr.state) - if newstate: - pr.state = newstate - pr.reviewed_by = author - # TODO: logging & feedback + # only the author is delegated explicitely on the + pr._parse_commands(author, merge_bot + ' r+', login) elif token == 'close': close = False message = "I'm sorry, @{}. I can't close this PR for you." @@ -653,19 +651,16 @@ class PullRequests(models.Model): _logger.info("Created forward-port PR %s", new_pr) new_batch |= new_pr + # delegate original author on merged original PR & on new PR so + # they can r+ the forward ports (via mergebot or forwardbot) + source.delegates |= source.author 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 has_conflicts else False, - }) - # delegate original author on merged original PR & on new PR so - # they can r+ the forward ports (via mergebot or forwardbot) - source.author.write({ - 'delegate_reviewer': [ - (4, source.id, False), - (4, new_pr.id, False), - ] + # copy all delegates of source to new + 'delegates': [(6, False, source.delegates.ids)] }) # not great but we probably want to avoid the risk of the webhook # creating the PR from under us. There's still a "hole" between diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index 0c06b819..3374cb29 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -246,7 +246,7 @@ def test_update_pr(env, config, make_repo, users): # should merge the staging then create the FP PR env.run_crons() - pr0, pr1 = env['runbot_merge.pull_requests'].search([], order='number') + pr0_id, pr1_id = env['runbot_merge.pull_requests'].search([], order='number') fp_intermediate = (users['user'], '''\ This PR targets b and is part of the forward-port chain. Further PRs will be created up to c. @@ -260,23 +260,23 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot-and-Forwardbot#forward-p # some delivery lag allowing for the cron to run between each delivery for st, ctx in [('failure', 'ci/runbot'), ('failure', 'ci/runbot'), ('success', 'legal/cla'), ('success', 'legal/cla')]: with prod: - prod.post_status(pr1.head, st, ctx) + prod.post_status(pr1_id.head, st, ctx) env.run_crons() with prod: # should be ignored because the description doesn't matter - prod.post_status(pr1.head, 'failure', 'ci/runbot', description="HAHAHAHAHA") + prod.post_status(pr1_id.head, 'failure', 'ci/runbot', description="HAHAHAHAHA") env.run_crons() # check that FP did not resume & we have a ping on the PR - assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\ + assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\ "forward port should not continue on CI failure" - pr1_remote = prod.get_pr(pr1.number) + pr1_remote = prod.get_pr(pr1_id.number) assert pr1_remote.comments == [fp_intermediate, ci_warning] # it was a false positive, rebuild... it fails again! with prod: - prod.post_status(pr1.head, 'failure', 'ci/runbot', target_url='http://example.org/4567890') + prod.post_status(pr1_id.head, 'failure', 'ci/runbot', target_url='http://example.org/4567890') env.run_crons() # check that FP did not resume & we have a ping on the PR - assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\ + assert env['runbot_merge.pull_requests'].search([], order='number') == pr0_id | pr1_id,\ "ensure it still hasn't restarted" assert pr1_remote.comments == [fp_intermediate, ci_warning, ci_warning] @@ -285,44 +285,44 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot-and-Forwardbot#forward-p # rebuild again, finally passes with prod: - prod.post_status(pr1.head, 'success', 'ci/runbot') + prod.post_status(pr1_id.head, 'success', 'ci/runbot') env.run_crons() - pr0, pr1, pr2 = env['runbot_merge.pull_requests'].search([], order='number') - assert pr1.parent_id == pr0 - assert pr2.parent_id == pr1 - pr1_head = pr1.head - pr2_head = pr2.head + pr0_id, pr1_id, pr2_id = env['runbot_merge.pull_requests'].search([], order='number') + assert pr1_id.parent_id == pr0_id + assert pr2_id.parent_id == pr1_id + pr1_head = pr1_id.head + pr2_head = pr2_id.head # turns out branch b is syntactically but not semantically compatible! It # needs x to be 5! - pr_repo, pr_ref = prod.get_pr(pr1.number).branch + pr_repo, pr_ref = prod.get_pr(pr1_id.number).branch with pr_repo: # force-push correct commit to PR's branch [new_c] = pr_repo.make_commits( - pr1.target.name, + pr1_id.target.name, Commit('whop whop', tree={'x': '5'}), ref='heads/%s' % pr_ref, make=False ) env.run_crons() - assert pr1.head == new_c != pr1_head, "the FP PR should be updated" - assert not pr1.parent_id, "the FP PR should be detached from the original" + assert pr1_id.head == new_c != pr1_head, "the FP PR should be updated" + assert not pr1_id.parent_id, "the FP PR should be detached from the original" assert pr1_remote.comments == [ fp_intermediate, ci_warning, ci_warning, - (users['user'], "This PR was modified / updated and has become a normal PR. It should be merged the normal way (via @%s)" % pr1.repository.project_id.github_prefix), + (users['user'], "This PR was modified / updated and has become a normal PR. It should be merged the normal way (via @%s)" % pr1_id.repository.project_id.github_prefix), ], "users should be warned that the PR has become non-FP" # NOTE: should the followup PR wait for pr1 CI or not? - assert pr2.head != pr2_head - assert pr2.parent_id == pr1, "the followup PR should still be linked" + assert pr2_id.head != pr2_head + assert pr2_id.parent_id == pr1_id, "the followup PR should still be linked" - assert prod.read_tree(prod.commit(pr1.head)) == { + assert prod.read_tree(prod.commit(pr1_id.head)) == { 'f': 'c', 'g': 'b', 'x': '5' }, "the FP PR should have the new code" - assert prod.read_tree(prod.commit(pr2.head)) == { + assert prod.read_tree(prod.commit(pr2_id.head)) == { 'f': 'c', 'g': 'a', 'h': 'a', @@ -331,18 +331,18 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot-and-Forwardbot#forward-p with pr_repo: pr_repo.make_commits( - pr1.target.name, + 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.head)) == { + assert prod.read_tree(prod.commit(pr1_id.head)) == { 'f': 'c', 'g': 'b', 'h': '0' } - assert prod.read_tree(prod.commit(pr2.head)) == { + assert prod.read_tree(prod.commit(pr2_id.head)) == { 'f': 'c', 'g': 'a', 'h': re_matches(r'''<<<\x3c<<< HEAD @@ -352,6 +352,32 @@ a >>>\x3e>>> [0-9a-f]{7,}(...)? temp '''), } + [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 == [ + (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-and-Forwardbot#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 diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index e4fdf8d3..10509707 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -815,21 +815,28 @@ class PullRequests(models.Model): }) elif command == 'review': if param and is_reviewer: + oldstate = self.state newstate = RPLUS.get(self.state) if newstate: self.state = newstate self.reviewed_by = author ok = True - if self.status == 'failure': - # the normal infrastructure is for failure and - # prefixes messages with "I'm sorry" - Feedback.create({ - 'repository': self.repository.id, - 'pull_request': self.number, - 'message': "You may want to rebuild or fix this PR as it has failed CI.", - }) else: msg = "This PR is already reviewed, reviewing it again is useless." + _logger.debug( + "r+ on %s by %s (%s->%s) status=%s message? %s", + self.display_name, author.github_login, + oldstate, newstate or oldstate, + self.status, self.status == 'failure' + ) + if self.status == 'failure': + # the normal infrastructure is for failure and + # prefixes messages with "I'm sorry" + Feedback.create({ + 'repository': self.repository.id, + 'pull_request': self.number, + 'message': "@{}, you may want to rebuild or fix this PR as it has failed CI.".format(author.github_login), + }) elif not param and is_author: newstate = RMINUS.get(self.state) if self.priority == 0 or newstate: diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index ebdab3e2..e59cccb1 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -3158,7 +3158,7 @@ class TestFeedback: (users['user'], "'ci/runbot' failed on this reviewed PR.") ] - def test_review_unvalidated(self, repo, env, users, config): + def test_review_failed(self, repo, env, users, config): """r+-ing a PR with failed CI sends feedback""" with repo: m = repo.make_commit(None, 'initial', None, tree={'m': 'm'}) @@ -3184,7 +3184,7 @@ class TestFeedback: assert prx.comments == [ (users['reviewer'], 'hansen r+'), - (users['user'], "You may want to rebuild or fix this PR as it has failed CI.") + (users['user'], "@%s, you may want to rebuild or fix this PR as it has failed CI." % users['reviewer']) ] class TestInfrastructure: def test_protection(self, repo):