summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorOswaldo Ferreira <oswaldo@gitlab.com>2017-10-27 22:11:21 +0200
committerOswaldo Ferreira <oswaldo@gitlab.com>2017-10-27 22:33:48 +0200
commita6c52c4e593850eb270e5bfbd021cdfb56e6eed6 (patch)
tree78a94e69c450dc157176102f119e1396a6523a18
parentdfd6c3f824ada5558546b809fd32468325788b94 (diff)
downloadgitlab-ce-a6c52c4e593850eb270e5bfbd021cdfb56e6eed6.tar.gz
Make merge_jid handling less stateful in MergeService
-rw-r--r--app/models/merge_request.rb4
-rw-r--r--app/services/merge_requests/merge_service.rb9
-rw-r--r--app/workers/stuck_merge_jobs_worker.rb2
-rw-r--r--spec/models/merge_request_spec.rb6
-rw-r--r--spec/services/merge_requests/merge_service_spec.rb49
-rw-r--r--spec/workers/stuck_merge_jobs_worker_spec.rb9
6 files changed, 18 insertions, 61 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index c3fae16d109..c952ab862d7 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -396,7 +396,9 @@ class MergeRequest < ActiveRecord::Base
end
def merge_ongoing?
- !!merge_jid && !merged? && Gitlab::SidekiqStatus.running?(merge_jid)
+ # While the MergeRequest is locked, it should present itself as 'merge ongoing'.
+ # The unlocking process is handled by StuckMergeJobsWorker scheduled in Cron.
+ locked? || !!merge_jid && !merged? && Gitlab::SidekiqStatus.running?(merge_jid)
end
def closed_without_fork?
diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb
index 8c5821aa870..156e7b2f078 100644
--- a/app/services/merge_requests/merge_service.rb
+++ b/app/services/merge_requests/merge_service.rb
@@ -82,16 +82,9 @@ module MergeRequests
@merge_request.can_remove_source_branch?(branch_deletion_user)
end
- # Logs merge error message and cleans `MergeRequest#merge_jid`.
- #
def handle_merge_error(log_message:, save_message_on_model: false)
Rails.logger.error("MergeService ERROR: #{merge_request_info} - #{log_message}")
-
- if save_message_on_model
- @merge_request.update(merge_error: log_message, merge_jid: nil)
- else
- clean_merge_jid
- end
+ @merge_request.update(merge_error: log_message) if save_message_on_model
end
def merge_request_info
diff --git a/app/workers/stuck_merge_jobs_worker.rb b/app/workers/stuck_merge_jobs_worker.rb
index 7843179d77c..a396c0f27b2 100644
--- a/app/workers/stuck_merge_jobs_worker.rb
+++ b/app/workers/stuck_merge_jobs_worker.rb
@@ -23,7 +23,7 @@ class StuckMergeJobsWorker
merge_requests = MergeRequest.where(id: completed_ids)
merge_requests.where.not(merge_commit_sha: nil).update_all(state: :merged)
- merge_requests.where(merge_commit_sha: nil).update_all(state: :opened)
+ merge_requests.where(merge_commit_sha: nil).update_all(state: :opened, merge_jid: nil)
Rails.logger.info("Updated state of locked merge jobs. JIDs: #{completed_jids.join(', ')}")
end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 2b8aa7cf9c3..476a2697605 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1460,6 +1460,12 @@ describe MergeRequest do
end
describe '#merge_ongoing?' do
+ it 'returns true when the merge request is locked' do
+ merge_request = build_stubbed(:merge_request, state: :locked)
+
+ expect(merge_request.merge_ongoing?).to be(true)
+ end
+
it 'returns true when merge_id, MR is not merged and it has no running job' do
merge_request = build_stubbed(:merge_request, state: :open, merge_jid: 'foo')
allow(Gitlab::SidekiqStatus).to receive(:running?).with('foo') { true }
diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb
index d1043f99b5a..ac196e92601 100644
--- a/spec/services/merge_requests/merge_service_spec.rb
+++ b/spec/services/merge_requests/merge_service_spec.rb
@@ -12,55 +12,6 @@ describe MergeRequests::MergeService do
end
describe '#execute' do
- context 'MergeRequest#merge_jid' do
- let(:service) do
- described_class.new(project, user, commit_message: 'Awesome message')
- end
-
- before do
- merge_request.update_column(:merge_jid, 'hash-123')
- end
-
- it 'is cleaned when no error is raised' do
- service.execute(merge_request)
-
- expect(merge_request.reload.merge_jid).to be_nil
- end
-
- it 'is cleaned when expected error is raised' do
- allow(service).to receive(:commit).and_raise(described_class::MergeError)
-
- service.execute(merge_request)
-
- expect(merge_request.reload.merge_jid).to be_nil
- end
-
- it 'is cleaned when merge request is not mergeable' do
- allow(merge_request).to receive(:mergeable?).and_return(false)
-
- service.execute(merge_request)
-
- expect(merge_request.reload.merge_jid).to be_nil
- end
-
- it 'is cleaned when no source is found' do
- allow(merge_request).to receive(:diff_head_sha).and_return(nil)
-
- service.execute(merge_request)
-
- expect(merge_request.reload.merge_jid).to be_nil
- end
-
- it 'is not cleaned when unexpected error is raised' do
- service = described_class.new(project, user, commit_message: 'Awesome message')
- allow(service).to receive(:commit).and_raise(StandardError)
-
- expect { service.execute(merge_request) }.to raise_error(StandardError)
-
- expect(merge_request.reload.merge_jid).to be_present
- end
- end
-
context 'valid params' do
let(:service) { described_class.new(project, user, commit_message: 'Awesome message') }
diff --git a/spec/workers/stuck_merge_jobs_worker_spec.rb b/spec/workers/stuck_merge_jobs_worker_spec.rb
index a5ad78393c9..f8b55e873df 100644
--- a/spec/workers/stuck_merge_jobs_worker_spec.rb
+++ b/spec/workers/stuck_merge_jobs_worker_spec.rb
@@ -12,8 +12,13 @@ describe StuckMergeJobsWorker do
worker.perform
- expect(mr_with_sha.reload).to be_merged
- expect(mr_without_sha.reload).to be_opened
+ mr_with_sha.reload
+ mr_without_sha.reload
+
+ expect(mr_with_sha).to be_merged
+ expect(mr_without_sha).to be_opened
+ expect(mr_with_sha.merge_jid).to be_present
+ expect(mr_without_sha.merge_jid).to be_nil
end
it 'updates merge request to opened when locked but has not been merged' do