Merge branch 'read-feature-flag-default-enabled-from-yaml' into 'master'

Allow feature flag checks to read default_enabled value from YAML

See merge request gitlab-org/gitlab!47403

doc/development/feature_flags/development.md

@@ -178,6 +178,29 @@ if Feature.disabled?(:my_feature_flag, project, default_enabled: true)
 end
 ```
 
+If not specified, `default_enabled` is `false`.
+
+To force reading the `default_enabled` value from the relative YAML definition file, use
+`default_enabled: :yaml`:
+
+```ruby
+if Feature.enabled?(:feature_flag, project, default_enabled: :yaml)
+  # execute code if feature flag is enabled
+end
+```
+
+```ruby
+if Feature.disabled?(:feature_flag, project, default_enabled: :yaml)
+  # execute code if feature flag is disabled
+end
+```
+
+This allows to use the same feature flag check across various parts of the codebase and
+maintain the status of `default_enabled` in the YAML definition file which is the SSOT.
+
+If `default_enabled: :yaml` is used, a YAML definition is expected or an error is raised
+in development or test environment, while returning `false` on production.
+
 If not specified, the default feature flag type for `Feature.enabled?` and `Feature.disabled?`
 is `type: development`. For all other feature flag types, you must specify the `type:`:
 

lib/feature.rb

@@ -68,6 +68,9 @@ class Feature
         Feature::Definition.valid_usage!(key, type: type, default_enabled: default_enabled)
       end
 
+      # If `default_enabled: :yaml` we fetch the value from the YAML definition instead.
+      default_enabled = Feature::Definition.default_enabled?(key) if default_enabled == :yaml
+
       # During setup the database does not exist yet. So we haven't stored a value
       # for the feature yet and return the default.
       return default_enabled unless Gitlab::Database.exists?

lib/feature/definition.rb

@@ -71,9 +71,7 @@ class Feature
           "a valid syntax: #{TYPES.dig(type, :example)}"
       end
 
-      # We accept an array of defaults as some features are undefined
-      # and have `default_enabled: true/false`
-      unless Array(default_enabled).include?(default_enabled_in_code)
+      unless default_enabled_in_code == :yaml || default_enabled == default_enabled_in_code
         # Raise exception in test and dev
         raise Feature::InvalidFeatureFlagError, "The `default_enabled:` of `#{key}` is not equal to config: " \
           "#{default_enabled_in_code} vs #{default_enabled}. Ensure to update #{path}"
@@ -96,6 +94,10 @@ class Feature
         @definitions ||= load_all!
       end
 
+      def get(key)
+        definitions[key.to_sym]
+      end
+
       def reload!
         @definitions = load_all!
       end
@@ -105,7 +107,7 @@ class Feature
       end
 
       def valid_usage!(key, type:, default_enabled:)
-        if definition = definitions[key.to_sym]
+        if definition = get(key)
           definition.valid_usage!(type_in_code: type, default_enabled_in_code: default_enabled)
         elsif type_definition = self::TYPES[type]
           raise InvalidFeatureFlagError, "Missing feature definition for `#{key}`" unless type_definition[:optional]
@@ -114,6 +116,17 @@ class Feature
         end
       end
 
+      def default_enabled?(key)
+        if definition = get(key)
+          definition.default_enabled
+        else
+          Gitlab::ErrorTracking.track_and_raise_for_dev_exception(
+            InvalidFeatureFlagError.new("The feature flag YAML definition for '#{key}' does not exist"))
+
+          false
+        end
+      end
+
       def register_hot_reloader!
         # Reload feature flags on change of this file or any `.yml`
         file_watcher = Rails.configuration.file_watcher.new(reload_files, reload_directories) do

spec/lib/feature/definition_spec.rb

@@ -64,6 +64,11 @@ RSpec.describe Feature::Definition do
         expect { definition.valid_usage!(type_in_code: :development, default_enabled_in_code: false) }
           .to raise_error(/The `default_enabled:` of `feature_flag` is not equal to config/)
       end
+
+      it 'allows passing `default_enabled: :yaml`' do
+        expect { definition.valid_usage!(type_in_code: :development, default_enabled_in_code: :yaml) }
+          .not_to raise_error
+      end
     end
   end
 
@@ -209,4 +214,58 @@ RSpec.describe Feature::Definition do
       end
     end
   end
+
+  describe '.defaul_enabled?' do
+    subject { described_class.default_enabled?(key) }
+
+    context 'when feature flag exist' do
+      let(:key) { definition.key }
+
+      before do
+        allow(described_class).to receive(:definitions) do
+          { definition.key => definition }
+        end
+      end
+
+      context 'when default_enabled is true' do
+        it 'returns the value from the definition' do
+          expect(subject).to eq(true)
+        end
+      end
+
+      context 'when default_enabled is false' do
+        let(:attributes) do
+          { name: 'feature_flag',
+            type: 'development',
+            default_enabled: false }
+        end
+
+        it 'returns the value from the definition' do
+          expect(subject).to eq(false)
+        end
+      end
+    end
+
+    context 'when feature flag does not exist' do
+      let(:key) { :unknown_feature_flag }
+
+      context 'when on dev or test environment' do
+        it 'raises an error' do
+          expect { subject }.to raise_error(
+            Feature::InvalidFeatureFlagError,
+            "The feature flag YAML definition for 'unknown_feature_flag' does not exist")
+        end
+      end
+
+      context 'when on production environment' do
+        before do
+          allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(false)
+        end
+
+        it 'returns false' do
+          expect(subject).to eq(false)
+        end
+      end
+    end
+  end
 end

spec/lib/feature_spec.rb

@@ -249,10 +249,12 @@ RSpec.describe Feature, stub_feature_flags: false do
         Feature::Definition.new('development/my_feature_flag.yml',
           name: 'my_feature_flag',
           type: 'development',
-          default_enabled: false
+          default_enabled: default_enabled
         ).tap(&:validate!)
       end
 
+      let(:default_enabled) { false }
+
       before do
         stub_env('LAZILY_CREATE_FEATURE_FLAG', '0')
 
@@ -275,6 +277,63 @@ RSpec.describe Feature, stub_feature_flags: false do
         expect { described_class.enabled?(:my_feature_flag, default_enabled: true) }
           .to raise_error(/The `default_enabled:` of/)
       end
+
+      context 'when `default_enabled: :yaml` is used in code' do
+        it 'reads the default from the YAML definition' do
+          expect(described_class.enabled?(:my_feature_flag, default_enabled: :yaml)).to eq(false)
+        end
+
+        context 'when default_enabled is true in the YAML definition' do
+          let(:default_enabled) { true }
+
+          it 'reads the default from the YAML definition' do
+            expect(described_class.enabled?(:my_feature_flag, default_enabled: :yaml)).to eq(true)
+          end
+        end
+
+        context 'when YAML definition does not exist for an optional type' do
+          let(:optional_type) { described_class::Shared::TYPES.find { |name, attrs| attrs[:optional] }.first }
+
+          context 'when in dev or test environment' do
+            it 'raises an error for dev' do
+              expect { described_class.enabled?(:non_existent_flag, type: optional_type, default_enabled: :yaml) }
+                .to raise_error(
+                  Feature::InvalidFeatureFlagError,
+                  "The feature flag YAML definition for 'non_existent_flag' does not exist")
+            end
+          end
+
+          context 'when in production' do
+            before do
+              allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(false)
+            end
+
+            context 'when database exists' do
+              before do
+                allow(Gitlab::Database).to receive(:exists?).and_return(true)
+              end
+
+              it 'checks the persisted status and returns false' do
+                expect(described_class).to receive(:get).with(:non_existent_flag).and_call_original
+
+                expect(described_class.enabled?(:non_existent_flag, type: optional_type, default_enabled: :yaml)).to eq(false)
+              end
+            end
+
+            context 'when database does not exist' do
+              before do
+                allow(Gitlab::Database).to receive(:exists?).and_return(false)
+              end
+
+              it 'returns false without checking the status in the database' do
+                expect(described_class).not_to receive(:get)
+
+                expect(described_class.enabled?(:non_existent_flag, type: optional_type, default_enabled: :yaml)).to eq(false)
+              end
+            end
+          end
+        end
+      end
     end
   end
 

spec/support_specs/helpers/stub_feature_flags_spec.rb

@@ -5,19 +5,20 @@ require 'spec_helper'
 RSpec.describe StubFeatureFlags do
   let_it_be(:dummy_feature_flag) { :dummy_feature_flag }
 
-  # We inject dummy feature flag defintion
-  # to ensure that we strong validate it's usage
-  # as well
-  before(:all) do
-    definition = Feature::Definition.new(
+  let_it_be(:dummy_definition) do
+    Feature::Definition.new(
       nil,
       name: dummy_feature_flag,
       type: 'development',
-      # we allow ambigious usage of `default_enabled:`
-      default_enabled: [false, true]
+      default_enabled: false
     )
+  end
 
-    Feature::Definition.definitions[dummy_feature_flag] = definition
+  # We inject dummy feature flag defintion
+  # to ensure that we strong validate it's usage
+  # as well
+  before(:all) do
+    Feature::Definition.definitions[dummy_feature_flag] = dummy_definition
   end
 
   after(:all) do
@@ -47,6 +48,10 @@ RSpec.describe StubFeatureFlags do
         it { expect(Feature.disabled?(feature_name)).not_to eq(expected_result) }
 
         context 'default_enabled does not impact feature state' do
+          before do
+            allow(dummy_definition).to receive(:default_enabled).and_return(true)
+          end
+
           it { expect(Feature.enabled?(feature_name, default_enabled: true)).to eq(expected_result) }
           it { expect(Feature.disabled?(feature_name, default_enabled: true)).not_to eq(expected_result) }
         end
@@ -79,6 +84,10 @@ RSpec.describe StubFeatureFlags do
         it { expect(Feature.disabled?(feature_name, actor(tested_actor))).not_to eq(expected_result) }
 
         context 'default_enabled does not impact feature state' do
+          before do
+            allow(dummy_definition).to receive(:default_enabled).and_return(true)
+          end
+
           it { expect(Feature.enabled?(feature_name, actor(tested_actor), default_enabled: true)).to eq(expected_result) }
           it { expect(Feature.disabled?(feature_name, actor(tested_actor), default_enabled: true)).not_to eq(expected_result) }
         end