Optimistic batching for batch merging

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.

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)

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 <source_branch> on latest <batch> 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 <batch> 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 <batch> 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 <source_branch> 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 <source_branch> 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

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
 
 

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")."""

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():

pylintrc

@@ -30,6 +30,7 @@ max-line-length=110
 [DESIGN]
 max-args=10
 max-attributes=15
+max-locals=16
 max-public-methods=35
 
 [REPORTS]

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]