Feature flag name should be unique, have a consistent `default_enabled:` and be used only in single context
TL;DR
Today we have a number of implicit feature flags. This does create a few problems for inconclusive default state of them.
Usage of feature flags
-
via feature_available?
: https://docs.gitlab.com/ce/development/feature_flags/development.html that hasdefault_enabled: true
-
via beta_feature_available?
: https://docs.gitlab.com/ce/development/feature_flags/development.html that hasdefault_enabled: false
-
via Feature.enabled?(name, default_enabled: user_provided)
-
we also added promo_feature_available?
which creates a completely dynamic set of a new feature flags:promo_
that hasdefault_enabled: false
-
we added disable_*_deduplication
an implicit feature flag based on sidekiq worker that is bydefault_enabled: false
: !32469 (diffs)
feature_available?
and beta_feature_available?
Usage of We use feature_available?
always as part of precompiling a list of feature flags,
this leads to problem with beta_feature_available?
as it has different default_enabled:
.
Example from current master:
condition(:group_activity_analytics_available) do
@subject.beta_feature_available?(:group_activity_analytics)
end
Both use the same licensed name, but have a different default state.
Usage of a colliding implicit licensed feature vs explicit feature flag
Since feature_available?
has default_enabled: true
this leads to a situation where
we might use the same name in two different contexes, but with different defaults:
- development
- licensed
Example from current master:
EEP_FEATURES = EES_FEATURES + %i[
...
ci_project_subscriptions
]
EEP_FEATURES.freeze
def project_has_subscriptions?
return false unless ::Feature.enabled?(:ci_project_subscriptions, project)
project.downstream_projects.any?
end
Both use the same licensed name, but have a non-consistent default state.
promo_...
Usage of This creates yet another feature flag for each licensed feature making this explicit. Do we need that? Can we model that differently?
def promo_feature_available?(feature)
::Feature.enabled?("promo_#{feature}", default_enabled: false)
end
def feature_available_in_plan?(feature)
return true if ::License.promo_feature_available?(feature)
available_features = strong_memoize(:features_available_in_plan) do
Hash.new do |h, f|
h[f] = (plans.map(&:name) & self.class.plans_with_feature(f)).any?
end
end
available_features[feature]
end
This introduces a number of additional feature flags required to be created for each
licensed feature, with a prefix of promo_
.
Proposal
- Remove
default_enabled:
fromFeature.enabled/disabled?
and read it from YAML definition - Extend
Feature.enabled/disabled?
with an expect type of feature flag, defaulting to:development
or whatever name is - Make
feature_available?
to useFeature.enabled?(..., type: :licensed, ...)
- Remove
beta_feature_available?
and control beta features withdefault_state:
- Remove
promo_feature_available?
and control promo features withdefault_state: