diff options
-rw-r--r-- | GITLAB_SHELL_VERSION | 2 | ||||
-rw-r--r-- | app/models/merge_request.rb | 15 | ||||
-rw-r--r-- | app/services/auto_merge/base_service.rb | 52 | ||||
-rw-r--r-- | app/services/auto_merge/merge_when_pipeline_succeeds_service.rb | 34 | ||||
-rw-r--r-- | changelogs/unreleased/update-gitlab-shell-9-3-0.yml | 5 | ||||
-rw-r--r-- | spec/frontend/helpers/timeout.js | 16 | ||||
-rw-r--r-- | spec/frontend/test_setup.js | 2 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 16 | ||||
-rw-r--r-- | spec/services/auto_merge/base_service_spec.rb | 144 |
9 files changed, 222 insertions, 64 deletions
diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index deeb3d66ef0..b13d146a7b0 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -9.2.0 +9.3.0 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 59416fb4b51..4fcaac75655 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -984,21 +984,6 @@ class MergeRequest < ApplicationRecord end end - def reset_auto_merge - return unless auto_merge_enabled? - - self.auto_merge_enabled = false - self.merge_user = nil - if merge_params - merge_params.delete('should_remove_source_branch') - merge_params.delete('commit_message') - merge_params.delete('squash_commit_message') - merge_params.delete('auto_merge_strategy') - end - - self.save - end - # Return array of possible target branches # depends on target project of MR def target_branches diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb new file mode 100644 index 00000000000..058105db3a4 --- /dev/null +++ b/app/services/auto_merge/base_service.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +module AutoMerge + class BaseService < ::BaseService + include Gitlab::Utils::StrongMemoize + + def execute(merge_request) + merge_request.merge_params.merge!(params) + merge_request.auto_merge_enabled = true + merge_request.merge_user = current_user + merge_request.auto_merge_strategy = strategy + + return :failed unless merge_request.save + + yield if block_given? + + strategy.to_sym + end + + def cancel(merge_request) + if cancel_auto_merge(merge_request) + yield if block_given? + + success + else + error("Can't cancel the automatic merge", 406) + end + end + + private + + def strategy + strong_memoize(:strategy) do + self.class.name.demodulize.remove('Service').underscore + end + end + + def cancel_auto_merge(merge_request) + merge_request.auto_merge_enabled = false + merge_request.merge_user = nil + + merge_request.merge_params&.except!( + 'should_remove_source_branch', + 'commit_message', + 'squash_commit_message', + 'auto_merge_strategy' + ) + + merge_request.save + end + end +end diff --git a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb index d0586468859..c41073a73e9 100644 --- a/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb +++ b/app/services/auto_merge/merge_when_pipeline_succeeds_service.rb @@ -1,32 +1,12 @@ # frozen_string_literal: true module AutoMerge - class MergeWhenPipelineSucceedsService < BaseService + class MergeWhenPipelineSucceedsService < AutoMerge::BaseService def execute(merge_request) - return :failed unless merge_request.actual_head_pipeline - - if merge_request.actual_head_pipeline.active? - merge_request.merge_params.merge!(params) - - unless merge_request.auto_merge_enabled? - merge_request.auto_merge_enabled = true - merge_request.merge_user = @current_user - merge_request.auto_merge_strategy = AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS - - SystemNoteService.merge_when_pipeline_succeeds(merge_request, @project, @current_user, merge_request.diff_head_commit) + super do + if merge_request.saved_change_to_auto_merge_enabled? + SystemNoteService.merge_when_pipeline_succeeds(merge_request, project, current_user, merge_request.diff_head_commit) end - - return :failed unless merge_request.save - - :merge_when_pipeline_succeeds - elsif merge_request.actual_head_pipeline.success? - # This can be triggered when a user clicks the auto merge button while - # the tests finish at about the same time - merge_request.merge_async(current_user.id, merge_params) - - :success - else - :failed end end @@ -38,12 +18,8 @@ module AutoMerge end def cancel(merge_request) - if merge_request.reset_auto_merge + super do SystemNoteService.cancel_merge_when_pipeline_succeeds(merge_request, @project, @current_user) - - success - else - error("Can't cancel the automatic merge", 406) end end diff --git a/changelogs/unreleased/update-gitlab-shell-9-3-0.yml b/changelogs/unreleased/update-gitlab-shell-9-3-0.yml new file mode 100644 index 00000000000..781ff31c7d8 --- /dev/null +++ b/changelogs/unreleased/update-gitlab-shell-9-3-0.yml @@ -0,0 +1,5 @@ +--- +title: Update to GitLab Shell v9.3.0 +merge_request: 29283 +author: +type: other diff --git a/spec/frontend/helpers/timeout.js b/spec/frontend/helpers/timeout.js index e74598ae20a..702ef0be5aa 100644 --- a/spec/frontend/helpers/timeout.js +++ b/spec/frontend/helpers/timeout.js @@ -5,7 +5,13 @@ const IS_DEBUGGING = process.execArgv.join(' ').includes('--inspect-brk'); let testTimeoutNS; export const setTestTimeout = newTimeoutMS => { - testTimeoutNS = newTimeoutMS * NS_PER_MS; + const newTimeoutNS = newTimeoutMS * NS_PER_MS; + // never accept a smaller timeout than the default + if (newTimeoutNS < testTimeoutNS) { + return; + } + + testTimeoutNS = newTimeoutNS; jest.setTimeout(newTimeoutMS); }; @@ -13,7 +19,13 @@ export const setTestTimeout = newTimeoutMS => { // Useful for tests with jQuery, which is very slow in big DOMs. let temporaryTimeoutNS = null; export const setTestTimeoutOnce = newTimeoutMS => { - temporaryTimeoutNS = newTimeoutMS * NS_PER_MS; + const newTimeoutNS = newTimeoutMS * NS_PER_MS; + // never accept a smaller timeout than the default + if (newTimeoutNS < testTimeoutNS) { + return; + } + + temporaryTimeoutNS = newTimeoutNS; }; export const initializeTestTimeout = defaultTimeoutMS => { diff --git a/spec/frontend/test_setup.js b/spec/frontend/test_setup.js index c24f0bc4776..7e7cc1488b8 100644 --- a/spec/frontend/test_setup.js +++ b/spec/frontend/test_setup.js @@ -15,7 +15,7 @@ afterEach(() => }), ); -initializeTestTimeout(500); +initializeTestTimeout(process.env.CI ? 5000 : 500); // fail tests for unmocked requests beforeEach(done => { diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 956c5675f38..fc28c216b21 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1088,22 +1088,6 @@ describe MergeRequest do end end - describe "#reset_auto_merge" do - let(:merge_if_green) do - create :merge_request, merge_when_pipeline_succeeds: true, merge_user: create(:user), - merge_params: { "should_remove_source_branch" => "1", "commit_message" => "msg" } - end - - it "sets the item to false" do - merge_if_green.reset_auto_merge - merge_if_green.reload - - expect(merge_if_green.merge_when_pipeline_succeeds).to be_falsey - expect(merge_if_green.merge_params["should_remove_source_branch"]).to be_nil - expect(merge_if_green.merge_params["commit_message"]).to be_nil - end - end - describe '#committers' do it 'returns all the committers of every commit in the merge request' do users = subject.commits.without_merge_commits.map(&:committer_email).uniq.map do |email| diff --git a/spec/services/auto_merge/base_service_spec.rb b/spec/services/auto_merge/base_service_spec.rb new file mode 100644 index 00000000000..197fa16961d --- /dev/null +++ b/spec/services/auto_merge/base_service_spec.rb @@ -0,0 +1,144 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe AutoMerge::BaseService do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:service) { described_class.new(project, user, params) } + let(:merge_request) { create(:merge_request) } + let(:params) { {} } + + describe '#execute' do + subject { service.execute(merge_request) } + + it 'sets properies to the merge request' do + subject + + merge_request.reload + expect(merge_request).to be_auto_merge_enabled + expect(merge_request.merge_user).to eq(user) + expect(merge_request.auto_merge_strategy).to eq('base') + end + + it 'yields block' do + expect { |b| service.execute(merge_request, &b) }.to yield_control.once + end + + it 'returns activated strategy name' do + is_expected.to eq(:base) + end + + context 'when merge parameters are given' 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 'sets merge parameters' do + subject + + merge_request.reload + expect(merge_request.merge_params['commit_message']).to eq("Merge branch 'patch-12' into 'master'") + expect(merge_request.merge_params['sha']).to eq('200fcc9c260f7219eaf0daba87d818f0922c5b18') + expect(merge_request.merge_params['should_remove_source_branch']).to eq(false) + expect(merge_request.merge_params['squash']).to eq(false) + expect(merge_request.merge_params['squash_commit_message']).to eq('Update README.md') + end + end + + context 'when strategy is merge when pipeline succeeds' do + let(:service) { AutoMerge::MergeWhenPipelineSucceedsService.new(project, user) } + + it 'sets the auto merge strategy' do + subject + + merge_request.reload + expect(merge_request.auto_merge_strategy).to eq(AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS) + end + + it 'returns activated strategy name' do + is_expected.to eq(AutoMergeService::STRATEGY_MERGE_WHEN_PIPELINE_SUCCEEDS.to_sym) + end + end + + context 'when failed to save' do + before do + allow(merge_request).to receive(:save) { false } + end + + it 'does not yield block' do + expect { |b| service.execute(merge_request, &b) }.not_to yield_control + end + + it 'returns failed' do + is_expected.to eq(:failed) + end + end + end + + describe '#cancel' do + subject { service.cancel(merge_request) } + + let(:merge_request) { create(:merge_request, :merge_when_pipeline_succeeds) } + + it 'removes properies from the merge request' do + subject + + merge_request.reload + expect(merge_request).not_to be_auto_merge_enabled + expect(merge_request.merge_user).to be_nil + expect(merge_request.auto_merge_strategy).to be_nil + end + + it 'yields block' do + expect { |b| service.cancel(merge_request, &b) }.to yield_control.once + end + + it 'returns success status' do + expect(subject[:status]).to eq(:success) + end + + context 'when merge params are set' do + before do + merge_request.update!(merge_params: + { + 'should_remove_source_branch' => false, + 'commit_message' => "Merge branch 'patch-12' into 'master'", + 'squash_commit_message' => "Update README.md", + 'auto_merge_strategy' => 'merge_when_pipeline_succeeds' + }) + end + + it 'removes merge parameters' do + subject + + merge_request.reload + expect(merge_request.merge_params['should_remove_source_branch']).to be_nil + expect(merge_request.merge_params['commit_message']).to be_nil + expect(merge_request.merge_params['squash_commit_message']).to be_nil + expect(merge_request.merge_params['auto_merge_strategy']).to be_nil + end + end + + context 'when failed to save' do + before do + allow(merge_request).to receive(:save) { false } + end + + it 'does not yield block' do + expect { |b| service.execute(merge_request, &b) }.not_to yield_control + end + + it 'returns error status' do + expect(subject[:status]).to eq(:error) + expect(subject[:message]).to eq("Can't cancel the automatic merge") + end + end + end +end |