Merge branch 'revert-0767b9e2' into 'master'

Revert "Merge branch 'ck3g-drop-default_enabled-from-Feature-enabled' into 'master'" [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57707

changelogs/unreleased/revert-0767b9e2.yml

+---
+title: Revert Ignore default_enabled value in Feature.enabled?
+merge_request: 57707
+author:
+type: fixed

config/feature_flags/ops/prometheus_metrics_method_instrumentation.yml

----
-name: prometheus_metrics_method_instrumentation
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/4304
-rollout_issue_url:
-milestone: '10.5'
-type: ops
-group:
-default_enabled: false

config/feature_flags/ops/prometheus_metrics_view_instrumentation.yml

----
-name: prometheus_metrics_view_instrumentation
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/4304
-rollout_issue_url:
-milestone: '10.5'
-type: ops
-group:
-default_enabled: false

lib/feature.rb

@@ -57,7 +57,7 @@ class Feature
     # use `default_enabled: true` to default the flag to being `enabled`
     # unless set explicitly.  The default is `disabled`
     # TODO: remove the `default_enabled:` and read it from the `defintion_yaml`
-    # check: https://gitlab.com/gitlab-org/gitlab/-/issues/271275
+    # check: https://gitlab.com/gitlab-org/gitlab/-/issues/30228
     def enabled?(key, thing = nil, type: :development, default_enabled: false)
       if check_feature_flags_definition?
         if thing && !thing.respond_to?(:flipper_id)
@@ -65,11 +65,11 @@ class Feature
             "The thing '#{thing.class.name}' for feature flag '#{key}' needs to include `FeatureGate` or implement `flipper_id`"
         end
 
-        Feature::Definition.valid_usage!(key, type: type, default_enabled: :yaml)
+        Feature::Definition.valid_usage!(key, type: type, default_enabled: default_enabled)
       end
 
-      # TODO: Remove rubocop disable comment once `default_enabled` argument is removed https://gitlab.com/gitlab-org/gitlab/-/issues/271275
-      default_enabled = Feature::Definition.default_enabled?(key) # rubocop:disable Lint/ShadowedArgument
+      # 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.

spec/graphql/features/feature_flag_spec.rb

@@ -15,7 +15,6 @@ RSpec.describe 'Graphql Field feature flags' do
 
   before do
     skip_feature_flags_yaml_validation
-    skip_default_enabled_yaml_check
   end
 
   subject { result }

spec/graphql/types/base_field_spec.rb

@@ -128,7 +128,6 @@ RSpec.describe Types::BaseField do
 
         before do
           skip_feature_flags_yaml_validation
-          skip_default_enabled_yaml_check
         end
 
         it 'returns false if the feature is not enabled' do

spec/lib/api/helpers_spec.rb

@@ -183,7 +183,6 @@ RSpec.describe API::Helpers do
 
     before do
       skip_feature_flags_yaml_validation
-      skip_default_enabled_yaml_check
     end
 
     context 'with feature enabled' do

spec/lib/feature/gitaly_spec.rb

@@ -8,7 +8,6 @@ RSpec.describe Feature::Gitaly do
 
   before do
     skip_feature_flags_yaml_validation
-    skip_default_enabled_yaml_check
   end
 
   describe ".enabled?" do

spec/lib/feature_spec.rb

@@ -123,35 +123,12 @@ RSpec.describe Feature, stub_feature_flags: false do
   end
 
   describe '.enabled?' do
-    let(:disabled_ff_definition) do
-      Feature::Definition.new(
-        'development/disabled_feature_flag.yml',
-        name: 'disabled_feature_flag',
-        type: 'development',
-        default_enabled: false
-      )
+    it 'returns false for undefined feature' do
+      expect(described_class.enabled?(:some_random_feature_flag)).to be_falsey
     end
 
-    let(:enabled_ff_definition) do
-      Feature::Definition.new(
-        'development/enabled_feature_flag.yml',
-        name: 'enabled_feature_flag',
-        type: 'development',
-        default_enabled: true
-      )
-    end
-
-    before do
-      allow(Feature::Definition).to receive(:definitions) do
-        {
-          disabled_ff_definition.key => disabled_ff_definition,
-          enabled_ff_definition.key => enabled_ff_definition
-        }
-      end
-    end
-
-    it 'raises an exception for undefined feature' do
-      expect { described_class.enabled?(:some_random_feature_flag) }.to raise_error Feature::InvalidFeatureFlagError
+    it 'returns true for undefined feature with default_enabled' do
+      expect(described_class.enabled?(:some_random_feature_flag, default_enabled: true)).to be_truthy
     end
 
     it 'returns false for existing disabled feature in the database' do
@@ -169,58 +146,39 @@ RSpec.describe Feature, stub_feature_flags: false do
     it { expect(described_class.send(:l1_cache_backend)).to eq(Gitlab::ProcessMemoryCache.cache_backend) }
     it { expect(described_class.send(:l2_cache_backend)).to eq(Rails.cache) }
 
-    it 'caches the status in L1 and L2 caches', :request_store, :use_clean_rails_memory_store_caching, :aggregate_failures do
+    it 'caches the status in L1 and L2 caches',
+       :request_store, :use_clean_rails_memory_store_caching do
       described_class.enable(:enabled_feature_flag)
-      flipper_features_key = 'flipper/v1/features'
-      flipper_feature_key = 'flipper/v1/feature/enabled_feature_flag'
+      flipper_key = "flipper/v1/feature/enabled_feature_flag"
 
-      allow(described_class.send(:l2_cache_backend)).to receive(:fetch).twice.and_call_original
-      allow(described_class.send(:l1_cache_backend)).to receive(:fetch).twice.and_call_original
+      expect(described_class.send(:l2_cache_backend))
+        .to receive(:fetch)
+        .once
+        .with(flipper_key, expires_in: 1.hour)
+        .and_call_original
+
+      expect(described_class.send(:l1_cache_backend))
+        .to receive(:fetch)
+        .once
+        .with(flipper_key, expires_in: 1.minute)
+        .and_call_original
 
       2.times do
         expect(described_class.enabled?(:enabled_feature_flag)).to be_truthy
       end
-
-      expect(described_class.send(:l2_cache_backend)).to have_received(:fetch).with(flipper_features_key, expires_in: 1.hour)
-      expect(described_class.send(:l2_cache_backend)).to have_received(:fetch).with(flipper_feature_key, expires_in: 1.hour)
-
-      expect(described_class.send(:l1_cache_backend)).to have_received(:fetch).with(flipper_features_key, expires_in: 1.minute)
-      expect(described_class.send(:l1_cache_backend)).to have_received(:fetch).with(flipper_feature_key, expires_in: 1.minute)
     end
 
-    it 'returns the default value when the database does not exist', :aggregate_falures do
-      a_feature = Feature::Definition.new(
-        'development/a_feature.yml',
-        name: 'a_feature',
-        type: 'development',
-        default_enabled: true
-      )
-
-      allow(Feature::Definition).to receive(:definitions) do
-        { a_feature.key => a_feature }
-      end
-
+    it 'returns the default value when the database does not exist' do
+      fake_default = double('fake default')
       expect(ActiveRecord::Base).to receive(:connection) { raise ActiveRecord::NoDatabaseError, "No database" }
 
-      expect(described_class.enabled?(:a_feature)).to eq(true)
+      expect(described_class.enabled?(:a_feature, default_enabled: fake_default)).to eq(fake_default)
     end
 
-    context 'cached feature flag', :request_store, :use_clean_rails_memory_store_caching, :aggregate_failures do
+    context 'cached feature flag', :request_store do
       let(:flag) { :some_feature_flag }
-      let(:some_feature_flag) do
-        Feature::Definition.new(
-          "development/#{flag}.yml",
-          name: flag.to_s,
-          type: 'development',
-          default_enabled: true
-        )
-      end
 
       before do
-        allow(Feature::Definition).to receive(:definitions) do
-          { some_feature_flag.key => some_feature_flag }
-        end
-
         described_class.send(:flipper).memoize = false
         described_class.enabled?(flag)
       end
@@ -315,48 +273,63 @@ RSpec.describe Feature, stub_feature_flags: false do
           .to raise_error(/The `type:` of/)
       end
 
-      it 'reads the default from the YAML definition' do
-        expect(described_class.enabled?(:my_feature_flag)).to eq(false)
+      it 'when invalid default_enabled is used' do
+        expect { described_class.enabled?(:my_feature_flag, default_enabled: true) }
+          .to raise_error(/The `default_enabled:` of/)
       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 `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 in dev or test environment' do
-          it 'raises an error for dev' do
-            expect { described_class.enabled?(:non_existent_flag, type: optional_type) }
-              .to raise_error(
-                Feature::InvalidFeatureFlagError,
-                "The feature flag YAML definition for 'non_existent_flag' does not exist")
+        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 in production' do
-          before do
-            allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(false)
+        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 database exists' do
+          context 'when in production' do
             before do
-              allow(Gitlab::Database).to receive(:exists?).and_return(true)
+              allow(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(false)
             end
 
-            it 'checks the persisted status and returns false' do
-              expect(described_class).to receive(:get).with(:non_existent_flag).and_call_original
+            context 'when database exists' do
+              before do
+                allow(Gitlab::Database).to receive(:exists?).and_return(true)
+              end
 
-              expect(described_class.enabled?(:non_existent_flag, type: optional_type)).to eq(false)
-            end
-          end
+              it 'checks the persisted status and returns false' do
+                expect(described_class).to receive(:get).with(:non_existent_flag).and_call_original
 
-          context 'when database does not exist' do
-            before do
-              allow(Gitlab::Database).to receive(:exists?).and_return(false)
+                expect(described_class.enabled?(:non_existent_flag, type: optional_type, default_enabled: :yaml)).to eq(false)
+              end
             end
 
-            it 'returns false without checking the status in the database' do
-              expect(described_class).not_to receive(:get)
+            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)).to eq(false)
+                expect(described_class.enabled?(:non_existent_flag, type: optional_type, default_enabled: :yaml)).to eq(false)
+              end
             end
           end
         end
@@ -364,36 +337,13 @@ RSpec.describe Feature, stub_feature_flags: false do
     end
   end
 
-  describe '.disabled?' do
-    let(:disabled_ff_definition) do
-      Feature::Definition.new(
-        'development/disabled_feature_flag.yml',
-        name: 'disabled_feature_flag',
-        type: 'development',
-        default_enabled: false
-      )
-    end
-
-    let(:enabled_ff_definition) do
-      Feature::Definition.new(
-        'development/enabled_feature_flag.yml',
-        name: 'enabled_feature_flag',
-        type: 'development',
-        default_enabled: true
-      )
-    end
-
-    before do
-      allow(Feature::Definition).to receive(:definitions) do
-        {
-          disabled_ff_definition.key => disabled_ff_definition,
-          enabled_ff_definition.key => enabled_ff_definition
-        }
-      end
+  describe '.disable?' do
+    it 'returns true for undefined feature' do
+      expect(described_class.disabled?(:some_random_feature_flag)).to be_truthy
     end
 
-    it 'raises an exception for undefined feature' do
-      expect { described_class.disabled?(:some_random_feature_flag) }.to raise_error Feature::InvalidFeatureFlagError
+    it 'returns false for undefined feature with default_enabled' do
+      expect(described_class.disabled?(:some_random_feature_flag, default_enabled: true)).to be_falsey
     end
 
     it 'returns true for existing disabled feature in the database' do

spec/lib/gitlab/gon_helper_spec.rb

@@ -12,7 +12,6 @@ RSpec.describe Gitlab::GonHelper do
   describe '#push_frontend_feature_flag' do
     before do
       skip_feature_flags_yaml_validation
-      skip_default_enabled_yaml_check
     end
 
     it 'pushes a feature flag to the frontend' do

spec/lib/gitlab/metrics/methods_spec.rb

@@ -102,11 +102,6 @@ RSpec.describe Gitlab::Metrics::Methods do
         let(:feature_name) { :some_metric_feature }
         let(:metric) { call_fetch_metric_method(docstring: docstring, with_feature: feature_name) }
 
-        before do
-          skip_feature_flags_yaml_validation
-          skip_default_enabled_yaml_check
-        end
-
         context 'when feature is enabled' do
           before do
             stub_feature_flags(feature_name => true)

spec/lib/gitlab/metrics_spec.rb

@@ -54,11 +54,6 @@ RSpec.describe Gitlab::Metrics do
   end
 
   describe '.measure' do
-    before do
-      skip_feature_flags_yaml_validation
-      skip_default_enabled_yaml_check
-    end
-
     context 'without a transaction' do
       it 'returns the return value of the block' do
         val = described_class.measure(:foo) { 10 }

spec/requests/api/features_spec.rb

@@ -438,8 +438,6 @@ RSpec.describe API::Features, stub_feature_flags: false do
 
       context 'when the gate value was set' do
         before do
-          skip_feature_flags_yaml_validation
-          skip_default_enabled_yaml_check
           Feature.enable(feature_name)
         end
 

spec/requests/api/usage_data_spec.rb

@@ -73,7 +73,6 @@ RSpec.describe API::UsageData do
       context 'with unknown event' do
         before do
           skip_feature_flags_yaml_validation
-          skip_default_enabled_yaml_check
         end
 
         it 'returns status ok' do
@@ -150,7 +149,6 @@ RSpec.describe API::UsageData do
       context 'with unknown event' do
         before do
           skip_feature_flags_yaml_validation
-          skip_default_enabled_yaml_check
         end
 
         it 'returns status ok' do