Commit 5a94623a authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'expose-only-feature-not-flipper' into 'master'

Abstract `Feature Flags` usage patterns

See merge request gitlab-org/gitlab!33178
parents 462ad4e9 0ec832ec
......@@ -274,6 +274,9 @@ Cop/ActiveRecordAssociationReload:
- 'spec/**/*'
- 'ee/spec/**/*'
Gitlab/AvoidFeatureGet:
Enabled: true
Naming/PredicateName:
Enabled: true
Exclude:
......
......@@ -2,7 +2,7 @@
module AutoDevopsHelper
def show_auto_devops_callout?(project)
Feature.get(:auto_devops_banner_disabled).off? &&
Feature.disabled?(:auto_devops_banner_disabled) &&
show_callout?('auto_devops_settings_dismissed') &&
can?(current_user, :admin_pipeline, project) &&
project.has_auto_devops_implicitly_disabled? &&
......
......@@ -357,7 +357,7 @@ Feature.enabled?(:my_feature2) # => false
Feature.enabled?(:my_feature2, project1) # => true
```
#### `stub_feature_flags` vs `Feature.get/enable`
#### `stub_feature_flags` vs `Feature.enable*`
It is preferred to use `stub_feature_flags` for enabling feature flags
in testing environment. This method provides a simple and well described
......@@ -378,10 +378,10 @@ stub_feature_flags(my_feature: [project, project2])
Feature.enable(:my_feature_2)
# Good: enable my_feature for 50% of time
Feature.get(:my_feature_3).enable_percentage_of_time(50)
Feature.enable_percentage_of_time(:my_feature_3, 50)
# Good: enable my_feature for 50% of actors/gates/things
Feature.get(:my_feature_4).enable_percentage_of_actors(50)
Feature.enable_percentage_of_actors(:my_feature_4, 50)
```
Each feature flag that has a defined state will be persisted
......
......@@ -598,7 +598,7 @@ The banner can be disabled for:
- By an administrator running the following in a Rails console:
```ruby
Feature.get(:auto_devops_banner_disabled).enable
Feature.enable(:auto_devops_banner_disabled)
```
- Through the REST API with an admin access token:
......
......@@ -346,7 +346,7 @@ of projects. From the console, enter the following command, replacing `10` with
your desired percentage:
```ruby
Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(10)
Feature.enable_percentage_of_actors(:force_autodevops_on_by_default, 10)
```
### Deployment strategy
......
......@@ -337,7 +337,7 @@ module EE
filter = user_preference.feature_filter_type.presence || 0
# We use a 2nd feature flag for control as enabled and percentage_of_time for chatops
flipper_feature = ::Feature.get((feature.to_s + '_control').to_sym)
flipper_feature = ::Feature.get((feature.to_s + '_control').to_sym) # rubocop:disable Gitlab/AvoidFeatureGet
percentage ||= flipper_feature.gate_values[:percentage_of_time] || 0 if flipper_feature
return false if percentage <= 0
......
......@@ -29,6 +29,30 @@ module EE
log_geo_event(key)
end
override :enable_percentage_of_time
def enable_percentage_of_time(key, percentage)
super
log_geo_event(key)
end
override :disable_percentage_of_time
def disable_percentage_of_time(key)
super
log_geo_event(key)
end
override :enable_percentage_of_actors
def enable_percentage_of_actors(key, percentage)
super
log_geo_event(key)
end
override :disable_percentage_of_actors
def disable_percentage_of_actors(key)
super
log_geo_event(key)
end
private
def log_geo_event(key)
......
......@@ -55,8 +55,8 @@ describe OnboardingExperimentHelper, type: :helper do
context 'and experiment_growth_onboarding has been set' do
it 'checks if feature is enabled for current_user' do
feature = Feature.get(described_class::EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME)
feature.enable_percentage_of_actors(50)
Feature.enable_percentage_of_actors(
described_class::EXPERIMENT_GROWTH_ONBOARDING_FEATURE_NAME, 50)
expect(helper.allow_access_to_onboarding?).to eq(true)
end
......
......@@ -44,7 +44,7 @@ describe RecaptchaExperimentHelper, type: :helper do
with_them do
before do
# Enable feature to 50% of actors
Feature.get(feature_name).enable_percentage_of_actors(50)
Feature.enable_percentage_of_actors(feature_name, 50)
end
it "returns expected_result" do
......
......@@ -977,7 +977,7 @@ describe User do
end
it 'returns true when 100% control percentage is provided' do
Feature.get(:discover_security_control).enable_percentage_of_time(100)
Feature.enable_percentage_of_time(:discover_security_control, 100)
expect(experiment_user.ab_feature_enabled?(:discover_security)).to eq(true)
expect(experiment_user.user_preference.feature_filter_type).to eq(UserPreference::FEATURE_FILTER_EXPERIMENT)
......
......@@ -61,7 +61,7 @@ module API
mutually_exclusive :key, :project
end
post ':name' do
feature = Feature.get(params[:name])
feature = Feature.get(params[:name]) # rubocop:disable Gitlab/AvoidFeatureGet
targets = gate_targets(params)
value = gate_value(params)
key = gate_key(params)
......@@ -92,7 +92,7 @@ module API
desc 'Remove the gate value for the given feature'
delete ':name' do
Feature.get(params[:name]).remove
Feature.remove(params[:name])
no_content!
end
......
......@@ -79,7 +79,7 @@ class Feature
# for the feature yet and return the default.
return default_enabled unless Gitlab::Database.exists?
feature = Feature.get(key)
feature = get(key)
# If we're not default enabling the flag or the feature has been set, always evaluate.
# `persisted?` can potentially generate DB queries and also checks for inclusion
......@@ -109,12 +109,41 @@ class Feature
get(key).disable_group(group)
end
def enable_percentage_of_time(key, percentage)
get(key).enable_percentage_of_time(percentage)
end
def disable_percentage_of_time(key)
get(key).disable_percentage_of_time
end
def enable_percentage_of_actors(key, percentage)
get(key).enable_percentage_of_actors(percentage)
end
def disable_percentage_of_actors(key)
get(key).disable_percentage_of_actors
end
def remove(key)
return unless persisted_name?(key)
get(key).remove
end
def reset
Gitlab::SafeRequestStore.delete(:flipper) if Gitlab::SafeRequestStore.active?
@flipper = nil
end
# This method is called from config/initializers/flipper.rb and can be used
# to register Flipper groups.
# See https://docs.gitlab.com/ee/development/feature_flags.html#feature-groups
def register_feature_groups
end
private
def flipper
if Gitlab::SafeRequestStore.active?
Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance
......@@ -123,11 +152,6 @@ class Feature
end
end
def reset
Gitlab::SafeRequestStore.delete(:flipper) if Gitlab::SafeRequestStore.active?
@flipper = nil
end
def build_flipper_instance
active_record_adapter = Flipper::Adapters::ActiveRecord.new(
feature_class: FlipperFeature,
......@@ -158,12 +182,6 @@ class Feature
Gitlab.dev_or_test_env?
end
# This method is called from config/initializers/flipper.rb and can be used
# to register Flipper groups.
# See https://docs.gitlab.com/ee/development/feature_flags.html#feature-groups
def register_feature_groups
end
def l1_cache_backend
Gitlab::ProcessMemoryCache.cache_backend
end
......
......@@ -12,19 +12,21 @@
#
# To enable the experiment for 10% of the users:
#
# chatops: `/chatops run feature set experiment_key_experiment_percentage 10`
# console: `Feature.get(:experiment_key_experiment_percentage).enable_percentage_of_time(10)`
# chatops: `/chatops run feature set experiment_key_experiment_percentage 10 --actors`
# console: `Feature.enable_percentage_of_actors(:experiment_key_experiment_percentage, 10)`
#
# To disable the experiment:
#
# chatops: `/chatops run feature delete experiment_key_experiment_percentage`
# console: `Feature.get(:experiment_key_experiment_percentage).remove`
# console: `Feature.remove(:experiment_key_experiment_percentage)`
#
# To check the current rollout percentage:
#
# chatops: `/chatops run feature get experiment_key_experiment_percentage`
# console: `Feature.get(:experiment_key_experiment_percentage).percentage_of_time_value`
#
# TODO: rewrite that
module Gitlab
module Experimentation
EXPERIMENTS = {
......@@ -178,7 +180,7 @@ module Gitlab
# When a feature does not exist, the `percentage_of_time_value` method will return 0
def experiment_percentage
@experiment_percentage ||= Feature.get(:"#{key}_experiment_percentage").percentage_of_time_value
@experiment_percentage ||= Feature.get(:"#{key}_experiment_percentage").percentage_of_time_value # rubocop:disable Gitlab/AvoidFeatureGet
end
end
end
......
......@@ -52,7 +52,7 @@ module Gitlab
end
def disabled_by_feature(options)
options.with_feature && !::Feature.get(options.with_feature).enabled?
options.with_feature && !::Feature.enabled?(options.with_feature)
end
def build_metric!(type, name, options)
......
......@@ -19,7 +19,7 @@ module Gitlab
private
def feature
Feature.get(:sourcegraph)
Feature.get(:sourcegraph) # rubocop:disable Gitlab/AvoidFeatureGet
end
end
end
......
......@@ -21,7 +21,7 @@ namespace :gitlab do
Gitlab::Git::RuggedImpl::Repository::FEATURE_FLAGS.each do |flag|
case status
when nil
Feature.get(flag).remove
Feature.remove(flag)
when true
Feature.enable(flag)
when false
......
# frozen_string_literal: true
module RuboCop
module Cop
module Gitlab
# Cop that blacklists the use of `Feature.get`.
class AvoidFeatureGet < RuboCop::Cop::Cop
MSG = 'Use `Feature.enable/disable` methods instead of `Feature.get`. ' \
'See doc/development/testing_guide/best_practices.md#feature-flags-in-tests for more information.'
def_node_matcher :feature_get?, <<~PATTERN
(send (const nil? :Feature) :get ...)
PATTERN
def_node_matcher :global_feature_get?, <<~PATTERN
(send (const (cbase) :Feature) :get ...)
PATTERN
def on_send(node)
return unless feature_get?(node) || global_feature_get?(node)
add_offense(node, location: :selector)
end
end
end
end
end
......@@ -36,10 +36,6 @@ describe SourcegraphDecorator do
sign_in user if user
end
after do
Feature.get(:sourcegraph).disable
end
subject do
get :index, format: format
......
......@@ -13,7 +13,7 @@ describe AutoDevopsHelper do
allow(helper).to receive(:can?).with(user, :admin_pipeline, project) { allowed }
allow(helper).to receive(:current_user) { user }
Feature.get(:auto_devops_banner_disabled).disable
stub_feature_flags(auto_devops_banner_disabled: false)
end
subject { helper.show_auto_devops_callout?(project) }
......@@ -32,7 +32,7 @@ describe AutoDevopsHelper do
context 'when the banner is disabled by feature flag' do
before do
Feature.get(:auto_devops_banner_disabled).enable
stub_feature_flags(auto_devops_banner_disabled: true)
end
it { is_expected.to be_falsy }
......
......@@ -142,7 +142,7 @@ describe Feature, stub_feature_flags: false do
expect(Flipper).to receive(:new).once.and_call_original
2.times do
described_class.flipper
described_class.send(:flipper)
end
end
end
......@@ -151,9 +151,9 @@ describe Feature, stub_feature_flags: false do
it 'memoizes the Flipper instance' do
expect(Flipper).to receive(:new).once.and_call_original
described_class.flipper
described_class.send(:flipper)
described_class.instance_variable_set(:@flipper, nil)
described_class.flipper
described_class.send(:flipper)
end
end
end
......@@ -179,21 +179,21 @@ describe Feature, stub_feature_flags: false do
expect(described_class.enabled?(:enabled_feature_flag)).to be_truthy
end
it { expect(described_class.l1_cache_backend).to eq(Gitlab::ProcessMemoryCache.cache_backend) }
it { expect(described_class.l2_cache_backend).to eq(Rails.cache) }
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 do
described_class.enable(:enabled_feature_flag)
flipper_key = "flipper/v1/feature/enabled_feature_flag"
expect(described_class.l2_cache_backend)
expect(described_class.send(:l2_cache_backend))
.to receive(:fetch)
.once
.with(flipper_key, expires_in: 1.hour)
.and_call_original
expect(described_class.l1_cache_backend)
expect(described_class.send(:l1_cache_backend))
.to receive(:fetch)
.once
.with(flipper_key, expires_in: 1.minute)
......@@ -215,14 +215,14 @@ describe Feature, stub_feature_flags: false do
let(:flag) { :some_feature_flag }
before do
described_class.flipper.memoize = false
described_class.send(:flipper).memoize = false
described_class.enabled?(flag)
end
it 'caches the status in L1 cache for the first minute' do
expect do
expect(described_class.l1_cache_backend).to receive(:fetch).once.and_call_original
expect(described_class.l2_cache_backend).not_to receive(:fetch)
expect(described_class.send(:l1_cache_backend)).to receive(:fetch).once.and_call_original
expect(described_class.send(:l2_cache_backend)).not_to receive(:fetch)
expect(described_class.enabled?(flag)).to be_truthy
end.not_to exceed_query_limit(0)
end
......@@ -230,8 +230,8 @@ describe Feature, stub_feature_flags: false do
it 'caches the status in L2 cache after 2 minutes' do
Timecop.travel 2.minutes do
expect do
expect(described_class.l1_cache_backend).to receive(:fetch).once.and_call_original
expect(described_class.l2_cache_backend).to receive(:fetch).once.and_call_original
expect(described_class.send(:l1_cache_backend)).to receive(:fetch).once.and_call_original
expect(described_class.send(:l2_cache_backend)).to receive(:fetch).once.and_call_original
expect(described_class.enabled?(flag)).to be_truthy
end.not_to exceed_query_limit(0)
end
......@@ -240,8 +240,8 @@ describe Feature, stub_feature_flags: false do
it 'fetches the status after an hour' do
Timecop.travel 61.minutes do
expect do
expect(described_class.l1_cache_backend).to receive(:fetch).once.and_call_original
expect(described_class.l2_cache_backend).to receive(:fetch).once.and_call_original
expect(described_class.send(:l1_cache_backend)).to receive(:fetch).once.and_call_original
expect(described_class.send(:l2_cache_backend)).to receive(:fetch).once.and_call_original
expect(described_class.enabled?(flag)).to be_truthy
end.not_to exceed_query_limit(1)
end
......
......@@ -11,7 +11,7 @@ describe Gitlab::Experimentation do
}
})
Feature.get(:test_experiment_experiment_percentage).enable_percentage_of_time(enabled_percentage)
Feature.enable_percentage_of_time(:test_experiment_experiment_percentage, enabled_percentage)
end
let(:environment) { Rails.env.test? }
......
......@@ -8,7 +8,6 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
let(:project) { create(:project, :repository) }
let(:repository) { project.repository }
let(:feature_flag_name) { 'feature-flag-name' }
let(:feature_flag) { Feature.get(feature_flag_name) }
let(:temp_gitaly_metadata_file) { create_temporary_gitaly_metadata_file }
before(:all) do
......
......@@ -26,7 +26,7 @@ describe Gitlab::Metrics::MethodCall do
context 'prometheus instrumentation is enabled' do
before do
Feature.get(:prometheus_metrics_method_instrumentation).enable
stub_feature_flags(prometheus_metrics_method_instrumentation: true)
end
around do |example|
......@@ -50,7 +50,7 @@ describe Gitlab::Metrics::MethodCall do
context 'prometheus instrumentation is disabled' do
before do
Feature.get(:prometheus_metrics_method_instrumentation).disable
stub_feature_flags(prometheus_metrics_method_instrumentation: false)
end
it 'observes using NullMetric' do
......
......@@ -104,7 +104,7 @@ describe Gitlab::Metrics::Methods do
context 'when feature is enabled' do
before do
Feature.get(feature_name).enable
stub_feature_flags(feature_name => true)
end
it "initializes #{metric_type} metric" do
......@@ -118,7 +118,7 @@ describe Gitlab::Metrics::Methods do
context 'when feature is disabled' do
before do
Feature.get(feature_name).disable
stub_feature_flags(feature_name => false)
end
it "returns NullMetric" do
......
......@@ -4262,7 +4262,7 @@ describe Project do
describe '#auto_devops_enabled?' do
before do
Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(0)
Feature.enable_percentage_of_actors(:force_autodevops_on_by_default, 0)
end
let_it_be(:project, reload: true) { create(:project) }
......@@ -4464,7 +4464,7 @@ describe Project do
let_it_be(:project, reload: true) { create(:project) }
before do
Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(0)
Feature.enable_percentage_of_actors(:force_autodevops_on_by_default, 0)
end
context 'when explicitly disabled' do
......@@ -4510,7 +4510,7 @@ describe Project do
before do
create(:project_auto_devops, project: project, enabled: false)
Feature.get(:force_autodevops_on_by_default).enable_percentage_of_actors(100)
Feature.enable_percentage_of_actors(:force_autodevops_on_by_default, 100)
end
it 'does not have auto devops implicitly disabled' do
......
......@@ -39,9 +39,9 @@ describe API::Features, stub_feature_flags: false do
end
before do
Feature.get('feature_1').enable
Feature.get('feature_2').disable
Feature.get('feature_3').enable Feature.group(:perf_team)
Feature.enable('feature_1')
Feature.disable('feature_2')
Feature.enable('feature_3', Feature.group(:perf_team))
end
it 'returns a 401 for anonymous users' do
......@@ -227,10 +227,8 @@ describe API::Features, stub_feature_flags: false do
end
context 'when the feature exists' do
let(:feature) { Feature.get(feature_name) }
before do
feature.disable # This also persists the feature on the DB
Feature.disable(feature_name) # This also persists the feature on the DB
end
context 'when passed value=true' do
......@@ -273,8 +271,8 @@ describe API::Features, stub_feature_flags: false do
context 'when feature is enabled and value=false is passed' do
it 'disables the feature' do
feature.enable
expect(feature).to be_enabled
Feature.enable(feature_name)
expect(Feature.enabled?(feature_name)).to eq(true)
post api("/features/#{feature_name}", admin), params: { value: 'false' }
......@@ -286,8 +284,8 @@ describe API::Features, stub_feature_flags: false do
end
it 'disables the feature for the given Flipper group when passed feature_group=perf_team' do
feature.enable(Feature.group(:perf_team))
expect(Feature.get(feature_name).enabled?(admin)).to be_truthy
Feature.enable(feature_name, Feature.group(:perf_team))
expect(Feature.enabled?(feature_name, admin)).to be_truthy
post api("/features/#{feature_name}", admin), params: { value: 'false', feature_group: 'perf_team' }
......@@ -299,8 +297,8 @@ describe API::Features, stub_feature_flags: false do
end
it 'disables the feature for the given user when passed user=username' do
feature.enable(user)
expect(Feature.get(feature_name).enabled?(user)).to be_truthy
Feature.enable(feature_name, user)
expect(Feature.enabled?(feature_name, user)).to be_truthy
post api("/features/#{feature_name}", admin), params: { value: 'false', user: user.username }
......@@ -314,7 +312,7 @@ describe API::Features, stub_feature_flags: false do
context 'with a pre-existing percentage of time value' do
before do
feature.enable_percentage_of_time(50)
Feature.enable_percentage_of_time(feature_name, 50)
end
it 'updates the percentage of time if passed an integer' do
......@@ -333,7 +331,7 @@ describe API::Features, stub_feature_flags: false do
context 'with a pre-existing percentage of actors value' do
before do
feature.enable_percentage_of_actors(42)
Feature.enable_percentage_of_actors(feature_name, 42)
end
it 'updates the percentage of actors if passed an integer' do
......@@ -378,14 +376,17 @@ describe API::Features, stub_feature_flags: false do
context 'when the gate value was set' do
before do
Feature.get(feature_name).enable
Feature.enable(feature_name)
end
it 'deletes an enabled feature' do
delete api("/features/#{feature_name}", admin)
expect do
delete api("/features/#{feature_name}", admin)
Feature.reset
end.to change { Feature.persisted_name?(feature_name) }
.and change { Feature.enabled?(feature_name) }
expect(response).to have_gitlab_http_status(:no_content)
expect(Feature.get(feature_name)).not_to be_enabled
end
end
end
......
......@@ -52,7 +52,7 @@ describe API::Settings, 'Settings' do
storages = Gitlab.config.repositories.storages
.merge({ 'custom' => 'tmp/tests/custom_repositories' })
allow(Gitlab.config.repositories).to receive(:storages).and_return(storages)
Feature.get(:sourcegraph).enable
stub_feature_flags(sourcegraph: true)
end
it "updates application settings" do
......
......@@ -47,7 +47,7 @@ module StubFeatureFlags
def stub_feature_flags(features)
features.each do |feature_name, actors|
# Remove feature flag overwrite
feature = Feature.get(feature_name)
feature = Feature.get(feature_name) # rubocop:disable Gitlab/AvoidFeatureGet
feature.remove
Array(actors).each do |actor|
......
......@@ -32,8 +32,7 @@ describe 'profiles/preferences/show' do
end
before do
# Can't use stub_feature_flags because we use Feature.get to check if conditinally applied
Feature.get(:sourcegraph).enable sourcegraph_feature
stub_feature_flags(sourcegraph: sourcegraph_feature)
stub_application_setting(sourcegraph_enabled: sourcegraph_enabled)
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment