From 48c8e53df272fd92e8da16934002adf272a3ae2e Mon Sep 17 00:00:00 2001 From: Martin Trigaux Date: Tue, 10 Sep 2019 16:29:02 +0200 Subject: [PATCH] [FIX] forwardport: clarify forward port message The ping message was not clear - don't know if anything is needed from the author - don't know if the last step in the chain Ping the authors only when something needs to be done (error or last step). Do not ping non-reviewers as they will not have the rights to r+ or modify the PR anyway Closes #192 --- forwardport/models/project.py | 60 +++++++++++++++++++++----------- forwardport/tests/test_simple.py | 43 ++++++++++++++--------- 2 files changed, 67 insertions(+), 36 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index f6fb2158..68ad2028 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -387,24 +387,6 @@ class PullRequests(models.Model): message += "Forward-Port-Of: %s#%s" % (source.repository.name, source.number) (h, out, err) = conflicts.get(pr) or (None, None, None) - if h: - message = """Cherrypicking %s of source #%d failed with the following - -stdout: -``` -%s -``` - -stderr: -``` -%s -``` - -Either perform the forward-port manually (and push to this branch, proceeding -as usual) or close this PR (maybe?). - -In the former case, you may want to edit this PR message as well. -""" % (h, source.number, out, err) r = requests.post( 'https://api.github.com/repos/{}/pulls'.format(pr.repository.name), json={ @@ -433,11 +415,49 @@ In the former case, you may want to edit this PR message as well. # only link to previous PR of sequence if cherrypick passed 'parent_id': pr.id if not has_conflicts else False, }) - assignees = (new_pr.source_id.author | new_pr.source_id.reviewed_by).mapped('github_login') + + assignees = (new_pr.source_id.author | new_pr.source_id.reviewed_by) \ + .filtered(lambda p: new_pr.source_id._pr_acl(p).is_reviewer) \ + .mapped('github_login') + ping = "Ping %s" % ', '.join('@' + login for login in assignees if login) + if h: + sout = serr = '' + if out.strip(): + sout = "\nstdout:\n```\n%s\n```\n" % out + if err.strip(): + serr = "\nstderr:\n```\n%s\n```\n" % err + + message = ping + """ +Cherrypicking %s of source #%d failed +%s%s +Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?). + +In the former case, you may want to edit this PR message as well. +""" % (h, source.number, sout, serr) + elif base.limit_id == target: + ancestors = "".join( + "* %s#%d\n" % (p.repository.name, p.number) + for p in pr._iter_ancestors() + if p.parent_id and p.parent_id != source + ) + message = ping + """ +This PR targets %s and is the last of the forward-port chain%s +%s +To merge the full chain, say +> @%s r+ + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +""" % (target.name, 'containing:' if ancestors else '.', ancestors, pr.repository.project_id.fp_github_name) + else: + message = """\ +This PR targets %s and is part of the forward-port chain. Further PRs will be created up to %s. + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +""" % (target.name, base.limit_id.name) self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': new_pr.repository.id, 'pull_request': new_pr.number, - 'message': "Ping %s" % ', '.join('@' + login for login in assignees if login), + 'message': message, }) # 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 331e5bf0..e815fb85 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -182,11 +182,15 @@ def test_straightforward_flow(env, config, make_repo, users): 'x': '1' } assert prod.get_pr(pr2.number).comments == [ - (users['user'], "Ping @%s, @%s" % (users['other'], users['reviewer'])), - (users['user'], "This forward port PR is awaiting action"), - ] - assert prod.get_pr(pr2.number).comments == [ - (users['user'], "Ping @%s, @%s" % (users['other'], users['reviewer'])), + (users['user'], """\ +Ping @%s +This PR targets c and is the last of the forward-port chain. + +To merge the full chain, say +> @%s r+ + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +""" % (users['reviewer'], project.fp_github_name)), (users['user'], "This forward port PR is awaiting action"), ] with prod: @@ -591,10 +595,12 @@ def test_empty(env, config, make_repo, users): env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron') env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron') failed_pr = prod.get_pr(fail_id.number) + # stop there (criminal scum), checking the entire message is a PITA and + # I'd need to look into integrating better diffing / error reporting when + # an re_matches doesn't match + ping_note = re_matches(r"Ping @%s\nCherrypicking [0-9a-z]{40} of source #%d failed" % (users['reviewer'], pr1.number)) assert failed_pr.comments == [ - (users['user'], 'Ping @{}, @{}'.format( - users['user'], users['reviewer'], - )), + (users['user'], ping_note), (users['user'], "This forward port PR is awaiting action"), (users['user'], "This forward port PR is awaiting action"), ] @@ -602,13 +608,12 @@ def test_empty(env, config, make_repo, users): with prod: failed_pr.close() env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron') + # FIXME: carve out git messages as they're not necessarily stable / parseable assert failed_pr.comments == [ - (users['user'], 'Ping @{}, @{}'.format( - users['user'], users['reviewer'], - )), + (users['user'], ping_note), (users['user'], "This forward port PR is awaiting action"), (users['user'], "This forward port PR is awaiting action"), - ], "should not have a third note" + ], "should not have a 4th note" def test_partially_empty(env, config, make_repo): """ Check what happens when only some commits of the PR are now empty @@ -925,7 +930,11 @@ def test_batched(env, config, make_repo, users): ]) # check that relevant users were pinged ping = [ - (users['user'], "Ping @{}, @{}".format(friendo['user'], users['reviewer'])), + (users['user'], """\ +This PR targets b and is part of the forward-port chain. Further PRs will be created up to c. + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +"""), ] pr_remote_1b = main1.get_pr(pr1b.number) pr_remote_2b = main2.get_pr(pr2b.number) @@ -1021,9 +1030,11 @@ class TestClosing: assert env['runbot_merge.pull_requests'].search([], order='number') == pr0 | pr1,\ "closing the PR should suppress the FP sequence" assert prod.get_pr(pr1.number).comments == [ - (users['user'], 'Ping @{}, @{}'.format( - users['user'], users['reviewer'], - )) + (users['user'], """\ +This PR targets b and is part of the forward-port chain. Further PRs will be created up to c. + +More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port +""") ] def test_closing_after_fp(self, env, config, make_repo):