summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorShinya Maeda <shinya@gitlab.com>2019-06-06 14:25:06 +0700
committerShinya Maeda <shinya@gitlab.com>2019-06-12 13:20:49 +0700
commitb8c131715296dbd864bb7417ef04fc2ad7e18a53 (patch)
treec6d9ae7c27b30db1e20c04b5ca925401171ac0d1
parent9baff6f6b58981ac1565d371b01948a286f7bffd (diff)
downloadgitlab-ce-b8c131715296dbd864bb7417ef04fc2ad7e18a53.tar.gz
Update merge options for auto merge strategies
Currently, merge options is updated on #execute method, however, we should have #update interface to make it explicit.
-rw-r--r--app/controllers/projects/merge_requests_controller.rb12
-rw-r--r--app/services/auto_merge/base_service.rb8
-rw-r--r--app/services/auto_merge_service.rb6
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb26
-rw-r--r--spec/services/auto_merge/base_service_spec.rb24
-rw-r--r--spec/services/auto_merge_service_spec.rb24
6 files changed, 96 insertions, 4 deletions
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index a3ceb76845e..9e7e3ed5afb 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -273,9 +273,15 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
@merge_request.update(merge_error: nil, squash: merge_params.fetch(:squash, false))
if auto_merge_requested?
- AutoMergeService.new(project, current_user, merge_params)
- .execute(merge_request,
- params[:auto_merge_strategy] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
+ if merge_request.auto_merge_enabled?
+ # TODO: We should have a dedicated endpoint for updating merge params.
+ # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63130.
+ AutoMergeService.new(project, current_user, merge_params).update(merge_request)
+ else
+ AutoMergeService.new(project, current_user, merge_params)
+ .execute(merge_request,
+ params[:auto_merge_strategy] || AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS)
+ end
else
@merge_request.merge_async(current_user.id, merge_params)
diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb
index 7f0a41b3dfa..d726085b89a 100644
--- a/app/services/auto_merge/base_service.rb
+++ b/app/services/auto_merge/base_service.rb
@@ -20,6 +20,14 @@ module AutoMerge
strategy.to_sym
end
+ def update(merge_request)
+ merge_request.merge_params.merge!(params)
+
+ return :failed unless merge_request.save
+
+ strategy.to_sym
+ end
+
def cancel(merge_request)
if cancel_auto_merge(merge_request)
yield if block_given?
diff --git a/app/services/auto_merge_service.rb b/app/services/auto_merge_service.rb
index a3a780ff388..926d2f5fc66 100644
--- a/app/services/auto_merge_service.rb
+++ b/app/services/auto_merge_service.rb
@@ -24,6 +24,12 @@ class AutoMergeService < BaseService
service.execute(merge_request)
end
+ def update(merge_request)
+ return :failed unless merge_request.auto_merge_enabled?
+
+ get_service_instance(merge_request.auto_merge_strategy).update(merge_request)
+ end
+
def process(merge_request)
return unless merge_request.auto_merge_enabled?
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index f8c0ab55eb4..34cbf0c8723 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -412,7 +412,7 @@ describe Projects::MergeRequestsController do
end
end
- context 'when the pipeline succeeds is passed' do
+ context 'when merge when pipeline succeeds option is passed' do
let!(:head_pipeline) do
create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request)
end
@@ -462,6 +462,30 @@ describe Projects::MergeRequestsController do
expect(json_response).to eq('status' => 'merge_when_pipeline_succeeds')
end
end
+
+ context 'when auto merge has not been enabled yet' do
+ it 'calls AutoMergeService#execute' do
+ expect_next_instance_of(AutoMergeService) do |service|
+ expect(service).to receive(:execute).with(merge_request, 'merge_when_pipeline_succeeds')
+ end
+
+ merge_when_pipeline_succeeds
+ end
+ end
+
+ context 'when auto merge has already been enabled' do
+ before do
+ merge_request.update!(auto_merge_enabled: true, merge_user: user)
+ end
+
+ it 'calls AutoMergeService#update' do
+ expect_next_instance_of(AutoMergeService) do |service|
+ expect(service).to receive(:update).with(merge_request)
+ end
+
+ merge_when_pipeline_succeeds
+ end
+ end
end
describe 'only_allow_merge_if_all_discussions_are_resolved? setting' do
diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb
index 35d60d6abbb..cd08e0b6f32 100644
--- a/spec/services/auto_merge/base_service_spec.rb
+++ b/spec/services/auto_merge/base_service_spec.rb
@@ -92,6 +92,30 @@ describe AutoMerge::BaseService do
end
end
+ describe '#update' do
+ subject { service.update(merge_request) }
+
+ let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
+
+ context 'when merge params are specified' do
+ let(:params) do
+ {
+ 'commit_message' => "Merge branch 'patch-12' into 'master'",
+ 'sha' => "200fcc9c260f7219eaf0daba87d818f0922c5b18",
+ 'should_remove_source_branch' => false,
+ 'squash' => false,
+ 'squash_commit_message' => "Update README.md"
+ }
+ end
+
+ it 'updates merge params' do
+ expect { subject }.to change {
+ merge_request.reload.merge_params.slice(*params.keys)
+ }.from({}).to(params)
+ end
+ end
+ end
+
describe '#cancel' do
subject { service.cancel(merge_request) }
diff --git a/spec/services/auto_merge_service_spec.rb b/spec/services/auto_merge_service_spec.rb
index d0eefed3150..7e9c412adb3 100644
--- a/spec/services/auto_merge_service_spec.rb
+++ b/spec/services/auto_merge_service_spec.rb
@@ -92,6 +92,30 @@ describe AutoMergeService do
end
end
+ describe '#update' do
+ subject { service.update(merge_request) }
+
+ context 'when auto merge is enabled' do
+ let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) }
+
+ it 'delegates to a relevant service instance' do
+ expect_next_instance_of(AutoMerge::MergeWhenPipelineSucceedsService) do |service|
+ expect(service).to receive(:update).with(merge_request)
+ end
+
+ subject
+ end
+ end
+
+ context 'when auto merge is not enabled' do
+ let(:merge_request) { create(:merge_request) }
+
+ it 'returns failed' do
+ is_expected.to eq(:failed)
+ end
+ end
+ end
+
describe '#process' do
subject { service.process(merge_request) }