From 8f7a5e55efd5aae6a1f8c982d4d2f193ba92f871 Mon Sep 17 00:00:00 2001 From: Martin Trigaux Date: Thu, 13 Sep 2018 10:30:47 +0200 Subject: [PATCH] [FIX] runbot_merge: avoid double closes message If the message already contains the pr number, no need to add it again --- runbot_merge/models/pull_requests.py | 20 ++++-- runbot_merge/tests/test_basic.py | 91 +++++++++++++++++++++++++++- 2 files changed, 105 insertions(+), 6 deletions(-) diff --git a/runbot_merge/models/pull_requests.py b/runbot_merge/models/pull_requests.py index 760ba38c..3cfa6813 100644 --- a/runbot_merge/models/pull_requests.py +++ b/runbot_merge/models/pull_requests.py @@ -922,6 +922,15 @@ class Batch(models.Model): :return: () or Batch object (if all prs successfully staged) """ + + def build_message(message, pr): + m = re.search(r'( |{repository})#{pr.number}\b'.format( + pr=pr, repository=pr.repository.name.replace('/', '\\/')), + message) + if m: + return message + return message + '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=pr) + new_heads = {} for pr in prs: gh = meta[pr.repository]['gh'] @@ -932,17 +941,17 @@ class Batch(models.Model): ) target = 'tmp.{}'.format(pr.target.name) - suffix = '\n\ncloses {pr.repository.name}#{pr.number}'.format(pr=pr) try: # nb: pr_commits is oldest to newest so pr.head is pr_commits[-1] pr_commits = gh.commits(pr.number) rebase_and_merge = pr.rebase squash = rebase_and_merge and len(pr_commits) == 1 if squash: - pr_commits[0]['commit']['message'] += suffix + msg = build_message(pr_commits[0]['commit']['message'], pr) + pr_commits[0]['commit']['message'] = msg new_heads[pr] = gh.rebase(pr.number, target, commits=pr_commits) elif rebase_and_merge: - msg = pr.message + suffix + msg = build_message(pr.message, pr) h = gh.rebase(pr.number, target, reset=True, commits=pr_commits) new_heads[pr] = gh.merge(h, target, msg)['sha'] else: @@ -964,8 +973,9 @@ class Batch(models.Model): original_head = gh.head(target) merge_tree = gh.merge(pr_head['sha'], target, 'temp merge')['tree']['sha'] new_parents = [original_head] + list(head_parents - {base_commit}) + msg = build_message(pr_head['commit']['message'], pr) copy = gh('post', 'git/commits', json={ - 'message': pr_head['commit']['message'] + suffix, + 'message': msg, 'tree': merge_tree, 'author': pr_head['commit']['author'], 'committer': pr_head['commit']['committer'], @@ -975,7 +985,7 @@ class Batch(models.Model): new_heads[pr] = copy['sha'] else: # otherwise do a regular merge - msg = pr.message + suffix + msg = build_message(pr.message, pr) new_heads[pr] = gh.merge(pr.head, target, msg)['sha'] except (exceptions.MergeError, AssertionError) as e: _logger.exception("Failed to merge %s:%s into staging branch (error: %s)", pr.repository.name, pr.number, e) diff --git a/runbot_merge/tests/test_basic.py b/runbot_merge/tests/test_basic.py index b82677cb..ebd389fe 100644 --- a/runbot_merge/tests/test_basic.py +++ b/runbot_merge/tests/test_basic.py @@ -62,7 +62,96 @@ def test_trivial_flow(env, repo): 'a': b'some other content', 'b': b'a second file', } - assert master.message, "gibberish\n\nblahblah\n\ncloses odoo/odoo#1" + assert master.message == "gibberish\n\nblahblah\n\ncloses {repo.name}#1".format(repo=repo) + +class TestCommitMessage: + def test_commit_simple(self, env, repo, users): + """ verify 'closes ...' is correctly added in the commit message + """ + c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'}) + repo.make_ref('heads/master', c1) + c2 = repo.make_commit(c1, 'simple commit message', None, tree={'f': 'm2'}) + + prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') + repo.post_status(prx.head, 'success', 'ci/runbot') + repo.post_status(prx.head, 'success', 'legal/cla') + prx.post_comment('hansen r+', "reviewer") + + env['runbot_merge.project']._check_progress() + + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + env['runbot_merge.project']._check_progress() + + master = repo.commit('heads/master') + assert master.message == "simple commit message\n\ncloses {repo.name}#1".format(repo=repo) + + def test_commit_existing(self, env, repo, users): + """ verify do not duplicate 'closes' instruction + """ + c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'}) + repo.make_ref('heads/master', c1) + c2 = repo.make_commit(c1, 'simple commit message that closes #1', None, tree={'f': 'm2'}) + + prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') + repo.post_status(prx.head, 'success', 'ci/runbot') + repo.post_status(prx.head, 'success', 'legal/cla') + prx.post_comment('hansen r+', "reviewer") + + env['runbot_merge.project']._check_progress() + + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + env['runbot_merge.project']._check_progress() + + master = repo.commit('heads/master') + # closes #1 is already present, should not modify message + assert master.message == "simple commit message that closes #1" + + def test_commit_other(self, env, repo, users): + """ verify do not duplicate 'closes' instruction + """ + c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'}) + repo.make_ref('heads/master', c1) + c2 = repo.make_commit(c1, 'simple commit message that closes odoo/enterprise#1', None, tree={'f': 'm2'}) + + prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') + repo.post_status(prx.head, 'success', 'ci/runbot') + repo.post_status(prx.head, 'success', 'legal/cla') + prx.post_comment('hansen r+', "reviewer") + + env['runbot_merge.project']._check_progress() + + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + env['runbot_merge.project']._check_progress() + + master = repo.commit('heads/master') + # closes on another repositoy, should modify the commit message + assert master.message == "simple commit message that closes odoo/enterprise#1\n\ncloses {repo.name}#1".format(repo=repo) + + def test_commit_wrong_number(self, env, repo, users): + """ verify do not match on a wrong number + """ + c1 = repo.make_commit(None, 'first!', None, tree={'f': 'm1'}) + repo.make_ref('heads/master', c1) + c2 = repo.make_commit(c1, 'simple commit message that closes #11', None, tree={'f': 'm2'}) + + prx = repo.make_pr('title', 'body', target='master', ctid=c2, user='user') + repo.post_status(prx.head, 'success', 'ci/runbot') + repo.post_status(prx.head, 'success', 'legal/cla') + prx.post_comment('hansen r+', "reviewer") + + env['runbot_merge.project']._check_progress() + + repo.post_status('heads/staging.master', 'success', 'ci/runbot') + repo.post_status('heads/staging.master', 'success', 'legal/cla') + env['runbot_merge.project']._check_progress() + + master = repo.commit('heads/master') + # closes on another repositoy, should modify the commit message + assert master.message == "simple commit message that closes #11\n\ncloses {repo.name}#1".format(repo=repo) + class TestWebhookSecurity: def test_no_secret(self, env, project, repo):