From 728524db12c8e9a49850081fb0137d39bac8c91a Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 13 Jun 2024 13:30:07 +0200 Subject: [PATCH] [IMP] runbot_merge: send merge method warning faster, and on review - Instead of warning about the merge method on ready PRs, also warn on *approved* (but exclude staged just cuz), as that's really when the user wants to know that they forgot to set the merge method - The cron only triggers hourly, but *if* a user approves a PR *and* the merge method is not set yet, chances are good they'll need a reminder (if they `r+ rebase-merge` or w/e the cron will just ignore the PR and it's no skin off our back), so `_trigger` the cron for validation. - Also do the same when skipchecks is set as it's very similar. In reality we might want to hook off of the state transitioning to reviewed but I'm not sure there's good ways to do that (triggering a cron inside a compute doesn't seem like a good idea). Update a pair of tests which would approve a multi-commit PR without setting a merge method, just because the helper they use to build the PR happens to create multiple commits. Fix #891 --- runbot_merge/models/pull_requests.py | 19 ++++++++++++------- runbot_merge/tests/test_basic.py | 10 ++++++---- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 8a1bd658..e4676bdb 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -777,6 +777,9 @@ class PullRequests(models.Model): case commands.SkipChecks() if is_admin: self.batch_id.skipchecks = True self.reviewed_by = author + if not (self.squash or self.merge_method): + self.env.ref('runbot_merge.check_linked_prs_status')._trigger() + for p in self.batch_id.prs - self: if not p.reviewed_by: p.reviewed_by = author @@ -999,13 +1002,12 @@ class PullRequests(models.Model): def _approve(self, author, login): oldstate = self.state newstate = RPLUS.get(self.state) - msg = None if not author.email: - msg = "I must know your email before you can review PRs. Please contact an administrator." + return "I must know your email before you can review PRs. Please contact an administrator." elif not newstate: - msg = "this PR is already reviewed, reviewing it again is useless." - else: - self.reviewed_by = author + return "this PR is already reviewed, reviewing it again is useless." + + self.reviewed_by = author _logger.debug( "r+ on %s by %s (%s->%s) status=%s message? %s", self.display_name, author.github_login, @@ -1020,7 +1022,9 @@ class PullRequests(models.Model): pull_request=self.number, format_args={'user': login, 'pr': self}, ) - return msg + if not (self.squash or self.merge_method): + self.env.ref('runbot_merge.check_linked_prs_status')._trigger() + return None def message_post(self, **kw): if author := self.env.cr.precommit.data.get('change-author'): @@ -1362,7 +1366,8 @@ class PullRequests(models.Model): if pair[0] != 'squash' ) for r in self.search([ - ('state', '=', 'ready'), + ('state', 'in', ("approved", "ready")), + ('staging_id', '=', False), ('squash', '=', False), ('merge_method', '=', False), ('method_warned', '=', False), diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 6dec9506..40b26eda 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -1050,7 +1050,7 @@ def test_ci_failure_after_review(env, repo, users, config): """ with repo: prx = _simple_init(repo) - prx.post_comment('hansen r+', config['role_reviewer']['token']) + prx.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token']) env.run_crons() for ctx, url in [ @@ -1066,8 +1066,9 @@ def test_ci_failure_after_review(env, repo, users, config): env.run_crons() assert prx.comments == [ - (users['reviewer'], 'hansen r+'), + (users['reviewer'], 'hansen r+ rebase-ff'), seen(env, prx, users), + (users['user'], "Merge method set to rebase and fast-forward."), (users['user'], "@{user} @{reviewer} 'ci/runbot' failed on this reviewed PR.".format_map(users)), (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)), (users['user'], "@{user} @{reviewer} 'legal/cla' failed on this reviewed PR.".format_map(users)), @@ -1302,14 +1303,15 @@ class TestRetry: """ with repo: prx = _simple_init(repo) - prx.post_comment('hansen r+', config['role_reviewer']['token']) + prx.post_comment('hansen r+ rebase-ff', config['role_reviewer']['token']) prx.post_comment('hansen retry', config['role_reviewer']['token']) env.run_crons() assert prx.comments == [ - (users['reviewer'], 'hansen r+'), + (users['reviewer'], 'hansen r+ rebase-ff'), (users['reviewer'], 'hansen retry'), seen(env, prx, users), + (users['user'], "Merge method set to rebase and fast-forward."), (users['user'], "@{reviewer} retry makes no sense when the PR is not in error.".format_map(users)), ]