From a6c52c4e593850eb270e5bfbd021cdfb56e6eed6 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Fri, 27 Oct 2017 22:11:21 +0200 Subject: Make merge_jid handling less stateful in MergeService --- app/models/merge_request.rb | 4 +- app/services/merge_requests/merge_service.rb | 9 +--- app/workers/stuck_merge_jobs_worker.rb | 2 +- spec/models/merge_request_spec.rb | 6 +++ spec/services/merge_requests/merge_service_spec.rb | 49 ---------------------- spec/workers/stuck_merge_jobs_worker_spec.rb | 9 +++- 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 -- cgit v1.2.3 From 7e60c764282c84d4acea517903705a4c4e3a61f2 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Fri, 27 Oct 2017 22:41:44 +0200 Subject: Add changelog --- changelogs/unreleased/make-merge-jid-handling-less-stateful.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/make-merge-jid-handling-less-stateful.yml diff --git a/changelogs/unreleased/make-merge-jid-handling-less-stateful.yml b/changelogs/unreleased/make-merge-jid-handling-less-stateful.yml new file mode 100644 index 00000000000..fe945e822fd --- /dev/null +++ b/changelogs/unreleased/make-merge-jid-handling-less-stateful.yml @@ -0,0 +1,5 @@ +--- +title: Fix widget of locked merge requests not being presented +merge_request: +author: +type: fixed -- cgit v1.2.3 From c4124fce48e83db603d69a83bb912ae5d0d54f65 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 30 Oct 2017 11:21:23 +0100 Subject: Move locked check to a guard-clause --- app/models/merge_request.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index c952ab862d7..07352db5d2d 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -398,7 +398,9 @@ class MergeRequest < ActiveRecord::Base def merge_ongoing? # 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) + return true if locked? + + !!merge_jid && !merged? && Gitlab::SidekiqStatus.running?(merge_jid) end def closed_without_fork? -- cgit v1.2.3