Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions marge/batch_job.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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)
Expand All @@ -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()

Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for 20 seconds to get status update seems brutal. Would it be better to have a function to poll the status and have an interval and a limit? It can be used for other sleeps as well.


working_merge_requests = []

for merge_request in merge_requests:
Expand Down Expand Up @@ -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:
Expand Down
3 changes: 3 additions & 0 deletions marge/git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion marge/gitlab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
17 changes: 15 additions & 2 deletions tests/test_batch_job.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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()

Expand Down