Merge branch 'fix-hot-reloading-of-ff' into 'master'

Fix `hot-reloading` of feature flag definitions

See merge request gitlab-org/gitlab!42876

config/feature_flags/ops/gitlab_sidekiq_enable_semi_reliable_fetcher.yml

+---
+name: gitlab_sidekiq_enable_semi_reliable_fetcher
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: ops
+default_enabled: true

config/feature_flags/ops/gitlab_sidekiq_reliable_fetcher.yml

+---
+name: gitlab_sidekiq_reliable_fetcher
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: ops
+default_enabled: true

config/initializers/sidekiq.rb

@@ -3,13 +3,13 @@
 def enable_reliable_fetch?
   return true unless Feature::FlipperFeature.table_exists?
 
-  Feature.enabled?(:gitlab_sidekiq_reliable_fetcher, default_enabled: true)
+  Feature.enabled?(:gitlab_sidekiq_reliable_fetcher, type: :ops, default_enabled: true)
 end
 
 def enable_semi_reliable_fetch_mode?
   return true unless Feature::FlipperFeature.table_exists?
 
-  Feature.enabled?(:gitlab_sidekiq_enable_semi_reliable_fetcher, default_enabled: true)
+  Feature.enabled?(:gitlab_sidekiq_enable_semi_reliable_fetcher, type: :ops, default_enabled: true)
 end
 
 # Custom Queues configuration

lib/feature.rb

@@ -138,7 +138,7 @@ class Feature
     def register_definitions
       return unless check_feature_flags_definition?
 
-      Feature::Definition.load_all!
+      Feature::Definition.reload!
     end
 
     def register_hot_reloader

lib/feature/definition.rb

@@ -84,17 +84,14 @@ class Feature
       end
 
       def definitions
-        @definitions ||= {}
+        # We lazily load all definitions
+        # The hot reloading might request a feature flag
+        # before we can properly call `load_all!`
+        @definitions ||= load_all!
       end
 
-      def load_all!
-        definitions.clear
-
-        paths.each do |glob_path|
-          load_all_from_path!(glob_path)
-        end
-
-        definitions
+      def reload!
+        @definitions = load_all!
       end
 
       def valid_usage!(key, type:, default_enabled:)
@@ -110,9 +107,7 @@ class Feature
       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
-          # We use `Feature::Definition` as on Ruby code-reload
-          # a new class definition is created
-          Feature::Definition.load_all!
+          Feature::Definition.reload!
         end
 
         Rails.application.reloaders << file_watcher
@@ -123,6 +118,16 @@ class Feature
 
       private
 
+      def load_all!
+        # We currently do not load feature flag definitions
+        # in production environments
+        return [] unless Gitlab.dev_or_test_env?
+
+        paths.each_with_object({}) do |glob_path, definitions|
+          load_all_from_path!(definitions, glob_path)
+        end
+      end
+
       def load_from_file(path)
         definition = File.read(path)
         definition = YAML.safe_load(definition)
@@ -133,7 +138,7 @@ class Feature
         raise Feature::InvalidFeatureFlagError, "Invalid definition for `#{path}`: #{e.message}"
       end
 
-      def load_all_from_path!(glob_path)
+      def load_all_from_path!(definitions, glob_path)
         Dir.glob(glob_path).each do |path|
           definition = load_from_file(path)
 
@@ -146,7 +151,7 @@ class Feature
       end
 
       def reload_files
-        [File.expand_path(__FILE__)]
+        []
       end
 
       def reload_directories

spec/lib/feature/definition_spec.rb

@@ -114,34 +114,32 @@ RSpec.describe Feature::Definition do
           File.join(store2, '**', '*.yml')
         ]
       )
-
-      # We stub `definitions` to ensure that they
-      # are not overwritten by `.load_all!`
-      allow(described_class).to receive(:definitions).and_return(definitions)
     end
 
+    subject { described_class.send(:load_all!) }
+
     it "when there's no feature flags a list of definitions is empty" do
-      expect(described_class.load_all!).to be_empty
+      is_expected.to be_empty
     end
 
     it "when there's a single feature flag it properly loads them" do
       write_feature_flag(store1, path, yaml_content)
 
-      expect(described_class.load_all!).to be_one
+      is_expected.to be_one
     end
 
     it "when the same feature flag is stored multiple times raises exception" do
       write_feature_flag(store1, path, yaml_content)
       write_feature_flag(store2, path, yaml_content)
 
-      expect { described_class.load_all! }
+      expect { subject }
         .to raise_error(/Feature flag 'feature_flag' is already defined/)
     end
 
     it "when one of the YAMLs is invalid it does raise exception" do
       write_feature_flag(store1, path, '{}')
 
-      expect { described_class.load_all! }
+      expect { subject }
         .to raise_error(/Feature flag is missing name/)
     end