diff --git a/.gitignore b/.gitignore index 6b92becc..b7e49fd4 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,6 @@ ENV/ # nix stuff result result-* + +# Editor +*.sw[po] diff --git a/marge/app.py b/marge/app.py index e099b336..e6b66db6 100644 --- a/marge/app.py +++ b/marge/app.py @@ -213,6 +213,17 @@ def regexp(str_regex): action='store_true', help='Disable fast forwarding when merging MR batches' ) + parser.add_argument( + '--use-merge-commit-batches', + action='store_true', + help='Use merge commit when creating batches, so that the commits in the batch MR ' + 'will be the same with in individual MRs. Requires sudo scope in the access token.\n', + ) + parser.add_argument( + '--skip-ci-batches', + action='store_true', + help='Skip CI when updating individual MRs when using batches' + ) config = parser.parse_args(args) if config.use_merge_strategy and config.batch: @@ -312,6 +323,8 @@ def main(args=None): ci_timeout=options.ci_timeout, fusion=fusion, use_no_ff_batches=options.use_no_ff_batches, + use_merge_commit_batches=options.use_merge_commit_batches, + skip_ci_batches=options.skip_ci_batches, ), batch=options.batch, ) diff --git a/marge/approvals.py b/marge/approvals.py index 16206a90..bf23ffdc 100644 --- a/marge/approvals.py +++ b/marge/approvals.py @@ -51,11 +51,15 @@ def reapprove(self): (which may invalidate approvals, depending on GitLab settings) and then restore the approval status. """ + self.approve(self) + + def approve(self, obj): + """Approve an object which can be a merge_request or an approval.""" if self._api.version().release >= (9, 2, 2): - approve_url = '/projects/{0.project_id}/merge_requests/{0.iid}/approve'.format(self) + approve_url = '/projects/{0.project_id}/merge_requests/{0.iid}/approve'.format(obj) else: # GitLab botched the v4 api before 9.2.3 - approve_url = '/projects/{0.project_id}/merge_requests/{0.id}/approve'.format(self) + approve_url = '/projects/{0.project_id}/merge_requests/{0.id}/approve'.format(obj) for uid in self.approver_ids: self._api.call(POST(approve_url), sudo=uid) diff --git a/marge/batch_job.py b/marge/batch_job.py index 2f4cae82..6b8e8268 100644 --- a/marge/batch_job.py +++ b/marge/batch_job.py @@ -1,8 +1,9 @@ -# pylint: disable=too-many-branches,too-many-statements +# pylint: disable=too-many-branches,too-many-statements,arguments-differ import logging as log from time import sleep from . import git +from . import gitlab from .commit import Commit from .job import MergeJob, CannotMerge, SkipMerge from .merge_request import MergeRequest @@ -46,6 +47,7 @@ def close_batch_mr(self): batch_mr.close() def create_batch_mr(self, target_branch): + self.push_batch() log.info('Creating batch MR') params = { 'source_branch': BatchMergeJob.BATCH_BRANCH_NAME, @@ -68,10 +70,10 @@ def get_mrs_with_common_target_branch(self, target_branch): if merge_request.target_branch == target_branch ] - def ensure_mergeable_mr(self, merge_request): + def ensure_mergeable_mr(self, merge_request, skip_ci=False): super().ensure_mergeable_mr(merge_request) - if self._project.only_allow_merge_if_pipeline_succeeds: + if self._project.only_allow_merge_if_pipeline_succeeds and not skip_ci: ci_status = self.get_mr_ci_status(merge_request) if ci_status != 'success': raise CannotBatch('This MR has not passed CI.') @@ -121,38 +123,54 @@ def merge_batch(self, target_branch, source_branch, no_ff=False): source_branch, ) - def accept_mr( + def update_merge_request( 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() - # Make sure latest commit in remote is the one we tested against - new_target_sha = Commit.last_on_branch(self._project.id, merge_request.target_branch, self._api).id - 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, + skip_ci=self._options.skip_ci_batches, ) sha_now = Commit.last_on_branch( merge_request.source_project_id, merge_request.source_branch, self._api, ).id + log.info('update_merge_request: sha_now (%s), actual_sha (%s)', sha_now, actual_sha) # 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. + # As we're not using the API to merge the individual 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) + return sha_now + + def accept_mr( + self, + merge_request, + expected_remote_target_branch_sha, + source_repo_url=None, + ): + log.info('Accept MR !%s', merge_request.iid) + + # Make sure latest commit in remote is the one we tested against + new_target_sha = Commit.last_on_branch(self._project.id, merge_request.target_branch, self._api).id + if new_target_sha != expected_remote_target_branch_sha: + raise CannotBatch('Someone was naughty and by-passed marge') + + # Rebase and apply the trailers + self.update_merge_request( + merge_request, + source_repo_url=source_repo_url, + ) # This switches git to final_sha = self.merge_batch( @@ -160,7 +178,6 @@ def accept_mr( merge_request.source_branch, self._options.use_no_ff_batches, ) - # Don't force push in case the remote has changed. self._repo.push(merge_request.target_branch, force=False) @@ -203,6 +220,11 @@ def execute(self): self._repo.checkout_branch(target_branch, 'origin/%s' % target_branch) self._repo.checkout_branch(BatchMergeJob.BATCH_BRANCH_NAME, 'origin/%s' % target_branch) + batch_mr = self.create_batch_mr( + target_branch=target_branch, + ) + batch_mr_sha = batch_mr.sha + working_merge_requests = [] for merge_request in merge_requests: @@ -213,42 +235,65 @@ def execute(self): '%s/%s' % (merge_request_remote, merge_request.source_branch), ) - # Update on latest branch so it contains previous MRs - self.fuse( - merge_request.source_branch, - BatchMergeJob.BATCH_BRANCH_NAME, - source_repo_url=source_repo_url, - local=True, - ) - - # Update branch with MR changes - self._repo.fast_forward( - BatchMergeJob.BATCH_BRANCH_NAME, - merge_request.source_branch, - local=True, - ) + if self._options.use_merge_commit_batches: + # Rebase and apply the trailers before running the batch MR + actual_sha = self.update_merge_request( + merge_request, + source_repo_url=source_repo_url, + ) + # Update branch with MR changes + batch_mr_sha = self._repo.merge( + BatchMergeJob.BATCH_BRANCH_NAME, + merge_request.source_branch, + '-m', + 'Batch merge !%s into %s (!%s)' % ( + merge_request.iid, + merge_request.target_branch, + batch_mr.iid + ), + local=True, + ) + else: + # Update on latest branch so it contains previous MRs + self.fuse( + merge_request.source_branch, + BatchMergeJob.BATCH_BRANCH_NAME, + source_repo_url=source_repo_url, + local=True, + ) + # Update branch with MR changes + self._repo.fast_forward( + BatchMergeJob.BATCH_BRANCH_NAME, + merge_request.source_branch, + local=True, + ) # We don't need anymore. Remove it now in case another # merge request is using the same branch name in a different project. - # FIXME: is this actually needed? self._repo.remove_branch(merge_request.source_branch) except git.GitError: log.warning('Skipping MR !%s, got conflicts while rebasing', merge_request.iid) continue else: + if self._options.use_merge_commit_batches: + # update merge_request with the current sha, we will compare it with + # the actual sha later to make sure no one pushed this MR meanwhile + merge_request.update_sha(actual_sha) + working_merge_requests.append(merge_request) + if len(working_merge_requests) <= 1: raise CannotBatch('not enough ready merge requests') + + # This switches git to branch + self.push_batch() + for merge_request in working_merge_requests: + merge_request.comment('I will attempt to batch this MR (!{})...'.format(batch_mr.iid)) + + # wait for the CI of the batch MR 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, - ) - 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) + self.wait_for_ci_to_pass(batch_mr, commit_sha=batch_mr_sha) except CannotMerge as err: for merge_request in working_merge_requests: merge_request.comment( @@ -258,17 +303,23 @@ def execute(self): ), ) raise CannotBatch(err.reason) from err + + # check each sub MR, and accept each sub MR if using the normal batch 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, - ) + # we know the batch MR's CI passed, so we skip CI for sub MRs this time + self.ensure_mergeable_mr(merge_request, skip_ci=True) + + if not self._options.use_merge_commit_batches: + # accept each MRs + remote_target_branch_sha = self.accept_mr( + merge_request, + remote_target_branch_sha, + source_repo_url=source_repo_url, + ) except CannotBatch as err: merge_request.comment( "I couldn't merge this branch: {error} I will retry later...".format( @@ -283,3 +334,24 @@ def execute(self): self.unassign_from_mr(merge_request) merge_request.comment("I couldn't merge this branch: %s" % err.reason) raise + + # Accept the batch MR + if self._options.use_merge_commit_batches: + # Approve the batch MR using the last sub MR's approvers + if not batch_mr.fetch_approvals().sufficient: + approvals = working_merge_requests[-1].fetch_approvals() + try: + approvals.approve(batch_mr) + except (gitlab.Forbidden, gitlab.Unauthorized): + log.exception('Failed to approve MR:') + + try: + ret = batch_mr.accept( + remove_branch=batch_mr.force_remove_source_branch, + sha=batch_mr_sha, + merge_when_pipeline_succeeds=bool(self._project.only_allow_merge_if_pipeline_succeeds), + ) + log.info('batch_mr.accept result: %s', ret) + except gitlab.ApiError as err: + log.exception('Gitlab API Error:') + raise CannotMerge('Gitlab API Error: %s' % err) diff --git a/marge/git.py b/marge/git.py index 3b424b5e..8ad95286 100644 --- a/marge/git.py +++ b/marge/git.py @@ -129,9 +129,10 @@ def remove_branch(self, branch, *, new_current_branch='master'): self.git('branch', '-D', branch) def checkout_branch(self, branch, start_point=''): - self.git('checkout', '-B', branch, start_point, '--') + create_and_reset = '-B' if start_point else '' + self.git('checkout', create_and_reset, 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 +147,8 @@ 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)) + skip_flag = ('-o', 'ci.skip') if skip_ci else () + self.git('push', force_flag, *skip_flag, 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 b2a22b81..c880bb95 100644 --- a/marge/job.py +++ b/marge/job.py @@ -265,10 +265,11 @@ def fuse(self, source, target, source_repo_url=None, local=False): def update_from_target_branch_and_push( self, merge_request, - *, source_repo_url=None, + skip_ci=False, + add_trailers=True, ): - """Updates `target_branch` with commits from `source_branch`, optionally add trailers and push. + """Updates `source_branch` on `target_branch`, optionally add trailers and push. The update strategy can either be rebase or merge. The default is rebase. Returns @@ -296,19 +297,17 @@ def update_from_target_branch_and_push( target_sha = repo.get_commit_hash('origin/' + target_branch) if updated_sha == target_sha: raise CannotMerge('these changes already exist in branch `{}`'.format(target_branch)) - final_sha = self.add_trailers(merge_request) or updated_sha + final_sha = self.add_trailers(merge_request) if add_trailers else None + final_sha = final_sha or updated_sha commits_rewrite_done = True branch_was_modified = final_sha != initial_mr_sha - self.synchronize_mr_with_local_changes(merge_request, branch_was_modified, source_repo_url) + self.synchronize_mr_with_local_changes( + merge_request, + branch_was_modified, + source_repo_url, + skip_ci=skip_ci, + ) except git.GitError: - if not branch_update_done: - raise CannotMerge('got conflicts while rebasing, your problem now...') - if not commits_rewrite_done: - raise CannotMerge('failed on filter-branch; check my logs!') - raise - else: - return target_sha, updated_sha, final_sha - finally: # A failure to clean up probably means something is fucked with the git repo # and likely explains any previous failure, so it will better to just # raise a GitError @@ -316,11 +315,19 @@ def update_from_target_branch_and_push( repo.checkout_branch('master') repo.remove_branch(source_branch) + if not branch_update_done: + raise CannotMerge('got conflicts while rebasing, your problem now...') + if not commits_rewrite_done: + raise CannotMerge('failed on filter-branch; check my logs!') + raise + return target_sha, updated_sha, final_sha + def synchronize_mr_with_local_changes( self, merge_request, branch_was_modified, source_repo_url=None, + skip_ci=False, ): if self._options.fusion is Fusion.gitlab_rebase: self.synchronize_using_gitlab_rebase(merge_request) @@ -329,6 +336,7 @@ def synchronize_mr_with_local_changes( merge_request, branch_was_modified, source_repo_url=source_repo_url, + skip_ci=skip_ci, ) def push_force_to_mr( @@ -336,12 +344,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(): @@ -409,6 +419,8 @@ class Fusion(enum.Enum): 'ci_timeout', 'fusion', 'use_no_ff_batches', + 'use_merge_commit_batches', + 'skip_ci_batches', ] @@ -424,7 +436,7 @@ def default( cls, *, add_tested=False, add_part_of=False, add_reviewers=False, reapprove=False, approval_timeout=None, embargo=None, ci_timeout=None, fusion=Fusion.rebase, - use_no_ff_batches=False, + use_no_ff_batches=False, use_merge_commit_batches=False, skip_ci_batches=False, ): approval_timeout = approval_timeout or timedelta(seconds=0) embargo = embargo or IntervalUnion.empty() @@ -439,6 +451,8 @@ def default( ci_timeout=ci_timeout, fusion=fusion, use_no_ff_batches=use_no_ff_batches, + use_merge_commit_batches=use_merge_commit_batches, + skip_ci_batches=skip_ci_batches, ) diff --git a/marge/merge_request.py b/marge/merge_request.py index bce1bbf8..f855690f 100644 --- a/marge/merge_request.py +++ b/marge/merge_request.py @@ -121,6 +121,10 @@ def web_url(self): def force_remove_source_branch(self): return self.info['force_remove_source_branch'] + def update_sha(self, sha): + """record the updated sha. We don't use refetch_info instead as it may hit cache.""" + self._info['sha'] = sha + def refetch_info(self): self._info = self._api.call(GET('/projects/{0.project_id}/merge_requests/{0.iid}'.format(self))) diff --git a/marge/single_merge_job.py b/marge/single_merge_job.py index f4ed83ef..75dc2c59 100644 --- a/marge/single_merge_job.py +++ b/marge/single_merge_job.py @@ -80,11 +80,12 @@ def update_merge_request_and_accept(self, approvals): self.ensure_mergeable_mr(merge_request) try: - merge_request.accept( + ret = merge_request.accept( remove_branch=merge_request.force_remove_source_branch, sha=actual_sha, merge_when_pipeline_succeeds=bool(target_project.only_allow_merge_if_pipeline_succeeds), ) + log.info('merge_request.accept result: %s', ret) except gitlab.NotAcceptable as err: new_target_sha = Commit.last_on_branch(self._project.id, merge_request.target_branch, api).id # target_branch has moved under us since we updated, just try again diff --git a/pylintrc b/pylintrc index de05e3d5..bc5347e1 100644 --- a/pylintrc +++ b/pylintrc @@ -31,6 +31,8 @@ max-line-length=110 max-args=10 max-attributes=15 max-public-methods=35 +# Maximum number of locals for function / method body +max-locals=25 [REPORTS] output-format=parseable diff --git a/tests/test_job.py b/tests/test_job.py index 6871a80d..77f59611 100644 --- a/tests/test_job.py +++ b/tests/test_job.py @@ -201,6 +201,8 @@ def test_default(self): ci_timeout=timedelta(minutes=15), fusion=Fusion.rebase, use_no_ff_batches=False, + use_merge_commit_batches=False, + skip_ci_batches=False, ) def test_default_ci_time(self):