From cdc70c178005682694b652ee6e5cee24006788ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 28 Jul 2020 18:48:53 +0200 Subject: [PATCH 1/2] Make feature flag auto creation to be a first class citizen --- .gitlab/ci/rails.gitlab-ci.yml | 4 +++ .../licensed/ci_project_subscriptions.yml | 8 +++++ .../licensed/enforce_pat_expiration.yml | 9 +++++ ...ha_beta_feature_support_shared_examples.rb | 4 +++ lib/feature/definition.rb | 35 ++++++++++++++++++- lib/feature/shared.rb | 6 +++- ...assignees_migration_progress_check_spec.rb | 2 +- 7 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 ee/config/feature_flags/licensed/ci_project_subscriptions.yml create mode 100644 ee/config/feature_flags/licensed/enforce_pat_expiration.yml diff --git a/.gitlab/ci/rails.gitlab-ci.yml b/.gitlab/ci/rails.gitlab-ci.yml index 38bcea0df1f6fc..079203e1e71c0b 100644 --- a/.gitlab/ci/rails.gitlab-ci.yml +++ b/.gitlab/ci/rails.gitlab-ci.yml @@ -349,9 +349,13 @@ rspec:coverage: dependencies: - setup-test-env - rspec migration pg11 + - rspec migration pg11-as-if-foss - rspec unit pg11 + - rspec unit pg11-as-if-foss - rspec integration pg11 + - rspec integration pg11-as-if-foss - rspec system pg11 + - rspec system pg11-as-if-foss - rspec-ee migration pg11 - rspec-ee unit pg11 - rspec-ee integration pg11 diff --git a/ee/config/feature_flags/licensed/ci_project_subscriptions.yml b/ee/config/feature_flags/licensed/ci_project_subscriptions.yml new file mode 100644 index 00000000000000..a660a0bec889cb --- /dev/null +++ b/ee/config/feature_flags/licensed/ci_project_subscriptions.yml @@ -0,0 +1,8 @@ +--- +name: ci_project_subscriptions +introduced_by_url: +rollout_issue_url: +type: licensed +group: +# This uses `feature_available?` and `beta_feature_available?` +default_enabled: [false, true] diff --git a/ee/config/feature_flags/licensed/enforce_pat_expiration.yml b/ee/config/feature_flags/licensed/enforce_pat_expiration.yml new file mode 100644 index 00000000000000..93bd0b88705020 --- /dev/null +++ b/ee/config/feature_flags/licensed/enforce_pat_expiration.yml @@ -0,0 +1,9 @@ +--- +name: enforce_pat_expiration +introduced_by_url: +rollout_issue_url: +type: licensed +group: +# License.feature_available?(:enforce_pat_expiration) +# ::Feature.enabled?(:enforce_pat_expiration, type: :licensed, default_enabled: false) +default_enabled: [false, true] diff --git a/ee/spec/support/shared_examples/models/alpha_beta_feature_support_shared_examples.rb b/ee/spec/support/shared_examples/models/alpha_beta_feature_support_shared_examples.rb index b9ab49a862cc6c..f80e80afd82db7 100644 --- a/ee/spec/support/shared_examples/models/alpha_beta_feature_support_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/alpha_beta_feature_support_shared_examples.rb @@ -7,6 +7,10 @@ with_them do let(:method_name) { "#{level}_feature_available?" } + before do + skip_feature_flags_yaml_validation + end + context 'when license does not allow it' do before do stub_licensed_features(insights: false) diff --git a/lib/feature/definition.rb b/lib/feature/definition.rb index 0ba1bdc47990e6..0f615ca22620cb 100644 --- a/lib/feature/definition.rb +++ b/lib/feature/definition.rb @@ -78,6 +78,17 @@ def to_h attributes end + def to_yaml + attributes.slice( + *::Feature::Shared::PARAMS + ).stringify_keys.to_yaml + end + + def save! + FileUtils.mkdir_p(File.dirname(path)) + File.write(path, to_yaml) + end + class << self def paths @paths ||= [Rails.root.join('config', 'feature_flags', '**', '*.yml')] @@ -98,7 +109,11 @@ 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] + if type_definition[:auto_create] + auto_create!(key, type: type, default_enabled: default_enabled) + elsif !type_definition[:optional] + raise InvalidFeatureFlagError, "Missing feature definition for `#{key}`" + end else raise InvalidFeatureFlagError, "Unknown feature flag type used: `#{type}`" end @@ -162,6 +177,24 @@ def reload_directories end end end + + def auto_create!(key, type:, default_enabled:) + return if definitions[key.to_sym] + + dir = File.dirname(paths.first) # strip /*.yml + dir = File.dirname(dir) # strip /**/ + path = File.join(dir, type.to_s, key.to_s + '.yml') + return if File.exist?(path) + + definition = Feature::Definition.new( + path, + name: key.to_s, + type: type.to_s, + default_enabled: default_enabled + ).tap(&:save!) + + definitions[definition.key] = definition + end end end end diff --git a/lib/feature/shared.rb b/lib/feature/shared.rb index 9ec56ee6b52508..27cc2ce67c84db 100644 --- a/lib/feature/shared.rb +++ b/lib/feature/shared.rb @@ -6,7 +6,8 @@ class Feature module Shared - # optional: defines if a on-disk definition is required for this feature flag type + # optional: defines if a on-disk definition is required or optional for this feature flag type + # auto_create: if definition is missing auto create it # rollout_issue: defines if `bin/feature-flag` asks for rollout issue # default_enabled: defines a default state of a feature flag when created by `bin/feature-flag` # ee_only: defines that a feature flag can only be created in a context of EE @@ -15,6 +16,7 @@ module Shared development: { description: 'Short lived, used to enable unfinished code to be deployed', optional: false, + auto_create: false, rollout_issue: true, ee_only: false, default_enabled: false, @@ -27,6 +29,7 @@ module Shared ops: { description: "Long-lived feature flags that control operational aspects of GitLab's behavior", optional: true, + auto_create: true, rollout_issue: false, ee_only: false, default_enabled: false, @@ -38,6 +41,7 @@ module Shared licensed: { description: 'Permanent feature flags used to temporarily disable licensed features.', optional: true, + auto_create: true, rollout_issue: false, ee_only: true, default_enabled: true, diff --git a/spec/lib/gitlab/background_migration/merge_request_assignees_migration_progress_check_spec.rb b/spec/lib/gitlab/background_migration/merge_request_assignees_migration_progress_check_spec.rb index 85a9c88ebff667..7408e4f3f5633e 100644 --- a/spec/lib/gitlab/background_migration/merge_request_assignees_migration_progress_check_spec.rb +++ b/spec/lib/gitlab/background_migration/merge_request_assignees_migration_progress_check_spec.rb @@ -73,7 +73,7 @@ described_class.new.perform - expect(Feature.enabled?(:multiple_merge_request_assignees, type: :licensed)).to eq(true) + expect(Feature.enabled?(:multiple_merge_request_assignees, type: :licensed, default_enabled: true)).to eq(true) end end -- GitLab From 09f048e61aa5856ee9f14c2a87ca9cfc17ee6fc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 6 Oct 2020 18:11:13 +0200 Subject: [PATCH 2/2] Fix specs failure --- .gitlab/ci/rails.gitlab-ci.yml | 4 ++++ app/controllers/projects/issues_controller.rb | 2 +- ee/app/controllers/ee/groups_controller.rb | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/.gitlab/ci/rails.gitlab-ci.yml b/.gitlab/ci/rails.gitlab-ci.yml index 079203e1e71c0b..6eb14943bc5b70 100644 --- a/.gitlab/ci/rails.gitlab-ci.yml +++ b/.gitlab/ci/rails.gitlab-ci.yml @@ -29,6 +29,8 @@ - tmp/capybara/ - tmp/memory_test/ - log/*.log + - config/feature_flags/ + - ee/config/feature_flags/ reports: junit: junit_rspec.xml @@ -377,6 +379,8 @@ rspec:coverage: - coverage/index.html - coverage/assets/ - tmp/memory_test/ + - config/feature_flags/ + - ee/config/feature_flags/ reports: cobertura: coverage/coverage.xml # EE/FOSS: default refs (MRs, master, schedules) jobs # diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 319a5183429808..220f4482e7e8f3 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/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, type: :licensed) + push_frontend_feature_flag(:scoped_labels, @project, type: :licensed, default_enabled: true) end around_action :allow_gitaly_ref_name_caching, only: [:discussions] diff --git a/ee/app/controllers/ee/groups_controller.rb b/ee/app/controllers/ee/groups_controller.rb index ba26b34998c8d4..077753030222ce 100644 --- a/ee/app/controllers/ee/groups_controller.rb +++ b/ee/app/controllers/ee/groups_controller.rb @@ -12,7 +12,7 @@ module GroupsController before_action :ee_authorize_admin_group!, only: [:restore] before_action only: :issues do - push_frontend_feature_flag(:scoped_labels, @group, type: :licensed) + push_frontend_feature_flag(:scoped_labels, @group, type: :licensed, default_enabled: true) end feature_category :subgroups, [:restore, :subgroups] -- GitLab