fix: resolve issue with batching MRs and forked MRs (#302)

Since all forked MRs are prepared to local branches, the MR
acceptance procedure needs to be also local.

* fix batching MRs when source branch has different repo url
* fix unit tests
* add unit test for use case forked MRs
* remove credentials from debug log

Changes:
/tmpiq7raaob/tmpb84bjxf0 # git merge origin/dummy21 --ff --ff-only
merge: origin/dummy21 - not something we can merge

Into:
/tmpiq7raaob/tmpb84bjxf0 # git merge dummy21 --ff --ff-only
Updating b51f541..9525b89
Fast-forward
 21 | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 21

Signed-off-by: Sascha Binckly <[email protected]>

Author: sg70

marge/batch_job.py

@@ -1,6 +1,6 @@
 # pylint: disable=too-many-branches,too-many-statements,arguments-differ
 import logging as log
-from time import sleep
+import time
 
 from . import git
 from . import gitlab
@@ -110,17 +110,19 @@ def ensure_mr_not_changed(self, merge_request):
             if getattr(changed_mr, attr) != getattr(merge_request, attr):
                 raise CannotMerge(error_message.format(attr.replace('_', ' ')))
 
-    def merge_batch(self, target_branch, source_branch, no_ff=False):
+    def merge_batch(self, target_branch, source_branch, no_ff=False, source_repo_url=None, local=False):
         if no_ff:
             return self._repo.merge(
                     target_branch,
                     source_branch,
                     '--no-ff',
+                    source_repo_url,
             )
 
         return self._repo.fast_forward(
             target_branch,
             source_branch,
+            local=local
         )
 
     def update_merge_request(
@@ -166,22 +168,29 @@ def accept_mr(
         if new_target_sha != expected_remote_target_branch_sha:
             raise CannotBatch('Someone was naughty and by-passed marge')
 
+        log.debug("batch: accept_mr: self.update_merge_request")
         # Rebase and apply the trailers
         self.update_merge_request(
             merge_request,
             source_repo_url=source_repo_url,
         )
 
-        # This switches git to <target_branch>
+        log.debug("batch: accept_mr: self.merge_batch")
         final_sha = self.merge_batch(
             merge_request.target_branch,
             merge_request.source_branch,
             self._options.use_no_ff_batches,
+            source_repo_url,
+            local=True  # since we're merging multiple MRs from forks, local needs to be used
         )
+        log.debug("batch: accept_mr: self._repo.push")
         # Don't force push in case the remote has changed.
         self._repo.push(merge_request.target_branch, force=False)
 
-        sleep(2)
+        # delay in case Gitlab is slow to respond
+        waiting_time_in_secs = 10
+        log.debug('Waiting for %s secs for Gitlab to start CI after push', waiting_time_in_secs)
+        time.sleep(waiting_time_in_secs)
 
         # At this point Gitlab should have recognised the MR as being accepted.
         log.info('Successfully merged MR !%s', merge_request.iid)
@@ -192,6 +201,9 @@ def accept_mr(
             branch=merge_request.source_branch,
             status='running',
         )
+
+        log.debug("batch: accept_mr: canceling pipelines in Gitlab")
+        # NOTE: this might not abort the pipeline running in external CI, it merely disconnects the pipeline
         for pipeline in pipelines:
             pipeline.cancel()
 
@@ -225,6 +237,11 @@ def execute(self):
         )
         batch_mr_sha = batch_mr.sha
 
+        # delay in case Gitlab is slow to respond
+        waiting_time_in_secs = 20
+        log.debug('Waiting for %s secs for MR !%s to get status update', waiting_time_in_secs, batch_mr.iid)
+        time.sleep(waiting_time_in_secs)
+
         working_merge_requests = []
 
         for merge_request in merge_requests:
@@ -290,6 +307,11 @@ def execute(self):
         for merge_request in working_merge_requests:
             merge_request.comment('I will attempt to batch this MR (!{})...'.format(batch_mr.iid))
 
+        # delay in case Gitlab is slow to respond
+        waiting_time_in_secs = 10
+        log.debug('Waiting for %s secs for Gitlab to start CI after push', waiting_time_in_secs)
+        time.sleep(waiting_time_in_secs)
+
         # wait for the CI of the batch MR
         if self._project.only_allow_merge_if_pipeline_succeeds:
             try:

marge/git.py

@@ -104,6 +104,9 @@ def rebase(self, branch, new_base, source_repo_url=None, local=False):
     def _fuse_branch(self, strategy, branch, target_branch, *fuse_args, source_repo_url=None, local=False):
         assert source_repo_url or branch != target_branch, branch
 
+        log.debug("git: _fuse_branch: %s %s %s %s", branch, target_branch, source_repo_url, local)
+
+        log.warning("git: _fuse_branch: local: %s", local)
         if not local:
             self.fetch('origin')
             target = 'origin/' + target_branch

marge/gitlab.py

@@ -16,7 +16,7 @@ def call(self, command, sudo=None):
         headers = {'PRIVATE-TOKEN': self._auth_token}
         if sudo:
             headers['SUDO'] = '%d' % sudo
-        log.debug('REQUEST: %s %s %r %r', method.__name__.upper(), url, headers, command.call_args)
+        log.debug('REQUEST: %s %s %r', method.__name__.upper(), url, command.call_args)
         # Timeout to prevent indefinitely hanging requests. 60s is very conservative,
         # but should be short enough to not cause any practical annoyances. We just
         # crash rather than retry since marge-bot should be run in a restart loop anyway.

tests/test_batch_job.py

@@ -142,10 +142,22 @@ def test_merge_batch(self, api, mocklab):
         batch_merge_job = self.get_batch_merge_job(api, mocklab)
         target_branch = 'master'
         source_branch = mocklab.merge_request_info['source_branch']
-        batch_merge_job.merge_batch(target_branch, source_branch, no_ff=False)
+        batch_merge_job.merge_batch(target_branch, source_branch, no_ff=False, local=False)
         batch_merge_job._repo.fast_forward.assert_called_once_with(
             target_branch,
             source_branch,
+            local=False
+        )
+
+    def test_merge_batch_forks(self, api, mocklab):
+        batch_merge_job = self.get_batch_merge_job(api, mocklab)
+        target_branch = 'master'
+        source_branch = mocklab.merge_request_info['source_branch']
+        batch_merge_job.merge_batch(target_branch, source_branch, no_ff=False, local=True)
+        batch_merge_job._repo.fast_forward.assert_called_once_with(
+            target_branch,
+            source_branch,
+            local=True
         )
 
     def test_merge_batch_with_no_ff_enabled(self, api, mocklab):
@@ -156,7 +168,8 @@ def test_merge_batch_with_no_ff_enabled(self, api, mocklab):
         batch_merge_job._repo.merge.assert_called_once_with(
             target_branch,
             source_branch,
-            '--no-ff'
+            '--no-ff',
+            None,
         )
         batch_merge_job._repo.fast_forward.assert_not_called()