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'