From 23e1b93465bb6f0f54a35a318ea3ef9ffb71df37 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Tue, 14 Feb 2023 13:38:37 +0100 Subject: [PATCH] [FIX] runbot_merge: a few issues with updated staging check 1cea247e6ce6b8add5bca06634655e43a53024dd tried to improve staging checks to avoid staging PRs in the wrong state, however it had two issues: PR state -------- The process would reset the PR's state to open, but unless the head was being resync'd it wouldn't re-apply the statuses on the state, leading to a PR with all-valid statuses, but a missing CI. Message ------- The message check didn't compose the PR message the same way PR creation / update did (it did not trim the title and description individually, only after concatenation), resulting in a not-actually-existing divergence getting signaled in the case where the PR title ends or the description starts with whitespace. Expand relevant test, add a utility function to compose a PR message and use it everywhere for coherence. Also update the logging and reporting to show a diff of all the updated items (hidden behind a `details` element). --- conftest.py | 2 + runbot_merge/controllers/__init__.py | 5 +- runbot_merge/models/pull_requests.py | 71 +++++++++++++++++++--------- runbot_merge/tests/test_basic.py | 67 +++++++++++++++++++++++--- runbot_merge/utils.py | 5 ++ 5 files changed, 116 insertions(+), 34 deletions(-) diff --git a/conftest.py b/conftest.py index 72e5ba71..b447bff3 100644 --- a/conftest.py +++ b/conftest.py @@ -834,6 +834,8 @@ class Comment(tuple): self._c = c return self def __getitem__(self, item): + if isinstance(item, int): + return super().__getitem__(item) return self._c[item] diff --git a/runbot_merge/controllers/__init__.py b/runbot_merge/controllers/__init__.py index 6840c79d..87b84606 100644 --- a/runbot_merge/controllers/__init__.py +++ b/runbot_merge/controllers/__init__.py @@ -130,10 +130,7 @@ def handle_pr(env, event): # turns out github doesn't bother sending a change key if the body is # changing from empty (None), therefore ignore that entirely, just # generate the message and check if it changed - message = pr['title'].strip() - body = (pr['body'] or '').strip() - if body: - message += f"\n\n{body}" + message = utils.make_message(pr) if message != pr_obj.message: updates['message'] = message diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 23bbfa76..549d673a 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -14,6 +14,7 @@ import pprint import re import time +from difflib import Differ from itertools import takewhile import requests @@ -1120,10 +1121,6 @@ class PullRequests(models.Model): ('github_login', '=', description['user']['login']), ], limit=1) - message = description['title'].strip() - body = description['body'] and description['body'].strip() - if body: - message += '\n\n' + body return self.env['runbot_merge.pull_requests'].create({ 'state': 'opened' if description['state'] == 'open' else 'closed', 'number': description['number'], @@ -1133,7 +1130,7 @@ class PullRequests(models.Model): 'repository': repo.id, 'head': description['head']['sha'], 'squash': description['commits'] == 1, - 'message': message, + 'message': utils.make_message(description), 'draft': description['draft'], }) @@ -1313,15 +1310,11 @@ class PullRequests(models.Model): # sync and signal possibly missed updates invalid = {} + diff = [] pr_head = pr_commits[-1]['sha'] 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 + diff.append(('Head', self.head, pr_head)) if self.target.name != prdict['base']['ref']: branch = self.env['runbot_merge.branch'].with_context(active_test=False).search([ @@ -1332,10 +1325,20 @@ class PullRequests(models.Model): self.unlink() raise exceptions.Unmergeable(self, "While staging, found this PR had been retargeted to an un-managed branch.") invalid['target'] = branch.id + diff.append(('Target branch', self.target.name, branch.name)) + + if self.squash != commits == 1: + invalid['squash'] = commits == 1 + diff.append(('Single commit', self.squash, commits == 1)) + + msg = utils.make_message(prdict) + if self.message != msg: + invalid['message'] = msg + diff.append(('Message', self.message, msg)) if invalid: - self.write({**invalid, 'state': 'opened'}) - raise exceptions.Mismatch(invalid) + self.write({**invalid, 'state': 'opened', 'head': pr_head}) + raise exceptions.Mismatch(invalid, diff) if self.reviewed_by and self.reviewed_by.name == self.reviewed_by.github_login: # XXX: find other trigger(s) to sync github name? @@ -2182,27 +2185,49 @@ class Batch(models.Model): except github.MergeError: raise exceptions.MergeError(pr) except exceptions.Mismatch as e: + def format_items(items): + """ Bit of a pain in the ass because difflib really wants + all lines to be newline-terminated, but not all values are + actual lines, and also needs to split multiline values. + """ + for name, value in items: + yield name + ':\n' + if not value.endswith('\n'): + value += '\n' + yield from value.splitlines(keepends=True) + yield '\n' + + old = list(format_items((n, v) for n, v, _ in e.args[1])) + new = list(format_items((n, v) for n, _, v in e.args[1])) + diff = ''.join(Differ().compare(old, new)) _logger.warning( - "data mismatch on %s: %s", - pr.display_name, e.args[0] + "data mismatch on %s:\n%s", + pr.display_name, diff ) self.env['runbot_merge.pull_requests.feedback'].create({ 'repository': pr.repository.id, 'pull_request': pr.number, 'message': """\ -%swe apparently missed updates to this PR and tried to stage it in a state +{ping}we 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. +The properties {mismatch} were not correctly synchronized and have been updated. -Note that we are unable to check the properties %s. +
differences + +```diff +{diff}``` +
+ +Note that we are unable to check the properties {unchecked}. 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), - ) +""".format( + ping=pr.ping(), + mismatch=', '.join(pr_fields[f].string for f in e.args[0]), + diff=diff, + unchecked=', '.join(pr_fields[f].string for f in UNCHECKABLE), +) }) return self.env['runbot_merge.batch'] diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index 01f7dc4f..ea5dd098 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -2408,7 +2408,7 @@ class TestPRUpdate(object): repo.make_ref('heads/somethingelse', c) [c] = repo.make_commits( - 'heads/master', repo.Commit('c', tree={'a': '1'}), ref='heads/abranch') + 'heads/master', repo.Commit('title \n\nbody', tree={'a': '1'}), ref='heads/abranch') pr = repo.make_pr(target='master', head='abranch') repo.post_status(pr.head, 'success', 'legal/cla') repo.post_status(pr.head, 'success', 'ci/runbot') @@ -2418,6 +2418,7 @@ class TestPRUpdate(object): ('number', '=', pr.number), ]) env.run_crons('runbot_merge.process_updated_commits') + assert pr_id.message == 'title\n\nbody' assert pr_id.state == 'ready' # TODO: find way to somehow skip / ignore the update_ref? @@ -2449,18 +2450,70 @@ 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.message == 'title\n\nbody' 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 + assert pr.comments[-1]['body'] == """\ +@{} @{} we apparently missed updates to this PR and tried to stage it in a state \ which might not have been approved. -The properties Head, Message, Target were not correctly synchronized and have been updated. +The properties Head, Target, Message were not correctly synchronized and have been updated. + +
differences + +```diff + Head: +- {} ++ {} + + Target branch: +- somethingelse ++ master + + Message: +- Something else ++ title + ++ body ++ +``` +
Note that we are unable to check the properties Merge Method, Overrides, Draft. Please check and re-approve. -""".format(users['user'], users['reviewer'])) +""".format(users['user'], users['reviewer'], c, c2) + + # if the head commit doesn't change, that part should still be valid + with repo: + pr.post_comment('hansen r+', config['role_reviewer']['token']) + assert pr_id.state == 'ready' + pr_id.write({'message': 'wrong'}) + env.run_crons() + + assert pr_id.message == 'title\n\nbody' + assert pr_id.state == 'validated' + assert pr.comments[-1]['body'] == """\ +@{} @{} we apparently missed updates to this PR and tried to stage it in a state \ +which might not have been approved. + +The properties Message were not correctly synchronized and have been updated. + +
differences + +```diff + Message: +- wrong ++ title + ++ body ++ +``` +
+ +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, @@ -2473,7 +2526,7 @@ Please check and re-approve. 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.message == 'title\n\nbody' # 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}.") diff --git a/runbot_merge/utils.py b/runbot_merge/utils.py index 6ef1c10d..bd35d3db 100644 --- a/runbot_merge/utils.py +++ b/runbot_merge/utils.py @@ -30,3 +30,8 @@ def backoff(func=None, *, delays=BACKOFF_DELAYS, exc=Exception): if delay is None: raise time.sleep(delay) + +def make_message(pr_dict): + title = pr_dict['title'].strip() + body = (pr_dict.get('body') or '').strip() + return f'{title}\n\n{body}' if body else title