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.

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