Merge branch 'make-development-feature-flags-required' into 'master'

Make `development` flags to be `required`

See merge request gitlab-org/gitlab!42687

app/controllers/projects/issues_controller.rb

@@ -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]

app/controllers/projects/services_controller.rb

@@ -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

config/feature_flags/development/approval_rule.yml

+---
+name: approval_rule
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: development
+default_enabled: true

config/feature_flags/development/code_navigation.yml

+---
+name: code_navigation
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: development
+default_enabled: true

config/feature_flags/development/coverage_report_view.yml

+---
+name: coverage_report_view
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: development
+default_enabled: true

config/feature_flags/development/dag_pipeline_tab.yml

+---
+name: dag_pipeline_tab
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: development
+default_enabled: true

config/feature_flags/development/default_merge_ref_for_diffs.yml

+---
+name: default_merge_ref_for_diffs
+introduced_by_url: 
+rollout_issue_url: 
+type: development
+group:
+default_enabled: false

config/feature_flags/development/graphql_releases_page.yml

+---
+name: graphql_releases_page
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: development
+default_enabled: true

config/feature_flags/development/merge_ref_head_comments.yml

+---
+name: merge_ref_head_comments
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: development
+default_enabled: true

config/feature_flags/development/usage_data_a_compliance_audit_events_api.yml

+---
+name: usage_data_a_compliance_audit_events_api
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: development
+default_enabled: false

config/feature_flags/development/usage_data_i_source_code_code_intelligence.yml

+---
+name: usage_data_i_source_code_code_intelligence
+introduced_by_url: 
+rollout_issue_url: 
+group: 
+type: development
+default_enabled: true

ee/config/feature_flags/development/rugged-feature-flag-name.yml → config/feature_flags/development/vue_sidebar_labels.yml

 ---
-name: rugged-feature-flag-name
+name: vue_sidebar_labels
 introduced_by_url: 
 rollout_issue_url: 
 group: 

ee/app/controllers/ee/groups_controller.rb

@@ -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
 

ee/app/controllers/projects/integrations/jira/issues_controller.rb

@@ -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

ee/app/models/ee/personal_access_token.rb

@@ -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
 

ee/config/feature_flags/development/geo_package_file_replication.yml

+---
+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]

ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb

@@ -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

ee/spec/requests/api/features_spec.rb

@@ -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

lib/feature/shared.rb

@@ -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

spec/bin/feature_flag_spec.rb

@@ -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) }

spec/graphql/features/feature_flag_spec.rb

@@ -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

spec/graphql/types/base_field_spec.rb

@@ -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)
 

spec/lib/api/helpers_spec.rb

@@ -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)

spec/lib/feature/definition_spec.rb

@@ -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

spec/lib/feature_spec.rb

@@ -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

spec/lib/gitlab/background_migration/merge_request_assignees_migration_progress_check_spec.rb

@@ -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
 

spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb

@@ -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)

spec/lib/gitlab/gon_helper_spec.rb

@@ -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')

spec/lib/gitlab/utils/usage_data_spec.rb

@@ -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)

spec/requests/api/features_spec.rb

@@ -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

spec/requests/api/usage_data_spec.rb

@@ -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)
 

spec/support/helpers/stub_feature_flags.rb

@@ -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

spec/support_specs/helpers/stub_feature_flags_spec.rb

@@ -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 }