From 885611a22ef40c939a46e11ced32469a93f44bd3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Wed, 10 Jun 2020 13:43:37 +0200
Subject: [PATCH] Introduce Feature Flag Definition
This adds a YAML-based definition of feature flags
that are stored in `(ee/)configs/feature-flags/`.
Currently none of feature flag types are required
to have a present YAML definition.
This definition contains information like:
- what MR introduced FF
- helps to create and has a issue that tracks
the rollout and removal of FF
- intentionally adds `default_enabled` to YAML
- force checks the consistency of `default_enabled`
between what is in code and what is in YAML
---
config/initializers/0_inject_feature_flags.rb | 5 +
config/initializers/flipper.rb | 1 -
ee/lib/ee/feature/definition.rb | 20 ++
lib/feature.rb | 14 +-
lib/feature/definition.rb | 137 ++++++++++++
lib/feature/shared.rb | 33 +++
spec/lib/feature/definition_spec.rb | 209 ++++++++++++++++++
spec/lib/feature_spec.rb | 30 +++
spec/spec_helper.rb | 3 +
9 files changed, 448 insertions(+), 4 deletions(-)
create mode 100644 config/initializers/0_inject_feature_flags.rb
delete mode 100644 config/initializers/flipper.rb
create mode 100644 ee/lib/ee/feature/definition.rb
create mode 100644 lib/feature/definition.rb
create mode 100644 lib/feature/shared.rb
create mode 100644 spec/lib/feature/definition_spec.rb
diff --git a/config/initializers/0_inject_feature_flags.rb b/config/initializers/0_inject_feature_flags.rb
new file mode 100644
index 0000000000000..45e6546e29439
--- /dev/null
+++ b/config/initializers/0_inject_feature_flags.rb
@@ -0,0 +1,5 @@
+# This needs to be loaded after
+# config/initializers/0_inject_enterprise_edition_module.rb
+
+Feature.register_feature_groups
+Feature.register_definitions
diff --git a/config/initializers/flipper.rb b/config/initializers/flipper.rb
deleted file mode 100644
index 80cab7273e5cc..0000000000000
--- a/config/initializers/flipper.rb
+++ /dev/null
@@ -1 +0,0 @@
-Feature.register_feature_groups
diff --git a/ee/lib/ee/feature/definition.rb b/ee/lib/ee/feature/definition.rb
new file mode 100644
index 0000000000000..8f0f7de93c30d
--- /dev/null
+++ b/ee/lib/ee/feature/definition.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+module EE
+ module Feature
+ module Definition
+ module ClassMethods
+ extend ::Gitlab::Utils::Override
+
+ override :paths
+ def paths
+ @ee_paths ||= [Rails.root.join('ee', 'config', 'feature_flags', '**', '*.yml')] + super
+ end
+ end
+
+ def self.prepended(base)
+ base.singleton_class.prepend ClassMethods
+ end
+ end
+ end
+end
diff --git a/lib/feature.rb b/lib/feature.rb
index 3115c97658d64..7cf40b63fdff3 100644
--- a/lib/feature.rb
+++ b/lib/feature.rb
@@ -54,12 +54,14 @@ def persisted_name?(feature_name)
# 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/30228
- def enabled?(key, thing = nil, default_enabled: false)
+ def enabled?(key, thing = nil, type: :development, default_enabled: false)
if check_feature_flags_definition?
if thing && !thing.respond_to?(:flipper_id)
raise InvalidFeatureFlagError,
"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: default_enabled)
end
# During setup the database does not exist yet. So we haven't stored a value
@@ -75,9 +77,9 @@ def enabled?(key, thing = nil, default_enabled: false)
!default_enabled || Feature.persisted_name?(feature.name) ? feature.enabled?(thing) : true
end
- def disabled?(key, thing = nil, default_enabled: false)
+ def disabled?(key, thing = nil, type: :development, default_enabled: false)
# we need to make different method calls to make it easy to mock / define expectations in test mode
- thing.nil? ? !enabled?(key, default_enabled: default_enabled) : !enabled?(key, thing, default_enabled: default_enabled)
+ thing.nil? ? !enabled?(key, type: type, default_enabled: default_enabled) : !enabled?(key, thing, type: type, default_enabled: default_enabled)
end
def enable(key, thing = true)
@@ -129,6 +131,12 @@ def reset
def register_feature_groups
end
+ def register_definitions
+ return unless check_feature_flags_definition?
+
+ Feature::Definition.load_all!
+ end
+
private
def flipper
diff --git a/lib/feature/definition.rb b/lib/feature/definition.rb
new file mode 100644
index 0000000000000..b0ea55c58051b
--- /dev/null
+++ b/lib/feature/definition.rb
@@ -0,0 +1,137 @@
+# frozen_string_literal: true
+
+class Feature
+ class Definition
+ include ::Feature::Shared
+
+ attr_reader :path
+ attr_reader :attributes
+
+ PARAMS.each do |param|
+ define_method(param) do
+ attributes[param]
+ end
+ end
+
+ def initialize(path, opts = {})
+ @path = path
+ @attributes = {}
+
+ # assign nil, for all unknown opts
+ PARAMS.each do |param|
+ @attributes[param] = opts[param]
+ end
+ end
+
+ def key
+ name.to_sym
+ end
+
+ def validate!
+ unless name.present?
+ raise Feature::InvalidFeatureFlagError, "Feature flag is missing name"
+ end
+
+ unless path.present?
+ raise Feature::InvalidFeatureFlagError, "Feature flag '#{name}' is missing path"
+ end
+
+ unless type.present?
+ raise Feature::InvalidFeatureFlagError, "Feature flag '#{name}' is missing type. Ensure to update #{path}"
+ end
+
+ unless Definition::TYPES.include?(type.to_sym)
+ raise Feature::InvalidFeatureFlagError, "Feature flag '#{name}' type '#{type}' is invalid. Ensure to update #{path}"
+ end
+
+ unless File.basename(path, ".yml") == name
+ raise Feature::InvalidFeatureFlagError, "Feature flag '#{name}' has an invalid path: '#{path}'. Ensure to update #{path}"
+ end
+
+ unless File.basename(File.dirname(path)) == type
+ raise Feature::InvalidFeatureFlagError, "Feature flag '#{name}' has an invalid type: '#{path}'. Ensure to update #{path}"
+ end
+
+ if default_enabled.nil?
+ raise Feature::InvalidFeatureFlagError, "Feature flag '#{name}' is missing default_enabled. Ensure to update #{path}"
+ end
+ end
+
+ def valid_usage!(type_in_code:, default_enabled_in_code:)
+ unless Array(type).include?(type_in_code.to_s)
+ # Raise exception in test and dev
+ raise Feature::InvalidFeatureFlagError, "The `type:` of `#{key}` is not equal to config: " \
+ "#{type_in_code} vs #{type}. Ensure to use valid type in #{path} or ensure that you use " \
+ "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)
+ # 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}"
+ end
+ end
+
+ def to_h
+ attributes
+ end
+
+ class << self
+ def paths
+ @paths ||= [Rails.root.join('config', 'feature_flags', '**', '*.yml')]
+ end
+
+ def definitions
+ @definitions ||= {}
+ end
+
+ def load_all!
+ definitions.clear
+
+ paths.each do |glob_path|
+ load_all_from_path!(glob_path)
+ end
+
+ definitions
+ end
+
+ def valid_usage!(key, type:, default_enabled:)
+ if definition = definitions[key.to_sym]
+ 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]
+ else
+ raise InvalidFeatureFlagError, "Unknown feature flag type used: `#{type}`"
+ end
+ end
+
+ private
+
+ def load_from_file(path)
+ definition = File.read(path)
+ definition = YAML.safe_load(definition)
+ definition.deep_symbolize_keys!
+
+ self.new(path, definition).tap(&:validate!)
+ rescue => e
+ raise Feature::InvalidFeatureFlagError, "Invalid definition for `#{path}`: #{e.message}"
+ end
+
+ def load_all_from_path!(glob_path)
+ Dir.glob(glob_path).each do |path|
+ definition = load_from_file(path)
+
+ if previous = definitions[definition.key]
+ raise InvalidFeatureFlagError, "Feature flag '#{definition.key}' is already defined in '#{previous.path}'"
+ end
+
+ definitions[definition.key] = definition
+ end
+ end
+ end
+ end
+end
+
+Feature::Definition.prepend_if_ee('EE::Feature::Definition')
diff --git a/lib/feature/shared.rb b/lib/feature/shared.rb
new file mode 100644
index 0000000000000..14efbb07100b2
--- /dev/null
+++ b/lib/feature/shared.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+# This file can contain only simple constructs as it is shared between:
+# 1. `Pure Ruby`: `bin/feature-flag`
+# 2. `GitLab Rails`: `lib/feature/definition.rb`
+
+class Feature
+ module Shared
+ # optional: defines if a on-disk definition is required for this feature flag type
+ # rollout_issue: defines if `bin/feature-flag` asks for rollout issue
+ # example: usage being shown when exception is raised
+ TYPES = {
+ development: {
+ description: 'Short lived, used to enable unfinished code to be deployed',
+ optional: true,
+ rollout_issue: true,
+ example: <<-EOS
+ Feature.enabled?(:my_feature_flag)
+ Feature.enabled?(:my_feature_flag, type: :development)
+ EOS
+ }
+ }.freeze
+
+ PARAMS = %i[
+ name
+ default_enabled
+ type
+ introduced_by_url
+ rollout_issue_url
+ group
+ ].freeze
+ end
+end
diff --git a/spec/lib/feature/definition_spec.rb b/spec/lib/feature/definition_spec.rb
new file mode 100644
index 0000000000000..49224cf42796a
--- /dev/null
+++ b/spec/lib/feature/definition_spec.rb
@@ -0,0 +1,209 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Feature::Definition do
+ let(:attributes) do
+ { name: 'feature_flag',
+ type: 'development',
+ default_enabled: true }
+ end
+
+ let(:path) { File.join('development', 'feature_flag.yml') }
+ let(:definition) { described_class.new(path, attributes) }
+ let(:yaml_content) { attributes.deep_stringify_keys.to_yaml }
+
+ describe '#key' do
+ subject { definition.key }
+
+ it 'returns a symbol from name' do
+ is_expected.to eq(:feature_flag)
+ end
+ end
+
+ describe '#validate!' do
+ using RSpec::Parameterized::TableSyntax
+
+ where(:param, :value, :result) do
+ :name | nil | /Feature flag is missing name/
+ :path | nil | /Feature flag 'feature_flag' is missing path/
+ :type | nil | /Feature flag 'feature_flag' is missing type/
+ :type | 'invalid' | /Feature flag 'feature_flag' type 'invalid' is invalid/
+ :path | 'development/invalid.yml' | /Feature flag 'feature_flag' has an invalid path/
+ :path | 'invalid/feature_flag.yml' | /Feature flag 'feature_flag' has an invalid type/
+ :default_enabled | nil | /Feature flag 'feature_flag' is missing default_enabled/
+ end
+
+ with_them do
+ let(:params) { attributes.merge(path: path) }
+
+ before do
+ params[param] = value
+ end
+
+ it do
+ expect do
+ described_class.new(
+ params[:path], params.except(:path)
+ ).validate!
+ end.to raise_error(result)
+ end
+ end
+ end
+
+ describe '#valid_usage!' do
+ context 'validates type' do
+ it 'raises exception for invalid type' do
+ expect { definition.valid_usage!(type_in_code: :invalid, default_enabled_in_code: false) }
+ .to raise_error(/The `type:` of `feature_flag` is not equal to config/)
+ end
+ end
+
+ context 'validates default enabled' do
+ it 'raises exception for different value' 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
+ end
+ end
+
+ describe '.paths' do
+ it 'returns at least one path' do
+ expect(described_class.paths).not_to be_empty
+ end
+ end
+
+ describe '.load_from_file' do
+ it 'properly loads a definition from file' do
+ expect(File).to receive(:read).with(path) { yaml_content }
+
+ expect(described_class.send(:load_from_file, path).attributes)
+ .to eq(definition.attributes)
+ end
+
+ context 'for missing file' do
+ let(:path) { 'missing/feature-flag/file.yml' }
+
+ it 'raises exception' do
+ expect do
+ described_class.send(:load_from_file, path)
+ end.to raise_error(/Invalid definition for/)
+ end
+ end
+
+ context 'for invalid definition' do
+ it 'raises exception' do
+ expect(File).to receive(:read).with(path) { '{}' }
+
+ expect do
+ described_class.send(:load_from_file, path)
+ end.to raise_error(/Feature flag is missing name/)
+ end
+ end
+ end
+
+ describe '.load_all!' do
+ let(:store1) { Dir.mktmpdir('path1') }
+ let(:store2) { Dir.mktmpdir('path2') }
+
+ before do
+ allow(described_class).to receive(:paths).and_return(
+ [
+ File.join(store1, '**', '*.yml'),
+ File.join(store2, '**', '*.yml')
+ ]
+ )
+ end
+
+ it "when there's no feature flags a list of definitions is empty" do
+ expect(described_class.load_all!).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
+ 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! }
+ .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! }
+ .to raise_error(/Feature flag is missing name/)
+ end
+
+ after do
+ FileUtils.rm_rf(store1)
+ FileUtils.rm_rf(store2)
+ end
+
+ def write_feature_flag(store, path, content)
+ path = File.join(store, path)
+ dir = File.dirname(path)
+ FileUtils.mkdir_p(dir)
+ File.write(path, content)
+ end
+ end
+
+ describe '.valid_usage!' do
+ before do
+ allow(described_class).to receive(:definitions) do
+ { definition.key => definition }
+ end
+ end
+
+ context 'when a known feature flag is used' do
+ it 'validates it usage' do
+ expect(definition).to receive(:valid_usage!)
+
+ described_class.valid_usage!(:feature_flag, type: :development, default_enabled: false)
+ end
+ end
+
+ context 'when an unknown feature flag is used' do
+ context 'for a type that is required to have all feature flags registered' do
+ before do
+ stub_const('Feature::Shared::TYPES', {
+ development: { optional: false }
+ })
+ end
+
+ it 'raises exception' do
+ expect do
+ described_class.valid_usage!(:unknown_feature_flag, type: :development, default_enabled: false)
+ end.to raise_error(/Missing feature definition for `unknown_feature_flag`/)
+ end
+ end
+
+ context 'for a type that is optional' do
+ before do
+ stub_const('Feature::Shared::TYPES', {
+ development: { optional: true }
+ })
+ end
+
+ it 'does not raise exception' do
+ expect do
+ described_class.valid_usage!(:unknown_feature_flag, type: :development, default_enabled: false)
+ end.not_to raise_error
+ end
+ end
+
+ context 'for an unknown type' do
+ it 'raises exception' do
+ expect do
+ described_class.valid_usage!(:unknown_feature_flag, type: :unknown_type, default_enabled: false)
+ end.to raise_error(/Unknown feature flag type used: `unknown_type`/)
+ end
+ end
+ end
+ end
+end
diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb
index 49443af6830fb..acd7d97ac854b 100644
--- a/spec/lib/feature_spec.rb
+++ b/spec/lib/feature_spec.rb
@@ -242,6 +242,36 @@
end
end
end
+
+ context 'validates usage of feature flag with YAML definition' do
+ let(:definition) do
+ Feature::Definition.new('development/my_feature_flag.yml',
+ name: 'my_feature_flag',
+ type: 'development',
+ default_enabled: false
+ ).tap(&:validate!)
+ end
+
+ before do
+ allow(Feature::Definition).to receive(:definitions) do
+ { definition.key => definition }
+ end
+ end
+
+ it 'when usage is correct' do
+ expect { described_class.enabled?(:my_feature_flag) }.not_to raise_error
+ end
+
+ it 'when invalid type is used' do
+ expect { described_class.enabled?(:my_feature_flag, type: :licensed) }
+ .to raise_error(/The `type:` of/)
+ end
+
+ 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
+ end
end
describe '.disable?' do
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index c43e96f2a1dae..ed3211a9c87a5 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -155,6 +155,9 @@
config.before(:suite) do
Timecop.safe_mode = true
TestEnv.init
+
+ # Reload all feature flags definitions
+ Feature.register_definitions
end
config.after(:all) do
--
GitLab