diff --git a/README.md b/README.md index 27ee7494..aa2c6459 100644 --- a/README.md +++ b/README.md @@ -128,6 +128,11 @@ optional arguments: [env var: MARGE_SOURCE_BRANCH_REGEXP] (default: .*) --debug Debug logging (includes all HTTP requests etc). [env var: MARGE_DEBUG] (default: False) + --optimistic-batch Optimistic batching enabled. This is similar to the above batch feature + but it will optimistically add trailers to each MR that will be merged. This will + ensure that the individual MR git hash matches up with the hash of the + equivalent changes that were added to the batch MR + ``` Here is a config file example ```yaml @@ -433,6 +438,26 @@ request, before attempting a new batch job. guarantee that the subset will. However, this would only happen in a rather convoluted situation that can be considered to be very rare. +## Optimistic batching + +This feature enables batching of MRs but optimistically adds the trailers to the +individual MRs before the batch MR pipeline runs. This is to ensure that the final +git commit hashes that make it into master, match up to the hashes of the commits +that ran through the pipeline. Many users build artifacts/images in their pipelines +and having the commit hashes match up allows them to deploy based on the git +hashes in master. + +### Difference from regular batching + +- Trailers are added to each MR and pushed to remote before the batch MR pipeline +runs +- After the batch MR pipeline passes each MR is rebased onto the previous MR to make +sure the git hashes match those in the branch MR. The branch MR is then merged, which +merges all of the individual MRs +- WARNING: Using this feature will cause individual MRs to be rebased on MRs with other +changes in them. If the merge of the batch MR fails for any reason then it will leave the +individual MRs in an undesirable state + ## Restricting the list of projects marge-bot considers By default marge-bot will work on all projects that she is a member of. diff --git a/marge/app.py b/marge/app.py index a9918111..2d2e9a29 100644 --- a/marge/app.py +++ b/marge/app.py @@ -213,6 +213,11 @@ def regexp(str_regex): action='store_true', help='Disable fast forwarding when merging MR batches' ) + parser.add_argument( + '--optimistic-batch', + action='store_true', + help='Experimental optimistic batch (adds trailers to MRs before batching)\n', + ) config = parser.parse_args(args) if config.use_merge_strategy and config.batch: @@ -280,6 +285,12 @@ def main(args=None): if options.batch: logging.warning('Experimental batch mode enabled') + if options.optimistic_batch: + logging.warning( + 'Experimental optimistic batch mode enabled. This may cause undesirable behaviour ' + 'with respect to MRs.' + ) + if options.use_merge_strategy: fusion = bot.Fusion.merge elif options.rebase_remotely: @@ -313,6 +324,7 @@ def main(args=None): fusion=fusion, ), batch=options.batch, + optimistic_batch=options.optimistic_batch, ) marge_bot = bot.Bot(api=api, config=config) diff --git a/marge/batch_job.py b/marge/batch_job.py index 2f4cae82..ebbe1ac2 100644 --- a/marge/batch_job.py +++ b/marge/batch_job.py @@ -16,8 +16,9 @@ class CannotBatch(Exception): class BatchMergeJob(MergeJob): BATCH_BRANCH_NAME = 'marge_bot_batch_merge_job' - def __init__(self, *, api, user, project, repo, options, merge_requests): + def __init__(self, *, api, user, project, repo, options, optimistic=False, merge_requests): super().__init__(api=api, user=user, project=project, repo=repo, options=options) + self._optimistic = optimistic self._merge_requests = merge_requests def remove_batch_branch(self): @@ -135,20 +136,22 @@ 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, - ) + if not self._optimistic: + # When optimistic batching is enabled we've already applied the trailers + # 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') + 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. @@ -213,6 +216,14 @@ def execute(self): '%s/%s' % (merge_request_remote, merge_request.source_branch), ) + if self._optimistic: + #TODO REBASE ON THE SAME VERSION OF MASTER AS THE + # BATCH MR WAS CREATED FROM HERE!!! + + # 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( merge_request.source_branch, @@ -239,12 +250,12 @@ def execute(self): 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() + batch_mr = self.create_batch_mr( + target_branch=target_branch, + ) 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: @@ -258,19 +269,56 @@ def execute(self): ), ) raise CannotBatch(err.reason) from err - for merge_request in working_merge_requests: + for index, merge_req in enumerate(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, - ) + _, source_repo_url, merge_request_remote = self.fetch_source_project(merge_req) + self.ensure_mr_not_changed(merge_req) + self.ensure_mergeable_mr(merge_req) + if self._optimistic: + if index == 0: + continue + elif index == len(working_merge_requests) - 1: + # Update so it contains previous merge_reqs + self.fuse( + merge_req.source_branch, + working_merge_requests[index - 1], + source_repo_url=source_repo_url, + local=True, + ) + self.push_force_to_mr( + merge_req, + True, + source_repo_url, + skip_ci=True, + ) + remote_target_branch_sha = self.accept_mr( + batch_mr, + remote_target_branch_sha, + source_repo_url=source_repo_url, + ) + else: + # Update so it contains previous merge_reqs + self.fuse( + merge_req.source_branch, + working_merge_requests[index - 1], + source_repo_url=source_repo_url, + local=True, + ) + self.push_force_to_mr( + merge_req, + True, + source_repo_url, + skip_ci=True, + ) + else: + remote_target_branch_sha = self.accept_mr( + merge_req, + remote_target_branch_sha, + source_repo_url=source_repo_url, + ) except CannotBatch as err: - merge_request.comment( + merge_req.comment( "I couldn't merge this branch: {error} I will retry later...".format( error=str(err), ), @@ -280,6 +328,6 @@ def execute(self): # Raise here to avoid being caught below - we don't want to be unassigned. raise except CannotMerge as err: - self.unassign_from_mr(merge_request) - merge_request.comment("I couldn't merge this branch: %s" % err.reason) + self.unassign_from_mr(merge_req) + merge_req.comment("I couldn't merge this branch: %s" % err.reason) raise diff --git a/marge/bot.py b/marge/bot.py index 398dde46..5680c2a0 100644 --- a/marge/bot.py +++ b/marge/bot.py @@ -146,7 +146,7 @@ def _process_merge_requests(self, repo_manager, project, merge_requests): raise log.info('Got %s requests to merge;', len(merge_requests)) - if self._config.batch and len(merge_requests) > 1: + if (self._config.batch or self._config.optimistic_batch) and len(merge_requests) > 1: log.info('Attempting to merge as many MRs as possible using BatchMergeJob...') batch_merge_job = batch_job.BatchMergeJob( api=self._api, @@ -155,6 +155,7 @@ def _process_merge_requests(self, repo_manager, project, merge_requests): merge_requests=merge_requests, repo=repo, options=self._config.merge_opts, + optimistic=self._config.optimistic_batch, ) try: batch_merge_job.execute() @@ -187,7 +188,7 @@ def _get_single_job(self, project, merge_request, repo, options): class BotConfig(namedtuple('BotConfig', 'user ssh_key_file project_regexp merge_order merge_opts git_timeout ' + - 'git_reference_repo branch_regexp source_branch_regexp batch')): + 'git_reference_repo branch_regexp source_branch_regexp batch optimistic_batch')): pass diff --git a/marge/git.py b/marge/git.py index 3b424b5e..5613c700 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,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 2abbce10..1964d273 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]