From 2746a131631bfb2b36e278176345be939f4418f5 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Wed, 7 Dec 2022 13:25:08 +0100 Subject: [PATCH] [FIX] runbot_merge: multi-commit squashing If commits have different authors (/ committers), the mergebot would ask github to create a commit with an author (/ committer) of `None` / `null`. Apparently github really does not like that, and complains that > nil is not an object So remove the key entirely. Also fix the collision between `author` and the `Co-Authored-By` list, which could lead to trying to set an `author` of `[name, email]` instead of an object, which is also not accepted by github. --- runbot_merge/models/pull_requests.py | 18 +++++++----- runbot_merge/tests/test_basic.py | 44 ++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 7f7c76e2..cd811641 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -1323,35 +1323,37 @@ class PullRequests(models.Model): def _stage_squash(self, gh, target, commits, related_prs=()): msg = self._build_merge_message(self, related_prs=related_prs) + authorship = {} + authors = { (c['commit']['author']['name'], c['commit']['author']['email']) for c in commits } - author = None if len(authors) == 1: name, email = authors.pop() - author = {'name': name, 'email': email} - for author in authors: - msg.headers['Co-Authored-By'] = "%s <%s>" % author + authorship['author'] = {'name': name, 'email': email} + else: + msg.headers.extend(sorted( + ('Co-Authored-By', "%s <%s>" % author) + for author in authors + )) committers = { (c['commit']['committer']['name'], c['commit']['committer']['email']) for c in commits } - committer = None if len(committers) == 1: name, email = committers.pop() - committer = {'name': name, 'email': email} + authorship['committer'] = {'name': name, 'email': email} # should committers also be added to co-authors? original_head = gh.head(target) merge_tree = gh.merge(self.head, target, 'temp merge')['tree']['sha'] head = gh('post', 'git/commits', json={ + **authorship, 'message': str(msg), 'tree': merge_tree, 'parents': [original_head], - 'author': author, - 'committer': committer, }).json()['sha'] gh.set_ref(target, head) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index a4cc3bce..259932c2 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -6,6 +6,7 @@ import time from unittest import mock import pytest +import requests from lxml import html, etree import odoo @@ -2006,19 +2007,36 @@ Part-of: {pr_id.display_name}""" assert log_to_node(repo.log('heads/master')), expected def test_squash_merge(self, repo, env, config, users): + other_user = requests.get(f'https://api.github.com/user', headers={ + 'Authorization': 'token %s' % config['role_other']['token'], + }).json() + other_user = { + 'name': other_user['name'] or other_user['login'], + # FIXME: not guaranteed + 'email': other_user['email'] or 'other@example.org', + } + a_user = {'name': 'bob', 'email': 'builder@example.org', 'date': '1999-04-12T08:19:30Z'} with repo: repo.make_commits(None, Commit('initial', tree={'a': '0'}), ref='heads/master') repo.make_commits( 'master', - Commit('sub', tree={'b': '0'}, committer={'name': 'bob', 'email': 'builder@example.org', 'date': '1999-04-12T08:19:30Z'}), + Commit('sub', tree={'b': '0'}, committer=a_user), ref='heads/other' ) pr1 = repo.make_pr(title='first pr', target='master', head='other') repo.post_status('other', 'success', 'legal/cla') repo.post_status('other', 'success', 'ci/runbot') - repo.make_commits('master', Commit('x', tree={'x': '0'}), Commit('y', tree={'x': '1'}), ref='heads/other2') + pr_2_commits = repo.make_commits( + 'master', + Commit('x', tree={'x': '0'}, author=other_user, committer=a_user), + Commit('y', tree={'x': '1'}, author=a_user, committer=other_user), + ref='heads/other2', + ) + c1, c2 = map(repo.commit, pr_2_commits) + assert c1.author['name'] != c2.author['name'] + assert c1.committer['name'] != c2.committer['name'] pr2 = repo.make_pr(title='second pr', target='master', head='other2') repo.post_status('other2', 'success', 'legal/cla') repo.post_status('other2', 'success', 'ci/runbot') @@ -2060,8 +2078,8 @@ closes {pr1_id.display_name} Signed-off-by: {get_partner(env, users["reviewer"]).formatted_email}\ """ - assert one['commit']['committer']['name'] == 'bob' - assert one['commit']['committer']['email'] == 'builder@example.org' + assert one['commit']['committer']['name'] == a_user['name'] + assert one['commit']['committer']['email'] == a_user['email'] commit_date = datetime.datetime.strptime(one['commit']['committer']['date'], '%Y-%m-%dT%H:%M:%SZ') # using timestamp (and working in seconds) because `pytest.approx` # silently fails on datetimes (#8395) @@ -2069,11 +2087,27 @@ Signed-off-by: {get_partner(env, users["reviewer"]).formatted_email}\ "the commit date of the merged commit should be about now, despite" \ " the source commit being >20 years old" + # FIXME: should probably get the token from the project to be sure it's + # the bot user + current_user = repo._session.get(f'https://api.github.com/user').json() + current_user = { + 'name': current_user['name'] or current_user['login'], + # FIXME: not guaranteed + 'email': current_user['email'] or 'user@example.org', + } + # since there are two authors & two committers on pr2, the auhor and + # committer of a squash commit should be reset to the bot's identity + assert two['commit']['committer']['name'] == current_user['name'] + assert two['commit']['committer']['email'] == current_user['email'] + assert two['commit']['author']['name'] == current_user['name'] + assert two['commit']['author']['email'] == current_user['email'] assert two['commit']['message'] == f"""second pr closes {pr2_id.display_name} -Signed-off-by: {get_partner(env, users["reviewer"]).formatted_email}\ +Signed-off-by: {get_partner(env, users["reviewer"]).formatted_email} +Co-authored-by: {a_user['name']} <{a_user['email']}> +Co-authored-by: {other_user['name']} <{other_user['email']}>\ """ assert repo.read_tree(repo.commit(two['sha'])) == { 'a': '0',