From bf34e9aa95188f5944156e6a7f0f50d2672dea3b Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Fri, 24 Sep 2021 08:03:24 +0200 Subject: [PATCH] [FIX] runbot_merge: ensure PR description is correct on merge Because sometimes github updates are missed (usually because github never triggers it), it's possible for the mergebot's view of a PR description to be incorrect. In that case, the PR may get merged with the wrong merge message entirely, through no fault of the user. Since we already fetch the PR info when staging it, there's very little overhead to checking that the PR message we store is correct then, and update it if it's not. This means the forward-port's description should also be correct. While at it, clean the forward port PR's creation a bit: - there should always be a message since the title is required on PRs (only the body can be missing), therefore no need to check that - as we're adding a bunch of pseudo-headers, there always is a body, no need for the condition - inline the `pr_data` and `URL`: they were extracted for the support of draft PRs, since that's been removed it's now unnecessary Fixes #530 --- forwardport/models/project.py | 26 +++++++------------ runbot_merge/models/pull_requests.py | 5 ++++ runbot_merge/tests/test_oddities.py | 37 ++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 17 deletions(-) diff --git a/forwardport/models/project.py b/forwardport/models/project.py index 71edb676..40c52093 100644 --- a/forwardport/models/project.py +++ b/forwardport/models/project.py @@ -642,36 +642,28 @@ class PullRequests(models.Model): for pr in self: owner, _ = pr.repository.fp_remote_target.split('/', 1) source = pr.source_id or pr - message = source.message - if message: - message += '\n\n' - else: - message = '' root = pr._get_root() - message += '\n'.join( + + message = source.message + '\n\n' + '\n'.join( "Forward-Port-Of: %s" % p.display_name for p in root | source ) title, body = re.match(r'(?P[^\n]+)\n*(?P<body>.*)', message, flags=re.DOTALL).groups() - title = '[FW]' + title - if not body: - body = None - self.env.cr.execute('LOCK runbot_merge_pull_requests IN SHARE MODE') - url = 'https://api.github.com/repos/{}/pulls'.format(pr.repository.name) - pr_data = { - 'base': target.name, 'head': '%s:%s' % (owner, new_branch), - 'title': title, 'body': body - } - r = gh.post(url, json=pr_data) + r = gh.post(f'https://api.github.com/repos/{pr.repository.name}/pulls', json={ + 'base': target.name, + 'head': f'{owner}:{new_branch}', + 'title': '[FW]' + title, + 'body': body + }) if not r.ok: _logger.warning("Failed to create forward-port PR for %s, deleting branches", pr.display_name) # delete all the branches this should automatically close the # PRs if we've created any. Using the API here is probably # simpler than going through the working copies for repo in self.mapped('repository'): - d = gh.delete('https://api.github.com/repos/{}/git/refs/heads/{}'.format(repo.fp_remote_target, new_branch)) + d = gh.delete(f'https://api.github.com/repos/{repo.fp_remote_target}/git/refs/heads/{new_branch}') if d.ok: _logger.info("Deleting %s:%s=success", repo.fp_remote_target, new_branch) else: diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index a5e2a0e5..f765bee7 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1418,6 +1418,11 @@ 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('-', '_'))( diff --git a/runbot_merge/tests/test_oddities.py b/runbot_merge/tests/test_oddities.py index b2b2f984..5514563e 100644 --- a/runbot_merge/tests/test_oddities.py +++ b/runbot_merge/tests/test_oddities.py @@ -1,3 +1,6 @@ +from utils import Commit, to_pr + + def test_partner_merge(env): p_src = env['res.partner'].create({ 'name': 'kfhsf', @@ -60,3 +63,37 @@ 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"