From 98aaa9107ffa3de1844ff13238c530d8448ce484 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 3 Jun 2024 15:29:47 +0200 Subject: [PATCH] [CHG] forwardport: notify the outstanding forwardports rather than source I have been convinced that this might be an improvement to the affairs of the people: originally the message was sent to the source PR so we wouldn't have to ping the author & reviewer and to limit the amount of spam, *however*: - we ended up adding pings anyway - it also pings the followers of the source PR - it increases the size of the original discussion (especially if was - originally long) - it adds steps to fixing the issue as you need to bounce from the source to the forward ports Note that this might still notify a lot of people as they might be made followers of the forward ports automatically, and it increases the messaging load of the forwardbot significantly. But we'll see how things go. Worst case scenario, we can revert it back. Fixes #836 --- forwardport/models/project.py | 19 ++--- forwardport/tests/test_simple.py | 69 +++++++++++++++---- ..._merge.pull_requests.feedback.template.csv | 6 +- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index a4099d05..1839979d 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -553,18 +553,13 @@ stderr: if source.merge_date > (cutoff_dt - backoff): continue source.reminder_backoff_factor += 1 - self.env.ref('runbot_merge.forwardport.reminder')._send( - repository=source.repository, - pull_request=source.number, - token_field='fp_github_token', - format_args={ - 'pr': source, - 'outstanding': ''.join( - f'\n- {pr.display_name}' - for pr in sorted(prs, key=lambda p: p.number) - ), - } - ) + for pr in prs: + self.env.ref('runbot_merge.forwardport.reminder')._send( + repository=pr.repository, + pull_request=pr.number, + token_field='fp_github_token', + format_args={'pr': pr, 'source': source}, + ) class Stagings(models.Model): _inherit = 'runbot_merge.stagings' diff --git a/forwardport/tests/test_simple.py b/forwardport/tests/test_simple.py index c086bd9b..e7214871 100644 --- a/forwardport/tests/test_simple.py +++ b/forwardport/tests/test_simple.py @@ -6,7 +6,7 @@ from datetime import datetime, timedelta import pytest -from utils import seen, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all, part_of, to_pr +from utils import seen, Commit, make_basic, REF_PATTERN, MESSAGE_TEMPLATE, validate_all, part_of, to_pr, re_matches FMT = '%Y-%m-%d %H:%M:%S' FAKE_PREV_WEEK = (datetime.now() + timedelta(days=1)).strftime(FMT) @@ -132,10 +132,20 @@ def test_straightforward_flow(env, config, make_repo, users): (users['reviewer'], 'hansen r+ rebase-ff'), seen(env, pr, users), (users['user'], 'Merge method set to rebase and fast-forward.'), - (users['user'], '@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n- %s' % ( - users['other'], users['reviewer'], - '\n- '.join((pr1 | pr2).mapped('display_name')) - )), + ] + pr1_remote = prod.get_pr(pr1.number) + assert pr1_remote.comments == [ + seen(env, pr1_remote, users), + (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 +"""), + (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % ( + users['other'], + users['reviewer'], + pr0.display_name, + )) ] assert pr0_ == pr0 @@ -168,6 +178,11 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port users['other'], users['reviewer'], pr1.display_name, )), + (users['user'], "@%s @%s this forward port of %s is awaiting action (not merged or closed)." % ( + users['other'], + users['reviewer'], + pr0.display_name, + )) ] with prod: prod.post_status(pr2.head, 'success', 'ci/runbot') @@ -231,7 +246,7 @@ More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port assert other_user_repo.get_ref(pr.ref) == p_1 # should have deleted all PR branches - pr1_ref = prod.get_pr(pr1.number).ref + pr1_ref = pr1_remote.ref with pytest.raises(AssertionError, match='Not Found'): other.get_ref(pr1_ref) @@ -326,27 +341,57 @@ def test_empty(env, config, make_repo, users): awaiting = ( users['other'], - '@%s @%s this pull request has forward-port PRs awaiting action (not merged or closed):\n- %s' % ( + '@%s @%s this forward port of %s is awaiting action (not merged or closed).' % ( users['user'], users['reviewer'], - fail_id.display_name + pr1_id.display_name ) ) + conflict = (users['user'], re_matches( + fr"""@{users['user']} @{users['reviewer']} cherrypicking of pull request {pr1_id.display_name} failed\. + +stdout: +``` +.* +``` + +stderr: +``` +.* +``` + +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\. + +:warning: after resolving this conflict, you will need to merge it via @{project.github_prefix}\. + +More info at https://github\.com/odoo/odoo/wiki/Mergebot#forward-port +""", re.DOTALL)) assert pr1.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr1, users), + ] + fail_pr = prod.get_pr(fail_id.number) + assert fail_pr.comments == [ + seen(env, fail_pr, users), + conflict, awaiting, awaiting, - ], "each cron run should trigger a new message on the ancestor" + ], "each cron run should trigger a new message" # check that this stops if we close the PR with prod: - prod.get_pr(fail_id.number).close() + fail_pr.close() env.run_crons('forwardport.reminder', 'runbot_merge.feedback_cron', context={'forwardport_updated_before': FAKE_PREV_WEEK}) assert pr1.comments == [ (users['reviewer'], 'hansen r+'), seen(env, pr1, users), - awaiting, - awaiting, ] + assert fail_pr.comments == [ + seen(env, fail_pr, users), + conflict, + awaiting, + awaiting, + ], "each cron run should trigger a new message" def test_partially_empty(env, config, make_repo): """ Check what happens when only some commits of the PR are now empty diff --git a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv index 5fe768ae..eab2a241 100644 --- a/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv +++ b/runbot_merge/data/runbot_merge.pull_requests.feedback.template.csv @@ -131,10 +131,10 @@ runbot_merge.forwardport.failure.conflict,"{pr.ping}the next pull request ({new. pr: the pr which was just forward ported new: the new forward-port footer: some footer text" -runbot_merge.forwardport.reminder,{pr.ping}this pull request has forward-port PRs awaiting action (not merged or closed):{outstanding},"Comment when a forward port has outstanding (not merged or closed) descendants +runbot_merge.forwardport.reminder,{pr.ping}this forward port of {source.display_name} is awaiting action (not merged or closed).,"Comment when a forward port has outstanding (not merged or closed) descendants -pr: the original (source) PR -outstanding: markdown-formatted list of outstanding PRs" +pr: the forward-port +source: the source PR" runbot_merge.forwardport.failure,"{pr.ping}cherrypicking of pull request {pr.source_id.display_name} failed. {commits}{stdout}{stderr} Either perform the forward-port manually (and push to this branch, proceeding as usual) or close this PR (maybe?).