Merge branch 'zj-feature-flag-gitaly' into 'master'

Automatically detect Gitaly feature flags

See merge request gitlab-org/gitlab!24679

doc/development/gitaly.md

@@ -298,27 +298,12 @@ Here are the steps to gate a new feature in Gitaly behind a feature flag.
 
 ### GitLab Rails
 
-1. In GitLab Rails:
-
-   1. Add the feature flag to `SERVER_FEATURE_FLAGS` in `lib/feature/gitaly.rb`:
-
-      ```ruby
-      SERVER_FEATURE_FLAGS = %w[go-find-all-tags].freeze
-      ```
-
-   1. Search for `["gitaly"]["features"]` (currently in `spec/requests/api/internal/base_spec.rb`)
-      and fix the expected results for the tests by adding the new feature flag into it:
-
-      ```ruby
-      expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-get-all-lfs-pointers-go' => 'true', 'gitaly-feature-go-find-all-tags' => 'true')
-      ```
-
 1. Test in a Rails console by setting the feature flag:
 
    NOTE: **Note:**
    Pay attention to the name of the flag and the one used in the Rails console.
    There is a difference between them (dashes replaced by underscores and name
-   prefix is changed).
+   prefix is changed). Make sure to prefix all flags with `gitaly_`.
 
    ```ruby
    Feature.enable('gitaly_go_find_all_tags')

lib/feature.rb

@@ -32,6 +32,8 @@ class Feature
     end
 
     def persisted_names
+      return [] unless Gitlab::Database.exists?
+
       Gitlab::SafeRequestStore[:flipper_persisted_names] ||=
         begin
           # We saw on GitLab.com, this database request was called 2300

lib/feature/gitaly.rb

 # frozen_string_literal: true
 
-require 'set'
-
 class Feature
   class Gitaly
-    # Server feature flags should use '_' to separate words.
-    SERVER_FEATURE_FLAGS =
-      %w[
-        cache_invalidator
-        inforef_uploadpack_cache
-        commit_without_batch_check
-        use_core_delta_islands
-        use_git_protocol_v2
-      ].freeze
-
-    DEFAULT_ON_FLAGS = Set.new([]).freeze
+    PREFIX = "gitaly_"
 
     class << self
       def enabled?(feature_flag)
         return false unless Feature::FlipperFeature.table_exists?
 
-        default_on = DEFAULT_ON_FLAGS.include?(feature_flag)
-        Feature.enabled?("gitaly_#{feature_flag}", default_enabled: default_on)
+        Feature.enabled?("#{PREFIX}#{feature_flag}")
       rescue ActiveRecord::NoDatabaseError, PG::ConnectionBad
         false
       end
 
       def server_feature_flags
-        SERVER_FEATURE_FLAGS.map do |f|
-          ["gitaly-feature-#{f.tr('_', '-')}", enabled?(f).to_s]
+        Feature.persisted_names
+          .select { |f| f.start_with?(PREFIX) }
+          .map do |f|
+          flag = f.delete_prefix(PREFIX)
+
+          ["gitaly-feature-#{flag.tr('_', '-')}", enabled?(flag).to_s]
         end.to_h
       end
     end

spec/lib/feature/gitaly_spec.rb

@@ -5,10 +5,6 @@ require 'spec_helper'
 describe Feature::Gitaly do
   let(:feature_flag) { "mep_mep" }
 
-  before do
-    stub_const("#{described_class}::SERVER_FEATURE_FLAGS", [feature_flag])
-  end
-
   describe ".enabled?" do
     context 'when the gate is closed' do
       before do
@@ -28,15 +24,13 @@ describe Feature::Gitaly do
   end
 
   describe ".server_feature_flags" do
-    context 'when one flag is disabled' do
-      before do
-        stub_feature_flags(gitaly_mep_mep: false)
-      end
+    before do
+      allow(Feature).to receive(:persisted_names).and_return(%w[gitaly_mep_mep foo])
+    end
 
-      subject { described_class.server_feature_flags }
+    subject { described_class.server_feature_flags }
 
-      it { is_expected.to be_a(Hash) }
-      it { is_expected.to eq("gitaly-feature-mep-mep" => "false") }
-    end
+    it { is_expected.to be_a(Hash) }
+    it { is_expected.to eq("gitaly-feature-mep-mep" => "true") }
   end
 end

spec/requests/api/internal/base_spec.rb

@@ -313,6 +313,10 @@ describe API::Internal::Base do
       end
 
       context "git pull" do
+        before do
+          allow(Feature).to receive(:persisted_names).and_return(%w[gitaly_mep_mep])
+        end
+
         it "has the correct payload" do
           pull(key, project)
 
@@ -326,7 +330,7 @@ describe API::Internal::Base do
           expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
           expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
           expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true', 'gitaly-feature-use-core-delta-islands' => 'true', 'gitaly-feature-use-git-protocol-v2' => 'true')
+          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-mep-mep' => 'true')
           expect(user.reload.last_activity_on).to eql(Date.today)
         end
       end
@@ -346,7 +350,6 @@ describe API::Internal::Base do
             expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
             expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
             expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-            expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true', 'gitaly-feature-use-core-delta-islands' => 'true', 'gitaly-feature-use-git-protocol-v2' => 'true')
             expect(user.reload.last_activity_on).to be_nil
           end
         end
@@ -594,7 +597,6 @@ describe API::Internal::Base do
           expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
           expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
           expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-inforef-uploadpack-cache' => 'true', 'gitaly-feature-cache-invalidator' => 'true', 'gitaly-feature-commit-without-batch-check' => 'true', 'gitaly-feature-use-core-delta-islands' => 'true', 'gitaly-feature-use-git-protocol-v2' => 'true')
         end
       end