From 1cea247e6ce6b8add5bca06634655e43a53024dd Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 20 Jan 2023 15:16:37 +0100 Subject: [PATCH] [FIX] runbot_merge: sync PR target and message on `check` Previously the mergebot would only sync the head commit, but synching more is useful. Also update the final sanity check on staging: - as with check, update the message & target branch - reset PR state and post a message when updating message instead of doing so silently Note: maybe only fail the staging if the message is updated *and* relevant to staging (aka there's a merge method and it's not `rebase`)? Fixes #680 --- forwardport/models/project_freeze.py | 6 +- forwardport/tests/test_weird.py | 6 +- runbot_merge/controllers/__init__.py | 11 ++-- runbot_merge/models/pull_requests.py | 83 +++++++++++++++++++--------- runbot_merge/tests/test_basic.py | 52 ++++++++++++----- runbot_merge/tests/test_oddities.py | 34 ------------ 6 files changed, 111 insertions(+), 81 deletions(-) diff --git a/forwardport/models/project_freeze.py b/forwardport/models/project_freeze.py index c8aec42f..635912c0 100644 --- a/forwardport/models/project_freeze.py +++ b/forwardport/models/project_freeze.py @@ -15,8 +15,12 @@ class FreezeWizard(models.Model): self.env.ref('forwardport.port_forward').active = False return r + def action_freeze(self): + return super(FreezeWizard, self.with_context(forwardport_keep_disabled=True))\ + .action_freeze() + def unlink(self): r = super().unlink() - if not self.search_count([]): + if not (self.env.context.get('forwardport_keep_disabled') or self.search_count([])): self.env.ref('forwardport.port_forward').active = True return r diff --git a/forwardport/tests/test_weird.py b/forwardport/tests/test_weird.py index 7a7ded4d..8ce88f77 100644 --- a/forwardport/tests/test_weird.py +++ b/forwardport/tests/test_weird.py @@ -761,7 +761,10 @@ def test_retarget_after_freeze(env, config, make_repo, users): assert job # fuck up yo life: retarget the existing FP PR to the new branch - port_id.target = new_branch.id + port_pr = prod.get_pr(port_id.number) + with prod: + port_pr.base = 'bprime' + assert port_id.target == new_branch env.run_crons('forwardport.port_forward') assert not job.exists(), "job should have succeeded and apoptosed" @@ -771,7 +774,6 @@ def test_retarget_after_freeze(env, config, make_repo, users): assert env['runbot_merge.pull_requests'].search([('state', 'not in', ('merged', 'closed'))]) == port_id # merge the retargered PR - port_pr = prod.get_pr(port_id.number) with prod: prod.post_status(port_pr.head, 'success', 'ci/runbot') prod.post_status(port_pr.head, 'success', 'legal/cla') diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 8aa1342f..6840c79d 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -137,11 +137,12 @@ def handle_pr(env, event): if message != pr_obj.message: updates['message'] = message - _logger.info("update: %s#%d = %s (by %s)", repo.name, pr['number'], updates, event['sender']['login']) + _logger.info("update: %s = %s (by %s)", pr_obj.display_name, updates, event['sender']['login']) if updates: - pr_obj.write(updates) - return 'Updated {}'.format(pr_obj.id) - return "Nothing to update ({})".format(event['changes'].keys()) + # copy because it updates the `updates` dict internally + pr_obj.write(dict(updates)) + return 'Updated {}'.format(', '.join(updates)) + return "Nothing to update ({})".format(', '.join(event['changes'].keys())) message = None if not branch: @@ -209,7 +210,7 @@ def handle_pr(env, event): 'head': pr['head']['sha'], 'squash': pr['commits'] == 1, }) - return 'Updated {} to {}'.format(pr_obj.display_name, pr_obj.head) + return f'Updated to {pr_obj.head}' if event['action'] == 'ready_for_review': pr_obj.draft = False diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 844ca04a..23bbfa76 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -121,22 +121,31 @@ All substitutions are tentatively applied sequentially to the input. }) return - # if the PR is already loaded, check... if the heads match? + # if the PR is already loaded, force sync a few attributes pr_id = self.env['runbot_merge.pull_requests'].search([ ('repository.name', '=', pr['base']['repo']['full_name']), ('number', '=', number), ]) if pr_id: - # TODO: edited, maybe (requires crafting a 'changes' object) - r = controllers.handle_pr(self.env, { + sync = controllers.handle_pr(self.env, { 'action': 'synchronize', 'pull_request': pr, 'sender': {'login': self.project_id.github_prefix} }) + edit = controllers.handle_pr(self.env, { + 'action': 'edited', + 'pull_request': pr, + 'changes': { + 'base': {'ref': {'from': pr_id.target.name}}, + 'title': {'from': pr_id.message.splitlines()[0]}, + 'body': {'from', ''.join(pr_id.message.splitlines(keepends=True)[2:])}, + }, + 'sender': {'login': self.project_id.github_prefix}, + }) feedback({ 'repository': pr_id.repository.id, 'pull_request': number, - 'message': r, + 'message': f"{edit}. {sync}.", }) return @@ -1154,7 +1163,7 @@ class PullRequests(models.Model): old_target.display_name, pr.target.display_name, ) old_message = prev[pr.id]['message'] - if pr.merge_method in ('merge', 'rebase-merge') and pr.message != old_message: + if pr.merge_method not in (False, 'rebase-ff') and pr.message != old_message: pr.unstage("merge message updated") return w @@ -1301,9 +1310,32 @@ class PullRequests(models.Model): f"missing email on {c['sha']} indicates the authorship is " f"most likely incorrect." ) + + # sync and signal possibly missed updates + invalid = {} pr_head = pr_commits[-1]['sha'] - if pr_head != self.head: - raise exceptions.Mismatch(self.head, pr_head, commits == 1) + if self.head != pr_head: + invalid['head'] = pr_head + if self.squash != commits == 1: + invalid['squash'] = commits == 1 + + msg = f'{prdict["title"]}\n\n{prdict.get("body") or ""}'.strip() + if self.message != msg: + invalid['message'] = msg + + if self.target.name != prdict['base']['ref']: + branch = self.env['runbot_merge.branch'].with_context(active_test=False).search([ + ('name', '=', prdict['base']['ref']), + ('project_id', '=', self.repository.project_id.id), + ]) + if not branch: + self.unlink() + raise exceptions.Unmergeable(self, "While staging, found this PR had been retargeted to an un-managed branch.") + invalid['target'] = branch.id + + if invalid: + self.write({**invalid, 'state': 'opened'}) + raise exceptions.Mismatch(invalid) if self.reviewed_by and self.reviewed_by.name == self.reviewed_by.github_login: # XXX: find other trigger(s) to sync github name? @@ -1311,11 +1343,6 @@ class PullRequests(models.Model): if gh_name: self.reviewed_by.name = gh_name - # update pr message in case an update was missed - msg = f'{prdict["title"]}\n\n{prdict.get("body") or ""}'.strip() - if self.message != msg: - self.message = msg - # NOTE: lost merge v merge/copy distinction (head being # a merge commit reused instead of being re-merged) return method, getattr(self, '_stage_' + method.replace('-', '_'))( @@ -2123,6 +2150,7 @@ class Batch(models.Model): :return: () or Batch object (if all prs successfully staged) """ new_heads = {} + pr_fields = self.env['runbot_merge.pull_requests']._fields for pr in prs: gh = meta[pr.repository]['gh'] @@ -2154,24 +2182,27 @@ class Batch(models.Model): except github.MergeError: raise exceptions.MergeError(pr) except exceptions.Mismatch as e: - old_head, new_head, to_squash = e.args - pr.write({ - 'state': 'opened', - 'squash': to_squash, - 'head': new_head, - }) _logger.warning( - "head mismatch on %s: had %s but found %s", - pr.display_name, old_head, new_head + "data mismatch on %s: %s", + pr.display_name, e.args[0] ) self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': pr.repository.id, 'pull_request': pr.number, - 'message': "%swe apparently missed an update to this PR " - "and tried to stage it in a state which " - "might not have been approved. PR has been " - "updated to %s, please check and approve or " - "re-approve." % (pr.ping(), new_head) + 'message': """\ +%swe apparently missed updates to this PR and tried to stage it in a state +which might not have been approved. + +The properties %s were not correctly synchronized and have been updated. + +Note that we are unable to check the properties %s. + +Please check and re-approve. +""" % ( + pr.ping(), + ', '.join(pr_fields[f].string for f in e.args[0]), + ', '.join(pr_fields[f].string for f in UNCHECKABLE), + ) }) return self.env['runbot_merge.batch'] @@ -2183,6 +2214,8 @@ class Batch(models.Model): 'prs': [(4, pr.id, 0) for pr in prs], }) +UNCHECKABLE = ['merge_method', 'overrides', 'draft'] + class FetchJob(models.Model): _name = _description = 'runbot_merge.fetch_job' diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 259932c2..01f7dc4f 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2389,26 +2389,23 @@ class TestPRUpdate(object): assert pr.head == c2 assert pr.state == 'validated' - def test_update_missed(self, env, repo, config): + def test_update_missed(self, env, repo, config, users): """ Sometimes github's webhooks don't trigger properly, a branch's HEAD does not get updated and we might e.g. attempt to merge a PR despite it now being unreviewed or failing CI or somesuch. - This is not a super frequent occurrence, and possibly not the most - problematic issue ever (e.g. if the branch doesn't CI it's not going to - pass staging, though we might still be staging a branch which had been - unreviewed). + Therefore during the staging process we should check what we can, reject + the staging if cricical properties were found to mismatch, and notify + the pull request. - So during the staging process, the heads should be checked, and the PR - will not be staged if the heads don't match (though it'll be reset to - open, rather than put in an error state as technically there's no - failure, we just want to notify users that something went odd with the - mergebot). - - TODO: other cases / situations where we want to update the head? + The PR should then be reset to open (and transition to validated on its + own if the existing or new head has valid statuses), we don't want to + put it in an error state as technically there's no error, just something + which went a bit weird. """ with repo: - repo.make_commits(None, repo.Commit('m', tree={'a': '0'}), ref='heads/master') + [c] = repo.make_commits(None, repo.Commit('m', tree={'a': '0'}), ref='heads/master') + repo.make_ref('heads/somethingelse', c) [c] = repo.make_commits( 'heads/master', repo.Commit('c', tree={'a': '1'}), ref='heads/abranch') @@ -2433,11 +2430,18 @@ class TestPRUpdate(object): repo.post_status(c2, 'success', 'ci/runbot') repo.update_ref(pr.ref, c2, force=True) + other = env['runbot_merge.branch'].create({ + 'name': 'somethingelse', + 'project_id': env['runbot_merge.project'].search([]).id, + }) + # we missed the update notification so the db should still be at c and # in a "ready" state pr_id.write({ 'head': c, 'state': 'ready', + 'message': "Something else", + 'target': other.id, }) env.run_crons() @@ -2445,13 +2449,33 @@ class TestPRUpdate(object): # the PR should not get merged, and should be updated assert pr_id.state == 'validated' assert pr_id.head == c2 + assert pr_id.message == 'c' + assert pr_id.target.name == 'master' + assert pr.comments[-1] == (users['user'], """\ +@{} @{} we apparently missed updates to this PR and tried to stage it in a state +which might not have been approved. - pr_id.write({'head': c, 'state': 'ready'}) +The properties Head, Message, Target were not correctly synchronized and have been updated. + +Note that we are unable to check the properties Merge Method, Overrides, Draft. + +Please check and re-approve. +""".format(users['user'], users['reviewer'])) + + pr_id.write({ + 'head': c, + 'state': 'ready', + 'message': "Something else", + 'target': other.id, + }) with repo: pr.post_comment('hansen check') env.run_crons() assert pr_id.state == 'validated' assert pr_id.head == c2 + assert pr_id.message == 'c' # the commit's message was used for the PR + assert pr_id.target.name == 'master' + assert pr.comments[-1] == (users['user'], f"Updated target, squash, message. Updated to {c2}.") def test_update_closed(self, env, repo): with repo: diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index cdce2db0..f0a501ce 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -63,40 +63,6 @@ def test_name_search(env): assert PRs.name_search('repo') == [pr2, pr0, pr1] assert PRs.name_search('repo#1959') == [pr1] -def test_message_desync(env, project, make_repo, users, setreviewers, config): - """If the PR message gets desync'd (github misses sending an update), the - merge message should still match what's on github rather than what's in the - db - """ - repo = make_repo('repo') - env['runbot_merge.repository'].create({ - 'project_id': project.id, - 'name': repo.name, - 'status_ids': [(0, 0, {'context': 'status'})] - }) - setreviewers(*project.repo_ids) - - with repo: - [m] = repo.make_commits(None, Commit('root', tree={'a': '1'}), ref='heads/master') - - [c] = repo.make_commits('master', Commit('whee', tree={'b': '2'})) - pr = repo.make_pr(title='title', body='body', target='master', head=c) - repo.post_status(c, 'success', 'status') - env.run_crons() - - pr_id = to_pr(env, pr) - assert pr_id.message == 'title\n\nbody' - pr_id.message = "xxx" - - with repo: - pr.post_comment('hansen merge r+', config['role_reviewer']['token']) - env.run_crons() - - st = repo.commit('staging.master') - assert st.message.startswith('title\n\nbody'),\ - "the stored PR message should have been ignored when staging" - assert st.parents == [m, c], "check the staging's ancestry is the right one" - def test_unreviewer(env, project, port): repo = env['runbot_merge.repository'].create({ 'project_id': project.id,