Commit 9145ed53 authored by Kamil Trzciński's avatar Kamil Trzciński

Make `development` flags to be `required`

This does:

- add all leftover `development` feature flags
- ensures that licensed `feature flags` are actually checked
  against `type: :licensed`
- makes `development` to be `optional: false`, aka `required`
parent b6d6708d
......@@ -56,7 +56,7 @@ class Projects::IssuesController < Projects::ApplicationController
end
before_action only: :index do
push_frontend_feature_flag(:scoped_labels, @project)
push_frontend_feature_flag(:scoped_labels, @project, type: :licensed)
end
around_action :allow_gitaly_ref_name_caching, only: [:discussions]
......
......@@ -12,7 +12,7 @@ class Projects::ServicesController < Projects::ApplicationController
before_action :set_deprecation_notice_for_prometheus_service, only: [:edit, :update]
before_action :redirect_deprecated_prometheus_service, only: [:update]
before_action only: :edit do
push_frontend_feature_flag(:jira_issues_integration, @project, { default_enabled: true })
push_frontend_feature_flag(:jira_issues_integration, @project, type: :licensed, default_enabled: true)
end
respond_to :html
......
---
name: approval_rule
introduced_by_url:
rollout_issue_url:
group:
type: development
default_enabled: true
---
name: code_navigation
introduced_by_url:
rollout_issue_url:
group:
type: development
default_enabled: true
---
name: coverage_report_view
introduced_by_url:
rollout_issue_url:
group:
type: development
default_enabled: true
---
name: dag_pipeline_tab
introduced_by_url:
rollout_issue_url:
group:
type: development
default_enabled: true
---
name: default_merge_ref_for_diffs
introduced_by_url:
rollout_issue_url:
type: development
group:
default_enabled: false
---
name: graphql_releases_page
introduced_by_url:
rollout_issue_url:
group:
type: development
default_enabled: true
---
name: merge_ref_head_comments
introduced_by_url:
rollout_issue_url:
group:
type: development
default_enabled: true
---
name: usage_data_a_compliance_audit_events_api
introduced_by_url:
rollout_issue_url:
group:
type: development
default_enabled: false
---
name: usage_data_i_source_code_code_intelligence
introduced_by_url:
rollout_issue_url:
group:
type: development
default_enabled: true
---
name: rugged-feature-flag-name
name: vue_sidebar_labels
introduced_by_url:
rollout_issue_url:
group:
......
......@@ -12,7 +12,7 @@ module EE
before_action :ee_authorize_admin_group!, only: [:restore]
before_action only: :issues do
push_frontend_feature_flag(:scoped_labels, @group)
push_frontend_feature_flag(:scoped_labels, @group, type: :licensed)
end
end
......
......@@ -11,7 +11,7 @@ module Projects
before_action :check_feature_enabled!
before_action do
push_frontend_feature_flag(:jira_issues_integration, project, { default_enabled: true })
push_frontend_feature_flag(:jira_issues_integration, project, type: :licensed, default_enabled: true)
end
rescue_from ::Projects::Integrations::Jira::IssuesFinder::IntegrationError, with: :render_integration_error
......
......@@ -48,7 +48,7 @@ module EE
def enforce_pat_expiration_feature_available?
License.feature_available?(:enforce_pat_expiration) &&
::Feature.enabled?(:enforce_pat_expiration, default_enabled: false)
::Feature.enabled?(:enforce_pat_expiration, type: :licensed, default_enabled: false)
end
end
......
---
name: geo_package_file_replication
introduced_by_url:
rollout_issue_url:
group:
type: development
# it appears that in some places
# `false` is specified vs `true` in other
default_enabled: [false, true]
......@@ -23,7 +23,7 @@ RSpec.describe Gitlab::BackgroundMigration::MigrateApproverToApprovalRulesCheckP
described_class.new.perform
expect(Feature.enabled?(:approval_rule)).to eq(true)
expect(Feature.enabled?(:approval_rule, default_enabled: true)).to eq(true)
end
end
end
......@@ -13,6 +13,8 @@ RSpec.describe API::Features, stub_feature_flags: false do
Flipper.register(:perf_team) do |actor|
actor.respond_to?(:admin) && actor.admin?
end
skip_feature_flags_yaml_validation
end
describe 'POST /feature' do
......
......@@ -13,7 +13,7 @@ class Feature
TYPES = {
development: {
description: 'Short lived, used to enable unfinished code to be deployed',
optional: true,
optional: false,
rollout_issue: true,
default_enabled: false,
example: <<-EOS
......@@ -46,11 +46,11 @@ class Feature
PARAMS = %i[
name
default_enabled
type
introduced_by_url
rollout_issue_url
type
group
default_enabled
].freeze
end
end
......@@ -8,6 +8,10 @@ load File.expand_path('../../bin/feature-flag', __dir__)
RSpec.describe 'bin/feature-flag' do
using RSpec::Parameterized::TableSyntax
before do
skip_feature_flags_yaml_validation
end
describe FeatureFlagCreator do
let(:argv) { %w[feature-flag-name -t development -g group::memory -i https://url -m http://url] }
let(:options) { FeatureFlagOptionParser.parse(argv) }
......
......@@ -12,6 +12,10 @@ RSpec.describe 'Graphql Field feature flags' do
let(:query_string) { '{ item { name } }' }
let(:result) { execute_query(query_type)['data'] }
before do
skip_feature_flags_yaml_validation
end
subject { result }
describe 'Feature flagged field' do
......
......@@ -126,6 +126,10 @@ RSpec.describe Types::BaseField do
let(:field) { described_class.new(name: 'test', type: GraphQL::STRING_TYPE, feature_flag: flag, null: false) }
let(:context) { {} }
before do
skip_feature_flags_yaml_validation
end
it 'returns false if the feature is not enabled' do
stub_feature_flags(flag => false)
......
......@@ -195,6 +195,10 @@ RSpec.describe API::Helpers do
let(:unknown_event) { 'unknown' }
let(:feature) { "usage_data_#{event_name}" }
before do
skip_feature_flags_yaml_validation
end
context 'with feature enabled' do
before do
stub_feature_flags(feature => true)
......
......@@ -105,6 +105,7 @@ RSpec.describe Feature::Definition do
describe '.load_all!' do
let(:store1) { Dir.mktmpdir('path1') }
let(:store2) { Dir.mktmpdir('path2') }
let(:definitions) { {} }
before do
allow(described_class).to receive(:paths).and_return(
......@@ -113,6 +114,10 @@ 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
it "when there's no feature flags a list of definitions is empty" do
......
......@@ -6,6 +6,7 @@ RSpec.describe Feature, stub_feature_flags: false do
before do
# reset Flipper AR-engine
Feature.reset
skip_feature_flags_yaml_validation
end
describe '.get' do
......@@ -253,6 +254,9 @@ RSpec.describe Feature, stub_feature_flags: false do
end
before do
stub_env('LAZILY_CREATE_FEATURE_FLAG', '0')
allow(Feature::Definition).to receive(:valid_usage!).and_call_original
allow(Feature::Definition).to receive(:definitions) do
{ definition.key => definition }
end
......
......@@ -73,7 +73,7 @@ RSpec.describe Gitlab::BackgroundMigration::MergeRequestAssigneesMigrationProgre
described_class.new.perform
expect(Feature.enabled?(:multiple_merge_request_assignees)).to eq(true)
expect(Feature.enabled?(:multiple_merge_request_assignees, type: :licensed)).to eq(true)
end
end
......
......@@ -7,7 +7,7 @@ require 'tempfile'
RSpec.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_name) { wrapper.rugged_feature_keys.first }
let(:temp_gitaly_metadata_file) { create_temporary_gitaly_metadata_file }
before(:all) do
......@@ -47,7 +47,7 @@ RSpec.describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
end
end
context 'when feature flag is not persisted' do
context 'when feature flag is not persisted', stub_feature_flags: false do
context 'when running puma with multiple threads' do
before do
allow(subject).to receive(:running_puma_with_multiple_threads?).and_return(true)
......
......@@ -10,6 +10,10 @@ RSpec.describe Gitlab::GonHelper do
end
describe '#push_frontend_feature_flag' do
before do
skip_feature_flags_yaml_validation
end
it 'pushes a feature flag to the frontend' do
gon = instance_double('gon')
thing = stub_feature_flag_gate('thing')
......
......@@ -216,6 +216,10 @@ RSpec.describe Gitlab::Utils::UsageData do
let(:unknown_event) { 'unknown' }
let(:feature) { "usage_data_#{event_name}" }
before do
skip_feature_flags_yaml_validation
end
context 'with feature enabled' do
before do
stub_feature_flags(feature => true)
......
......@@ -12,6 +12,8 @@ RSpec.describe API::Features, stub_feature_flags: false do
Flipper.register(:perf_team) do |actor|
actor.respond_to?(:admin) && actor.admin?
end
skip_feature_flags_yaml_validation
end
describe 'GET /features' do
......
......@@ -66,6 +66,10 @@ RSpec.describe API::UsageData do
end
context 'with unknown event' do
before do
skip_feature_flags_yaml_validation
end
it 'returns status ok' do
expect(Gitlab::Redis::HLL).not_to receive(:add)
......
......@@ -62,4 +62,8 @@ module StubFeatureFlags
StubFeatureGate.new(object)
end
def skip_feature_flags_yaml_validation
allow(Feature::Definition).to receive(:valid_usage!)
end
end
......@@ -3,12 +3,31 @@
require 'spec_helper'
RSpec.describe StubFeatureFlags do
let(:feature_name) { :test_feature }
DUMMY_FEATURE_FLAG = :dummy_feature_flag # rubocop:disable RSpec/LeakyConstantDeclaration
# We inject dummy feature flag defintion
# to ensure that we strong validate it's usage
# as well
before(:all) do
definition = Feature::Definition.new(
nil,
name: DUMMY_FEATURE_FLAG,
type: 'development',
# we allow ambigious usage of `default_enabled:`
default_enabled: [false, true]
)
Feature::Definition.definitions[DUMMY_FEATURE_FLAG] = definition
end
after(:all) do
Feature::Definition.definitions.delete(DUMMY_FEATURE_FLAG)
end
describe '#stub_feature_flags' do
using RSpec::Parameterized::TableSyntax
let(:feature_name) { :test_feature }
let(:feature_name) { DUMMY_FEATURE_FLAG }
context 'when checking global state' do
where(:feature_actors, :expected_result) do
......@@ -121,14 +140,14 @@ RSpec.describe StubFeatureFlags do
describe 'stub timing' do
context 'let_it_be variable' do
let_it_be(:let_it_be_var) { Feature.enabled?(:any_feature_flag) }
let_it_be(:let_it_be_var) { Feature.enabled?(DUMMY_FEATURE_FLAG) }
it { expect(let_it_be_var).to eq true }
end
context 'before_all variable' do
before_all do
@suite_var = Feature.enabled?(:any_feature_flag)
@suite_var = Feature.enabled?(DUMMY_FEATURE_FLAG)
end
it { expect(@suite_var).to eq true }
......@@ -136,14 +155,14 @@ RSpec.describe StubFeatureFlags do
context 'before(:all) variable' do
before(:all) do
@suite_var = Feature.enabled?(:any_feature_flag)
@suite_var = Feature.enabled?(DUMMY_FEATURE_FLAG)
end
it { expect(@suite_var).to eq true }
end
context 'with stub_feature_flags meta' do
let(:var) { Feature.enabled?(:any_feature_flag) }
let(:var) { Feature.enabled?(DUMMY_FEATURE_FLAG) }
context 'as true', :stub_feature_flags do
it { expect(var).to eq true }
......
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