diff options
author | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2019-06-12 15:32:29 +0200 |
---|---|---|
committer | Zeger-Jan van de Weg <git@zjvandeweg.nl> | 2019-06-18 13:18:18 +0200 |
commit | 968674e41798c437b9ebf4a9731fe2f2a4f07024 (patch) | |
tree | 0fdc959be0443bff849cf704ed762902ff265ef3 | |
parent | 28f7846c4723457c6d53808215796515396eaa7d (diff) | |
download | gitlab-ce-968674e41798c437b9ebf4a9731fe2f2a4f07024.tar.gz |
Move Gitaly feature flag logic to Feature::Gitaly
The GitalyClient held a lot of logic which was all very tightly coupled.
In this instance the feature logic was extracted to make it do just a
little less and create a bit more focus in the GitalyClient's
responsibilies.
-rw-r--r-- | lib/feature/gitaly.rb | 25 | ||||
-rw-r--r-- | lib/gitlab/gitaly_client.rb | 21 | ||||
-rw-r--r-- | spec/lib/feature/gitaly_spec.rb | 32 | ||||
-rw-r--r-- | spec/lib/gitlab/gitaly_client_spec.rb | 14 |
4 files changed, 60 insertions, 32 deletions
diff --git a/lib/feature/gitaly.rb b/lib/feature/gitaly.rb new file mode 100644 index 00000000000..33868e49f14 --- /dev/null +++ b/lib/feature/gitaly.rb @@ -0,0 +1,25 @@ +class Feature + class Gitaly + CATFILE_CACHE = 'catfile-cache'.freeze + + # Server feature flags should use '_' to separate words. + SERVER_FEATURE_FLAGS = [CATFILE_CACHE].freeze + + class << self + def enabled?(feature_flag) + Feature::FlipperFeature.table_exists? && Feature.enabled?("gitaly_#{feature_flag}") + rescue ActiveRecord::NoDatabaseError + false + end + + def server_feature_flags + @server_feature_flags ||= + begin + SERVER_FEATURE_FLAGS.map do |f| + ["gitaly-feature-#{f.tr('_', '-')}", enabled?(f).to_s] + end.to_h + end + end + end + end +end diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index e683d4e5bbe..f3b0e65400b 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -31,10 +31,6 @@ module Gitlab MAXIMUM_GITALY_CALLS = 30 CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze - SERVER_FEATURE_CATFILE_CACHE = 'catfile-cache'.freeze - # Server feature flags should use '_' to separate words. - SERVER_FEATURE_FLAGS = [SERVER_FEATURE_CATFILE_CACHE].freeze - MUTEX = Mutex.new define_histogram :gitaly_controller_action_duration_seconds do @@ -223,9 +219,9 @@ module Gitlab metadata['call_site'] = feature.to_s if feature metadata['gitaly-servers'] = address_metadata(remote_storage) if remote_storage metadata['x-gitlab-correlation-id'] = Labkit::Correlation::CorrelationId.current_id if Labkit::Correlation::CorrelationId.current_id - metadata['gitaly-session-id'] = session_id if feature_enabled?(SERVER_FEATURE_CATFILE_CACHE) + metadata['gitaly-session-id'] = session_id if Feature::Gitaly.enabled?(Feature::Gitaly::CATFILE_CACHE) - metadata.merge!(server_feature_flags) + metadata.merge!(Feature::Gitaly.server_feature_flags) result = { metadata: metadata } @@ -244,11 +240,6 @@ module Gitlab Gitlab::SafeRequestStore[:gitaly_session_id] ||= SecureRandom.uuid end - def self.server_feature_flags - SERVER_FEATURE_FLAGS.map do |f| - ["gitaly-feature-#{f.tr('_', '-')}", feature_enabled?(f).to_s] - end.to_h - end def self.token(storage) params = Gitlab.config.repositories.storages[storage] @@ -257,12 +248,6 @@ module Gitlab params['gitaly_token'].presence || Gitlab.config.gitaly['token'] end - def self.feature_enabled?(feature_name) - Feature::FlipperFeature.table_exists? && Feature.enabled?("gitaly_#{feature_name}") - rescue ActiveRecord::NoDatabaseError - false - end - # Ensures that Gitaly is not being abuse through n+1 misuse etc def self.enforce_gitaly_request_limits(call_site) # Only count limits in request-response environments (not sidekiq for example) @@ -295,7 +280,7 @@ module Gitlab # check if the limit is being exceeded while testing in those environments # In that case we can use a feature flag to indicate that we do want to # enforce request limits. - return true if feature_enabled?('enforce_requests_limits') + return true if Feature::Gitaly.enabled?('enforce_requests_limits') !(Rails.env.production? || ENV["GITALY_DISABLE_REQUEST_LIMITS"]) end diff --git a/spec/lib/feature/gitaly_spec.rb b/spec/lib/feature/gitaly_spec.rb new file mode 100644 index 00000000000..12923e4ec41 --- /dev/null +++ b/spec/lib/feature/gitaly_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe Feature::Gitaly do + let(:feature_flag) { "mepmep" } + + describe ".enabled?" do + context 'when the gate is closed' do + before do + allow(Feature).to receive(:enabled?).with("gitaly_mepmep").and_return(false) + end + + it 'returns false' do + expect(described_class.enabled?(feature_flag)).to be(false) + end + end + end + + describe ".server_feature_flags" do + before do + stub_const("#{described_class}::SERVER_FEATURE_FLAGS", [feature_flag]) + allow(Feature).to receive(:enabled?).with("gitaly_mepmep").and_return(false) + end + + subject { described_class.server_feature_flags } + + it { is_expected.to be_a(Hash) } + + context 'when one flag is disabled' do + it { is_expected.to eq("gitaly-feature-mepmep" => "false") } + end + end +end diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb index da1eb0c2618..eed233f1f3e 100644 --- a/spec/lib/gitlab/gitaly_client_spec.rb +++ b/spec/lib/gitlab/gitaly_client_spec.rb @@ -330,20 +330,6 @@ describe Gitlab::GitalyClient do end end - describe 'feature_enabled?' do - let(:feature_name) { 'my_feature' } - let(:real_feature_name) { "gitaly_#{feature_name}" } - - before do - allow(Feature).to receive(:enabled?).and_return(false) - end - - it 'returns false' do - expect(Feature).to receive(:enabled?).with(real_feature_name) - expect(described_class.feature_enabled?(feature_name)).to be(false) - end - end - describe 'timeouts' do context 'with default values' do before do |