diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index c6d8e45c86d12648489a13f83ddd4558609d7602..4f9c7235a307418f9f2ff3c3cb465db600a995d3 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -338,39 +338,45 @@ class ApplicationSetting < ActiveRecord::Base
end
def performance_bar_allowed_group_id=(group_full_path)
+ group_full_path = nil if group_full_path.blank?
+
+ if group_full_path.nil?
+ if group_full_path != performance_bar_allowed_group_id
+ super(group_full_path)
+ Gitlab::PerformanceBar.expire_allowed_user_ids_cache
+ end
+ return
+ end
+
group = Group.find_by_full_path(group_full_path)
- return unless group && group.id != performance_bar_allowed_group_id
- super(group.id)
- Gitlab::PerformanceBar.expire_allowed_user_ids_cache
+ if group
+ if group.id != performance_bar_allowed_group_id
+ super(group.id)
+ Gitlab::PerformanceBar.expire_allowed_user_ids_cache
+ end
+ else
+ super(nil)
+ Gitlab::PerformanceBar.expire_allowed_user_ids_cache
+ end
end
def performance_bar_allowed_group
Group.find_by_id(performance_bar_allowed_group_id)
end
- # Return true is the Performance Bar is available globally or for the
- # `performance_team` feature group
- def performance_bar_enabled?
- feature = Feature.get(:performance_bar)
-
- feature.on? || feature.groups_value.include?('performance_team')
+ # Return true if the Performance Bar is enabled for a given group
+ def performance_bar_enabled
+ performance_bar_allowed_group_id.present?
end
- # - If `enable` is true, enable the `performance_bar` feature for the
- # `performance_team` feature group
- # - If `enable` is false, disable the `performance_bar` feature globally
+ # - If `enable` is true, we early return since the actual attribute that holds
+ # the enabling/disabling is `performance_bar_allowed_group_id`
+ # - If `enable` is false, we set `performance_bar_allowed_group_id` to `nil`
def performance_bar_enabled=(enable)
- feature = Feature.get(:performance_bar)
- performance_bar_on = performance_bar_enabled?
+ return if enable
- if enable && !performance_bar_on
- feature.enable_group(:performance_team)
- Gitlab::PerformanceBar.expire_allowed_user_ids_cache
- elsif !enable && performance_bar_on
- feature.disable
- Gitlab::PerformanceBar.expire_allowed_user_ids_cache
- end
+ self.performance_bar_allowed_group_id = nil
end
# Choose one of the available repository storage options. Currently all have
diff --git a/config/initializers/flipper.rb b/config/initializers/flipper.rb
index bfab8c77a4b9de497a2858a14415e4097167b7de..8ec9613a4b7eee61a11da8a15507dbf6279a7ec0 100644
--- a/config/initializers/flipper.rb
+++ b/config/initializers/flipper.rb
@@ -3,6 +3,4 @@ require 'flipper/middleware/memoizer'
unless Rails.env.test?
Rails.application.config.middleware.use Flipper::Middleware::Memoizer,
lambda { Feature.flipper }
-
- Feature.register_feature_groups
end
diff --git a/doc/administration/monitoring/performance/performance_bar.md b/doc/administration/monitoring/performance/performance_bar.md
index 6ee21eae1945d4d25535bb8053a8f996b4cd764c..ee680c7b258db7244debcf6e06ff4a54767b1e39 100644
--- a/doc/administration/monitoring/performance/performance_bar.md
+++ b/doc/administration/monitoring/performance/performance_bar.md
@@ -33,31 +33,3 @@ Make sure _Enable the Performance Bar_ is checked and hit
---
-
-## Enable the Performance Bar via the API
-
-Under the hood, the Performance Bar activation is done via the `performance_bar`
-[Feature Flag](../../../development/features_flags.md).
-
-That means you can also enable or disable it via the
-[Features API](../../../api/features.md#set-or-create-a-feature).
-
-### For the `performance_team` feature group
-
-The `performance_team` feature group maps to the group specified in your [Admin
-area](#enable-the-performance-bar-via-the-admin-panel).
-
-```
-curl --data "feature_group=performance_team" --data "value=true" --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/features/performance_bar
-```
-
-### For specific users
-
-It's also possible to enable the Performance Bar for specific users in addition
-to a group, or even instead of a group:
-
-```
-curl --data "user=my_username" --data "value=true" --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/features/performance_bar
-```
-
-[reconfigure]: ../../restart_gitlab.md#omnibus-gitlab-reconfigure
diff --git a/doc/development/feature_flags.md b/doc/development/feature_flags.md
index a51adcfbd3f7140b0089ba31094e541963d49eca..59e8a087e02ca965319a393d93e94487799dd954 100644
--- a/doc/development/feature_flags.md
+++ b/doc/development/feature_flags.md
@@ -15,8 +15,7 @@ Starting from GitLab 9.4 we support feature groups via
Feature groups must be defined statically in `lib/feature.rb` (in the
`.register_feature_groups` method), but their implementation can obviously be
-dynamic (querying the DB etc.). You can see how the `performance_team` feature
-group for a concrete example.
+dynamic (querying the DB etc.).
Once defined in `lib/feature.rb`, you will be able to activate a
feature for a given feature group via the [`feature_group` param of the features API](../api/features.md#set-or-create-a-feature)
diff --git a/lib/feature.rb b/lib/feature.rb
index c4e1741df52a4c26a7c17394bed43b00bbd9ef72..363f66ba60eec2be663164881d412adb7abdf62c 100644
--- a/lib/feature.rb
+++ b/lib/feature.rb
@@ -57,13 +57,5 @@ class Feature
Flipper.new(adapter)
end
end
-
- def register_feature_groups
- Flipper.register(:performance_team) do |actor|
- user = actor.thing
-
- user&.is_a?(User) && Gitlab::PerformanceBar.allowed_user?(user)
- end
- end
end
end
diff --git a/lib/gitlab/performance_bar.rb b/lib/gitlab/performance_bar.rb
index aa033571de0249b088ac0c84172873178e8bef15..2da2ce45ebc08630cd67989a18d4de6bc9545031 100644
--- a/lib/gitlab/performance_bar.rb
+++ b/lib/gitlab/performance_bar.rb
@@ -3,16 +3,9 @@ module Gitlab
include Gitlab::CurrentSettings
ALLOWED_USER_IDS_KEY = 'performance_bar_allowed_user_ids'.freeze
- # The time (in seconds) after which a set of allowed user IDs is expired
- # automatically.
- ALLOWED_USER_IDS_TIME_TO_LIVE = 10.minutes
- def self.enabled?(current_user = nil)
- Feature.enabled?(:performance_bar, current_user)
- end
-
- def self.allowed_user?(user)
- return false unless allowed_group_id
+ def self.enabled?(user = nil)
+ return false unless user && allowed_group_id
allowed_user_ids.include?(user.id)
end
@@ -22,7 +15,7 @@ module Gitlab
end
def self.allowed_user_ids
- Rails.cache.fetch(ALLOWED_USER_IDS_KEY, expires_in: ALLOWED_USER_IDS_TIME_TO_LIVE) do
+ Rails.cache.fetch(ALLOWED_USER_IDS_KEY) do
group = Group.find_by_id(allowed_group_id)
if group
diff --git a/spec/lib/gitlab/performance_bar_spec.rb b/spec/lib/gitlab/performance_bar_spec.rb
index e77a1ebbb0dda03a2137dbb93bfa0104b727155c..8a586bdbf630658c6cdad3915a6943f6003a29e7 100644
--- a/spec/lib/gitlab/performance_bar_spec.rb
+++ b/spec/lib/gitlab/performance_bar_spec.rb
@@ -1,65 +1,55 @@
require 'spec_helper'
describe Gitlab::PerformanceBar do
- describe '.enabled?' do
- it 'returns false when given actor is nil' do
- expect(described_class.enabled?(nil)).to be_falsy
- end
-
- it 'returns false when feature is disabled' do
- actor = double('actor')
-
- expect(Feature).to receive(:enabled?)
- .with(:performance_bar, actor).and_return(false)
-
- expect(described_class.enabled?(actor)).to be_falsy
- end
-
- it 'returns true when feature is enabled' do
- actor = double('actor')
-
- expect(Feature).to receive(:enabled?)
- .with(:performance_bar, actor).and_return(true)
-
- expect(described_class.enabled?(actor)).to be_truthy
- end
- end
-
- shared_examples 'allowed user IDs are cached in Redis for 10 minutes' do
+ shared_examples 'allowed user IDs are cached' do
before do
# Warm the Redis cache
- described_class.allowed_user?(user)
+ described_class.enabled?(user)
end
it 'caches the allowed user IDs in cache', :caching do
expect do
- expect(described_class.allowed_user?(user)).to be_truthy
+ expect(described_class.enabled?(user)).to be_truthy
end.not_to exceed_query_limit(0)
end
end
- describe '.allowed_user?' do
+ describe '.enabled?' do
let(:user) { create(:user) }
before do
- stub_performance_bar_setting(allowed_group: 'my-group')
+ stub_application_setting(performance_bar_allowed_group_id: -1)
+ end
+
+ it 'returns false when given user is nil' do
+ expect(described_class.enabled?(nil)).to be_falsy
end
- context 'when allowed group does not exist' do
+ it 'returns false when allowed_group_id is nil' do
+ expect(described_class).to receive(:allowed_group_id).and_return(nil)
+
+ expect(described_class.enabled?(user)).to be_falsy
+ end
+
+ context 'when allowed group ID does not exist' do
it 'returns false' do
- expect(described_class.allowed_user?(user)).to be_falsy
+ expect(described_class.enabled?(user)).to be_falsy
end
end
context 'when allowed group exists' do
let!(:my_group) { create(:group, path: 'my-group') }
+ before do
+ stub_application_setting(performance_bar_allowed_group_id: my_group.id)
+ end
+
context 'when user is not a member of the allowed group' do
it 'returns false' do
- expect(described_class.allowed_user?(user)).to be_falsy
+ expect(described_class.enabled?(user)).to be_falsy
end
- it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes'
+ it_behaves_like 'allowed user IDs are cached'
end
context 'when user is a member of the allowed group' do
@@ -68,10 +58,10 @@ describe Gitlab::PerformanceBar do
end
it 'returns true' do
- expect(described_class.allowed_user?(user)).to be_truthy
+ expect(described_class.enabled?(user)).to be_truthy
end
- it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes'
+ it_behaves_like 'allowed user IDs are cached'
end
end
@@ -81,11 +71,11 @@ describe Gitlab::PerformanceBar do
before do
create(:group, path: 'my-group')
nested_my_group.add_developer(user)
- stub_performance_bar_setting(allowed_group: 'my-org/my-group')
+ stub_application_setting(performance_bar_allowed_group_id: nested_my_group.id)
end
it 'returns the nested group' do
- expect(described_class.allowed_user?(user)).to be_truthy
+ expect(described_class.enabled?(user)).to be_truthy
end
end
@@ -95,7 +85,7 @@ describe Gitlab::PerformanceBar do
end
it 'returns false' do
- expect(described_class.allowed_user?(user)).to be_falsy
+ expect(described_class.enabled?(user)).to be_falsy
end
end
end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 4b7281d593a0ec5569ec58eb4d7ae8c7d7ce5844..fb485d0b2c672502f8914812590b787e8b83eb9e 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -215,20 +215,27 @@ describe ApplicationSetting, models: true do
end
describe 'performance bar settings' do
- before do
- Flipper.unregister_groups
- Flipper.register(:performance_team)
- end
+ describe 'performance_bar_allowed_group_id=' do
+ context 'with a blank path' do
+ before do
+ setting.performance_bar_allowed_group_id = create(:group).full_path
+ end
- after do
- Flipper.unregister_groups
- end
+ it 'persists nil for a "" path and clears allowed user IDs cache' do
+ expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
- describe 'performance_bar_allowed_group_id=' do
- it 'does not persist an invalid group path' do
- setting.performance_bar_allowed_group_id = 'foo'
+ setting.performance_bar_allowed_group_id = ''
+
+ expect(setting.performance_bar_allowed_group_id).to be_nil
+ end
+ end
+
+ context 'with an invalid path' do
+ it 'does not persist an invalid group path' do
+ setting.performance_bar_allowed_group_id = 'foo'
- expect(setting.performance_bar_allowed_group_id).to be_nil
+ expect(setting.performance_bar_allowed_group_id).to be_nil
+ end
end
context 'with a path to an existing group' do
@@ -243,14 +250,28 @@ describe ApplicationSetting, models: true do
end
context 'when the given path is the same' do
- before do
- setting.performance_bar_allowed_group_id = group.full_path
+ context 'with a blank path' do
+ before do
+ setting.performance_bar_allowed_group_id = nil
+ end
+
+ it 'clears the cached allowed user IDs' do
+ expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
+
+ setting.performance_bar_allowed_group_id = ''
+ end
end
- it 'clears the cached allowed user IDs' do
- expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
+ context 'with a valid path' do
+ before do
+ setting.performance_bar_allowed_group_id = group.full_path
+ end
+
+ it 'clears the cached allowed user IDs' do
+ expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
- setting.performance_bar_allowed_group_id = group.full_path
+ setting.performance_bar_allowed_group_id = group.full_path
+ end
end
end
end
@@ -276,83 +297,71 @@ describe ApplicationSetting, models: true do
end
end
- describe 'performance_bar_enabled?' do
- context 'with the Performance Bar is enabled globally' do
- before do
- Feature.enable(:performance_bar)
- end
-
- it 'returns true' do
- expect(setting).to be_performance_bar_enabled
- end
- end
+ describe 'performance_bar_enabled' do
+ context 'with the Performance Bar is enabled' do
+ let(:group) { create(:group) }
- context 'with the Performance Bar is enabled for the performance_team group' do
before do
- Feature.enable_group(:performance_bar, :performance_team)
+ setting.performance_bar_allowed_group_id = group.full_path
end
it 'returns true' do
- expect(setting).to be_performance_bar_enabled
- end
- end
-
- context 'with the Performance Bar is enabled for a specific user' do
- before do
- Feature.enable(:performance_team, create(:user))
- end
-
- it 'returns false' do
- expect(setting).not_to be_performance_bar_enabled
+ expect(setting.performance_bar_enabled).to be_truthy
end
end
end
describe 'performance_bar_enabled=' do
context 'when the performance bar is enabled' do
+ let(:group) { create(:group) }
+
before do
- Feature.enable(:performance_bar)
+ setting.performance_bar_allowed_group_id = group.full_path
end
context 'when passing true' do
it 'does not clear allowed user IDs cache' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
+
setting.performance_bar_enabled = true
- expect(setting).to be_performance_bar_enabled
+ expect(setting.performance_bar_allowed_group_id).to eq(group.id)
+ expect(setting.performance_bar_enabled).to be_truthy
end
end
context 'when passing false' do
it 'disables the performance bar and clears allowed user IDs cache' do
expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
+
setting.performance_bar_enabled = false
- expect(setting).not_to be_performance_bar_enabled
+ expect(setting.performance_bar_allowed_group_id).to be_nil
+ expect(setting.performance_bar_enabled).to be_falsey
end
end
end
context 'when the performance bar is disabled' do
- before do
- Feature.disable(:performance_bar)
- end
-
context 'when passing true' do
- it 'enables the performance bar and clears allowed user IDs cache' do
- expect(Gitlab::PerformanceBar).to receive(:expire_allowed_user_ids_cache)
+ it 'does nothing and does not clear allowed user IDs cache' do
+ expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
+
setting.performance_bar_enabled = true
- expect(setting).to be_performance_bar_enabled
+ expect(setting.performance_bar_allowed_group_id).to be_nil
+ expect(setting.performance_bar_enabled).to be_falsey
end
end
context 'when passing false' do
- it 'does not clear allowed user IDs cache' do
+ it 'does nothing and does not clear allowed user IDs cache' do
expect(Gitlab::PerformanceBar).not_to receive(:expire_allowed_user_ids_cache)
+
setting.performance_bar_enabled = false
- expect(setting).not_to be_performance_bar_enabled
+ expect(setting.performance_bar_allowed_group_id).to be_nil
+ expect(setting.performance_bar_enabled).to be_falsey
end
end
end
diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb
index 1d9946b0ed179b890a11e1d06453dc8ae40c6cbf..48f454c71876b00eb9c9be967d877b383f163717 100644
--- a/spec/support/stub_configuration.rb
+++ b/spec/support/stub_configuration.rb
@@ -29,10 +29,6 @@ module StubConfiguration
allow(Gitlab.config.omniauth).to receive_messages(messages)
end
- def stub_performance_bar_setting(messages)
- allow(Gitlab.config.performance_bar).to receive_messages(messages)
- end
-
private
# Modifies stubbed messages to also stub possible predicate versions