From 670597eb0191f32be2ff9139bded937778baf4cc Mon Sep 17 00:00:00 2001 From: David Szervanszky Date: Mon, 30 Mar 2020 16:45:26 +0100 Subject: [PATCH] Optimistic batching for batch merging Fix for issue #164. Previously we added trailers to each MR and merged the constituent MRs of a batch MR to the target branch. Now we will now optimistically add the trailers to each MR and rebase on the batch branch. And when the batch branch passes CI we merge it which will automatically merge each constituent branch. --- marge/batch_job.py | 68 +++++++++++++++++------------------------ marge/git.py | 11 +++++-- marge/job.py | 2 ++ pylintrc | 1 + tests/git_repo_mock.py | 4 ++- tests/test_batch_job.py | 25 +-------------- 6 files changed, 44 insertions(+), 67 deletions(-) diff --git a/marge/batch_job.py b/marge/batch_job.py index b99fd875..c00d078c 100644 --- a/marge/batch_job.py +++ b/marge/batch_job.py @@ -112,7 +112,6 @@ def accept_mr( self, merge_request, expected_remote_target_branch_sha, - source_repo_url=None, ): log.info('Fusing MR !%s', merge_request.iid) approvals = merge_request.fetch_approvals() @@ -122,21 +121,6 @@ def accept_mr( if new_target_sha != expected_remote_target_branch_sha: raise CannotBatch('Someone was naughty and by-passed marge') - # FIXME: we should only add tested-by for the last MR in the batch - _, _, actual_sha = self.update_from_target_branch_and_push( - merge_request, - source_repo_url=source_repo_url, - ) - - sha_now = Commit.last_on_branch( - merge_request.source_project_id, merge_request.source_branch, self._api, - ).id - # Make sure no-one managed to race and push to the branch in the - # meantime, because we're about to impersonate the approvers, and - # we don't want to approve unreviewed commits - if sha_now != actual_sha: - raise CannotMerge('Someone pushed to branch while we were trying to merge') - # As we're not using the API to merge the MR, we don't strictly need to reapprove it. However, # it's a little weird to look at the merged MR to find it has no approvals, so let's do it anyway. self.maybe_reapprove(merge_request, approvals) @@ -198,6 +182,9 @@ def execute(self): merge_request.source_branch, '%s/%s' % (merge_request_remote, merge_request.source_branch), ) + # Apply the trailers before running the batch MR + self.add_trailers(merge_request) + self.push_force_to_mr(merge_request, True, source_repo_url, skip_ci=True) # Update on latest branch so it contains previous MRs self.fuse( @@ -207,6 +194,9 @@ def execute(self): local=True, ) + # Ensure that individual branch commit SHA matches matches that of its equivalent in batch MR + self.push_force_to_mr(merge_request, True, source_repo_url, skip_ci=True) + # Update branch with MR changes self._repo.fast_forward( BatchMergeJob.BATCH_BRANCH_NAME, @@ -225,36 +215,34 @@ def execute(self): working_merge_requests.append(merge_request) if len(working_merge_requests) <= 1: raise CannotBatch('not enough ready merge requests') - if self._project.only_allow_merge_if_pipeline_succeeds: - # This switches git to branch - self.push_batch() - batch_mr = self.create_batch_mr( - target_branch=target_branch, - ) + # This switches git to branch + self.push_batch() + batch_mr = self.create_batch_mr( + target_branch=target_branch, + ) + for merge_request in working_merge_requests: + merge_request.comment('I will attempt to batch this MR (!{})...'.format(batch_mr.iid)) + try: + self.wait_for_ci_to_pass(batch_mr) + except CannotMerge as err: for merge_request in working_merge_requests: - merge_request.comment('I will attempt to batch this MR (!{})...'.format(batch_mr.iid)) - try: - self.wait_for_ci_to_pass(batch_mr) - except CannotMerge as err: - for merge_request in working_merge_requests: - merge_request.comment( - 'Batch MR !{batch_mr_iid} failed: {error} I will retry later...'.format( - batch_mr_iid=batch_mr.iid, - error=err.reason, - ), - ) - raise CannotBatch(err.reason) from err + merge_request.comment( + 'Batch MR !{batch_mr_iid} failed: {error} I will retry later...'.format( + batch_mr_iid=batch_mr.iid, + error=err.reason, + ), + ) + raise CannotBatch(err.reason) from err for merge_request in working_merge_requests: try: # FIXME: this should probably be part of the merge request _, source_repo_url, merge_request_remote = self.fetch_source_project(merge_request) - self.ensure_mr_not_changed(merge_request) self.ensure_mergeable_mr(merge_request) - remote_target_branch_sha = self.accept_mr( - merge_request, - remote_target_branch_sha, - source_repo_url=source_repo_url, - ) + if merge_request == working_merge_requests[-1]: + self.accept_mr( + batch_mr, + remote_target_branch_sha, + ) except CannotBatch as err: merge_request.comment( "I couldn't merge this branch: {error} I will retry later...".format( diff --git a/marge/git.py b/marge/git.py index 3b424b5e..153c49ad 100644 --- a/marge/git.py +++ b/marge/git.py @@ -131,7 +131,7 @@ def remove_branch(self, branch, *, new_current_branch='master'): def checkout_branch(self, branch, start_point=''): self.git('checkout', '-B', branch, start_point, '--') - def push(self, branch, *, source_repo_url=None, force=False): + def push(self, branch, *, source_repo_url=None, force=False, skip_ci=False): self.git('checkout', branch, '--') self.git('diff-index', '--quiet', 'HEAD') # check it is not dirty @@ -146,7 +146,14 @@ def push(self, branch, *, source_repo_url=None, force=False): else: source = 'origin' force_flag = '--force' if force else '' - self.git('push', force_flag, source, '%s:%s' % (branch, branch)) + # The following needs to be split into 2 variables as any whitespace in the string + # causes it to be quoted which makes the string ignored by git + if skip_ci: + skip_1 = '-o' + skip_2 = 'ci.skip' + else: + skip_1, skip_2 = '', '' + self.git('push', force_flag, skip_1, skip_2, source, '%s:%s' % (branch, branch)) def get_commit_hash(self, rev='HEAD'): """Return commit hash for `rev` (default "HEAD").""" diff --git a/marge/job.py b/marge/job.py index 2abbce10..39eb8d00 100644 --- a/marge/job.py +++ b/marge/job.py @@ -336,12 +336,14 @@ def push_force_to_mr( merge_request, branch_was_modified, source_repo_url=None, + skip_ci=False, ): try: self._repo.push( merge_request.source_branch, source_repo_url=source_repo_url, force=True, + skip_ci=skip_ci ) except git.GitError: def fetch_remote_branch(): diff --git a/pylintrc b/pylintrc index de05e3d5..3a237775 100644 --- a/pylintrc +++ b/pylintrc @@ -30,6 +30,7 @@ max-line-length=110 [DESIGN] max-args=10 max-attributes=15 +max-locals=16 max-public-methods=35 [REPORTS] diff --git a/tests/git_repo_mock.py b/tests/git_repo_mock.py index 6aeca3f0..13787537 100644 --- a/tests/git_repo_mock.py +++ b/tests/git_repo_mock.py @@ -178,9 +178,11 @@ def merge(self, arg): self._local_repo.set_ref(self._branch, new_sha) def push(self, *args): - force_flag, remote_name, refspec = args + force_flag, skip_1, skip_2, remote_name, refspec = args assert force_flag in ('', '--force') + assert skip_1 in ('', '-o') + assert skip_2 in ('', 'ci.skip') branch, remote_branch = refspec.split(':') remote_url = self._remotes[remote_name] diff --git a/tests/test_batch_job.py b/tests/test_batch_job.py index 109b9056..99ba0fce 100644 --- a/tests/test_batch_job.py +++ b/tests/test_batch_job.py @@ -7,10 +7,9 @@ import marge.project import marge.user from marge.batch_job import BatchMergeJob, CannotBatch -from marge.gitlab import GET from marge.job import CannotMerge, MergeJobOptions from marge.merge_request import MergeRequest -from tests.gitlab_api_mock import MockLab, Ok, commit +from tests.gitlab_api_mock import MockLab class TestBatchJob: @@ -160,25 +159,3 @@ def test_fuse_mr_when_target_branch_was_moved(self, api, mocklab): with pytest.raises(CannotBatch) as exc_info: batch_merge_job.accept_mr(merge_request, 'abc') assert str(exc_info.value) == 'Someone was naughty and by-passed marge' - - def test_fuse_mr_when_source_branch_was_moved(self, api, mocklab): - batch_merge_job = self.get_batch_merge_job(api, mocklab) - merge_request = self._mock_merge_request( - source_project_id=mocklab.merge_request_info['source_project_id'], - target_branch='master', - source_branch=mocklab.merge_request_info['source_branch'], - ) - - api.add_transition( - GET( - '/projects/{project_iid}/repository/branches/useless_new_feature'.format( - project_iid=mocklab.merge_request_info['source_project_id'], - ), - ), - Ok({'commit': commit(commit_id='abc', status='running')}), - ) - - with pytest.raises(CannotMerge) as exc_info: - batch_merge_job.accept_mr(merge_request, mocklab.initial_master_sha) - - assert str(exc_info.value) == 'Someone pushed to branch while we were trying to merge'