diff options
| -rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 4 | ||||
| -rw-r--r-- | app/models/merge_request.rb | 12 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_service.rb | 3 | ||||
| -rw-r--r-- | app/services/merge_requests/merge_when_pipeline_succeeds_service.rb | 2 | ||||
| -rw-r--r-- | app/services/merge_requests/update_service.rb | 2 | ||||
| -rw-r--r-- | app/workers/merge_worker.rb | 2 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 39 | ||||
| -rw-r--r-- | spec/services/merge_requests/merge_service_spec.rb | 9 |
8 files changed, 44 insertions, 29 deletions
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2a3b73577a5..6f3dcc1d6fa 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -318,14 +318,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo elsif @merge_request.head_pipeline.success? # This can be triggered when a user clicks the auto merge button while # the tests finish at about the same time - MergeWorker.perform_async(@merge_request.id, current_user.id, params) + @merge_request.async_merge(current_user.id, params) :success else :failed end else - MergeWorker.perform_async(@merge_request.id, current_user.id, params) + @merge_request.async_merge(current_user.id, params) :success end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e283c3981a4..1e212685c71 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -241,6 +241,14 @@ class MergeRequest < ActiveRecord::Base end end + # Calls `MergeWorker` to proceed with the merge process and + # updates `merge_jid` with the MergeWorker#jid. + # This helps tracking enqueued and ongoing merge jobs. + def async_merge(user_id, params) + jid = MergeWorker.perform_async(id, user_id, params) + update_column(:merge_jid, jid) + end + def first_commit merge_request_diff ? merge_request_diff.first_commit : compare_commits.first end @@ -384,9 +392,7 @@ class MergeRequest < ActiveRecord::Base end def merge_ongoing? - return false unless merge_jid - - Gitlab::SidekiqStatus.num_running([merge_jid]) > 0 + merge_jid && !merged? end def closed_without_fork? diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 5be749cd6a0..aac1e7e19c3 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -31,6 +31,9 @@ module MergeRequests end rescue MergeError => e log_merge_error(e.message, save_message_on_model: true) + ensure + # Make sure to clean up merge_jid in the end of the merge process. + merge_request.update_column(:merge_jid, nil) end private diff --git a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb index aed5287940e..57c82e39410 100644 --- a/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb +++ b/app/services/merge_requests/merge_when_pipeline_succeeds_service.rb @@ -30,7 +30,7 @@ module MergeRequests next end - MergeWorker.perform_async(merge_request.id, merge_request.merge_user_id, merge_request.merge_params) + merge_request.async_merge(merge_request.merge_user_id, merge_request.merge_params) end end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 75a65aecd1a..6563a4101a3 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -83,7 +83,7 @@ module MergeRequests if merge_request.head_pipeline && merge_request.head_pipeline.active? MergeRequests::MergeWhenPipelineSucceedsService.new(project, current_user).execute(merge_request) else - MergeWorker.perform_async(merge_request.id, current_user.id, {}) + merge_request.async_merge(current_user.id, {}) end end diff --git a/app/workers/merge_worker.rb b/app/workers/merge_worker.rb index c3b58df92c1..48e2da338f6 100644 --- a/app/workers/merge_worker.rb +++ b/app/workers/merge_worker.rb @@ -7,8 +7,6 @@ class MergeWorker current_user = User.find(current_user_id) merge_request = MergeRequest.find(merge_request_id) - merge_request.update_column(:merge_jid, jid) - MergeRequests::MergeService.new(merge_request.target_project, current_user, params) .execute(merge_request) end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 2d10c6ef1da..de2f6c2139d 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -931,6 +931,23 @@ describe MergeRequest do end end + describe '#async_merge' do + it 'enqueues MergeWorker job and updates merge_jid' do + merge_request = create(:merge_request) + user_id = double(:user_id) + params = double(:params) + merge_jid = 'hash-123' + + expect(MergeWorker).to receive(:perform_async).with(merge_request.id, user_id, params) do + merge_jid + end + + merge_request.async_merge(user_id, params) + + expect(merge_request.reload.merge_jid).to eq(merge_jid) + end + end + describe '#check_if_can_be_merged' do let(:project) { create(:project, only_allow_merge_if_pipeline_succeeds: true) } @@ -1370,29 +1387,11 @@ describe MergeRequest do end describe '#merge_ongoing?' do - it 'returns true when merge process is ongoing for merge_jid' do - merge_request = create(:merge_request, merge_jid: 'foo') - - allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']).and_return(1) + it 'returns true when merge_id is present and MR is not merged' do + merge_request = build_stubbed(:merge_request, state: :open, merge_jid: 'foo') expect(merge_request.merge_ongoing?).to be(true) end - - it 'returns false when no merge process running for merge_jid' do - merge_request = build(:merge_request, merge_jid: 'foo') - - allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']).and_return(0) - - expect(merge_request.merge_ongoing?).to be(false) - end - - it 'returns false when merge_jid is nil' do - merge_request = build(:merge_request, merge_jid: nil) - - expect(Gitlab::SidekiqStatus).not_to receive(:num_running) - - expect(merge_request.merge_ongoing?).to be(false) - end end describe "#closed_without_fork?" do diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 5cfdb5372f3..0a007bdfc3b 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -12,6 +12,15 @@ describe MergeRequests::MergeService do end describe '#execute' do + it 'cleans up merge_jid from MergeRequest' do + merge_request.update_column(:merge_jid, 'hash-123') + service = described_class.new(project, user, commit_message: 'Awesome message') + + service.execute(merge_request) + + expect(merge_request.reload.merge_jid).to be_nil + end + context 'valid params' do let(:service) { described_class.new(project, user, commit_message: 'Awesome message') } |