From 9ab0254085cc4eed5cda322b6d5998c320ceef2c Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Wed, 11 Oct 2017 18:21:25 +0200 Subject: [PATCH 1/2] Make "merge ongoing" check more consistent --- app/models/merge_request.rb | 2 +- ...mprove-merge-ongoing-check-consistency.yml | 5 +++++ spec/models/merge_request_spec.rb | 22 ++++++++++++++++++- 3 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/39032-improve-merge-ongoing-check-consistency.yml diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 972a35dde4d..e34714e3f9e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -396,7 +396,7 @@ class MergeRequest < ActiveRecord::Base end def merge_ongoing? - !!merge_jid && !merged? + !!merge_jid && !merged? && Gitlab::SidekiqStatus.num_running([merge_jid]) > 0 end def closed_without_fork? diff --git a/changelogs/unreleased/39032-improve-merge-ongoing-check-consistency.yml b/changelogs/unreleased/39032-improve-merge-ongoing-check-consistency.yml new file mode 100644 index 00000000000..361b6af196a --- /dev/null +++ b/changelogs/unreleased/39032-improve-merge-ongoing-check-consistency.yml @@ -0,0 +1,5 @@ +--- +title: Make "merge ongoing" check more consistent +merge_request: +author: +type: fixed diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 17c9f15b021..09d7d0dbbe6 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1460,11 +1460,31 @@ describe MergeRequest do end describe '#merge_ongoing?' do - it 'returns true when merge_id is present and MR is not merged' do + 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(:num_running).with(['foo']) { 1 } expect(merge_request.merge_ongoing?).to be(true) end + + it 'returns false when merge_jid is nil' do + merge_request = build_stubbed(:merge_request, state: :open, merge_jid: nil) + + expect(merge_request.merge_ongoing?).to be(false) + end + + it 'returns false if MR is merged' do + merge_request = build_stubbed(:merge_request, state: :merged, merge_jid: 'foo') + + expect(merge_request.merge_ongoing?).to be(false) + end + + it 'returns false if there is no merge job running' do + merge_request = build_stubbed(:merge_request, state: :open, merge_jid: 'foo') + allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']) { 0 } + + expect(merge_request.merge_ongoing?).to be(false) + end end describe "#closed_without_fork?" do From b78954c13d127d84fa1e10db83f51b23790aa526 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Fri, 13 Oct 2017 10:17:41 +0200 Subject: [PATCH 2/2] Simplify check for running job on Redis --- app/models/merge_request.rb | 2 +- lib/gitlab/sidekiq_status.rb | 7 +++++++ spec/lib/gitlab/sidekiq_status_spec.rb | 12 ++++++++++++ spec/models/merge_request_spec.rb | 4 ++-- 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e34714e3f9e..c3fae16d109 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -396,7 +396,7 @@ class MergeRequest < ActiveRecord::Base end def merge_ongoing? - !!merge_jid && !merged? && Gitlab::SidekiqStatus.num_running([merge_jid]) > 0 + !!merge_jid && !merged? && Gitlab::SidekiqStatus.running?(merge_jid) end def closed_without_fork? diff --git a/lib/gitlab/sidekiq_status.rb b/lib/gitlab/sidekiq_status.rb index a0a2769cf9e..a1f689d94d9 100644 --- a/lib/gitlab/sidekiq_status.rb +++ b/lib/gitlab/sidekiq_status.rb @@ -51,6 +51,13 @@ module Gitlab self.num_running(job_ids).zero? end + # Returns true if the given job is running + # + # job_id - The Sidekiq job ID to check. + def self.running?(job_id) + num_running([job_id]) > 0 + end + # Returns the number of jobs that are running. # # job_ids - The Sidekiq job IDs to check. diff --git a/spec/lib/gitlab/sidekiq_status_spec.rb b/spec/lib/gitlab/sidekiq_status_spec.rb index c2e77ef6b6c..884f27b212c 100644 --- a/spec/lib/gitlab/sidekiq_status_spec.rb +++ b/spec/lib/gitlab/sidekiq_status_spec.rb @@ -39,6 +39,18 @@ describe Gitlab::SidekiqStatus do end end + describe '.running?', :clean_gitlab_redis_shared_state do + it 'returns true if job is running' do + described_class.set('123') + + expect(described_class.running?('123')).to be(true) + end + + it 'returns false if job is not found' do + expect(described_class.running?('123')).to be(false) + end + end + describe '.num_running', :clean_gitlab_redis_shared_state do it 'returns 0 if all jobs have been completed' do expect(described_class.num_running(%w(123))).to eq(0) diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 09d7d0dbbe6..73e038b61ed 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1462,7 +1462,7 @@ describe MergeRequest do describe '#merge_ongoing?' do 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(:num_running).with(['foo']) { 1 } + allow(Gitlab::SidekiqStatus).to receive(:running?).with('foo') { true } expect(merge_request.merge_ongoing?).to be(true) end @@ -1481,7 +1481,7 @@ describe MergeRequest do it 'returns false if there is no merge job running' do merge_request = build_stubbed(:merge_request, state: :open, merge_jid: 'foo') - allow(Gitlab::SidekiqStatus).to receive(:num_running).with(['foo']) { 0 } + allow(Gitlab::SidekiqStatus).to receive(:running?).with('foo') { false } expect(merge_request.merge_ongoing?).to be(false) end