diff options
author | James Lopez <james@gitlab.com> | 2019-06-12 10:25:02 +0300 |
---|---|---|
committer | James Lopez <james@gitlab.com> | 2019-06-12 10:25:02 +0300 |
commit | cb9dc4adcab890bc79ca28caae882db08163f6ed (patch) | |
tree | 3752bb276e93a2abe00c80d855d88018d64ba3fe | |
parent | 25347c8e91c06fcd1f60eba2cf17873816a450e6 (diff) | |
parent | b8c131715296dbd864bb7417ef04fc2ad7e18a53 (diff) |
Merge branch 'update-auto-merge-parameters' into 'master'
Update merge options for auto merge strategies
See merge request gitlab-org/gitlab-ce!29334
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 12 | ||||
-rw-r--r-- | app/services/auto_merge/base_service.rb | 8 | ||||
-rw-r--r-- | app/services/auto_merge_service.rb | 6 | ||||
-rw-r--r-- | spec/controllers/projects/merge_requests_controller_spec.rb | 26 | ||||
-rw-r--r-- | spec/services/auto_merge/base_service_spec.rb | 24 | ||||
-rw-r--r-- | spec/services/auto_merge_service_spec.rb | 24 |
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) } |