summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>2017-05-31 21:06:03 +0000
committerDmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>2017-05-31 21:06:03 +0000
commit3fc4b2c86090841d9a6245b9b73e46231610703e (patch)
treefc055d19700f433115bbed4f62a86a72c711c56f
parent322c9be816cd5cd9747a8737ec04622aa6b81e6b (diff)
parent671284ba375109becbfa2a288032cdc7301b157b (diff)
downloadgitlab-ce-3fc4b2c86090841d9a6245b9b73e46231610703e.tar.gz
Merge branch 'feature-flags-flipper' into 'master'
Add feature toggles through Flipper See merge request !11747
-rw-r--r--Gemfile4
-rw-r--r--Gemfile.lock6
-rw-r--r--changelogs/unreleased/feature-flags-flipper.yml4
-rw-r--r--db/migrate/20170525174156_create_feature_tables.rb26
-rw-r--r--db/schema.rb20
-rw-r--r--doc/api/features.md83
-rw-r--r--doc/development/README.md1
-rw-r--r--doc/development/feature_flags.md7
-rw-r--r--lib/api/api.rb1
-rw-r--r--lib/api/entities.rb22
-rw-r--r--lib/api/features.rb36
-rw-r--r--lib/feature.rb41
-rw-r--r--lib/gitlab/gitaly_client.rb22
-rw-r--r--spec/lib/feature_spec.rb26
-rw-r--r--spec/lib/gitlab/gitaly_client_spec.rb82
-rw-r--r--spec/requests/api/features_spec.rb104
-rw-r--r--spec/support/gitaly.rb3
17 files changed, 483 insertions, 5 deletions
diff --git a/Gemfile b/Gemfile
index d5d7051ecc6..dce2e4ba94e 100644
--- a/Gemfile
+++ b/Gemfile
@@ -370,3 +370,7 @@ gem 'sys-filesystem', '~> 1.1.6'
gem 'gitaly', '~> 0.7.0'
gem 'toml-rb', '~> 0.3.15', require: false
+
+# Feature toggles
+gem 'flipper', '~> 0.10.2'
+gem 'flipper-active_record', '~> 0.10.2'
diff --git a/Gemfile.lock b/Gemfile.lock
index b4252a2bdc9..f0728a358fa 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -206,6 +206,10 @@ GEM
path_expander (~> 1.0)
ruby_parser (~> 3.0)
sexp_processor (~> 4.0)
+ flipper (0.10.2)
+ flipper-active_record (0.10.2)
+ activerecord (>= 3.2, < 6)
+ flipper (~> 0.10.2)
flowdock (0.7.1)
httparty (~> 0.7)
multi_json
@@ -907,6 +911,8 @@ DEPENDENCIES
faraday (~> 0.11.0)
ffaker (~> 2.4)
flay (~> 2.8.0)
+ flipper (~> 0.10.2)
+ flipper-active_record (~> 0.10.2)
fog-aws (~> 0.9)
fog-core (~> 1.44)
fog-google (~> 0.5)
diff --git a/changelogs/unreleased/feature-flags-flipper.yml b/changelogs/unreleased/feature-flags-flipper.yml
new file mode 100644
index 00000000000..5be5c44166d
--- /dev/null
+++ b/changelogs/unreleased/feature-flags-flipper.yml
@@ -0,0 +1,4 @@
+---
+title: Add feature toggles and API endpoints for admins
+merge_request: 11747
+author:
diff --git a/db/migrate/20170525174156_create_feature_tables.rb b/db/migrate/20170525174156_create_feature_tables.rb
new file mode 100644
index 00000000000..a083c89c85f
--- /dev/null
+++ b/db/migrate/20170525174156_create_feature_tables.rb
@@ -0,0 +1,26 @@
+class CreateFeatureTables < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ def self.up
+ create_table :features do |t|
+ t.string :key, null: false
+ t.timestamps null: false
+ end
+ add_index :features, :key, unique: true
+
+ create_table :feature_gates do |t|
+ t.string :feature_key, null: false
+ t.string :key, null: false
+ t.string :value
+ t.timestamps null: false
+ end
+ add_index :feature_gates, [:feature_key, :key, :value], unique: true
+ end
+
+ def self.down
+ drop_table :feature_gates
+ drop_table :features
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 4c73f74ef1f..d0021a566c8 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20170523091700) do
+ActiveRecord::Schema.define(version: 20170525174156) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
@@ -440,6 +440,24 @@ ActiveRecord::Schema.define(version: 20170523091700) do
add_index "events", ["target_id"], name: "index_events_on_target_id", using: :btree
add_index "events", ["target_type"], name: "index_events_on_target_type", using: :btree
+ create_table "feature_gates", force: :cascade do |t|
+ t.string "feature_key", null: false
+ t.string "key", null: false
+ t.string "value"
+ t.datetime "created_at", null: false
+ t.datetime "updated_at", null: false
+ end
+
+ add_index "feature_gates", ["feature_key", "key", "value"], name: "index_feature_gates_on_feature_key_and_key_and_value", unique: true, using: :btree
+
+ create_table "features", force: :cascade do |t|
+ t.string "key", null: false
+ t.datetime "created_at", null: false
+ t.datetime "updated_at", null: false
+ end
+
+ add_index "features", ["key"], name: "index_features_on_key", unique: true, using: :btree
+
create_table "forked_project_links", force: :cascade do |t|
t.integer "forked_to_project_id", null: false
t.integer "forked_from_project_id", null: false
diff --git a/doc/api/features.md b/doc/api/features.md
new file mode 100644
index 00000000000..89b8d3ac948
--- /dev/null
+++ b/doc/api/features.md
@@ -0,0 +1,83 @@
+# Features API
+
+All methods require administrator authorization.
+
+Notice that currently the API only supports boolean and percentage-of-time gate
+values.
+
+## List all features
+
+Get a list of all persisted features, with its gate values.
+
+```
+GET /features
+```
+
+```bash
+curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/features
+```
+
+Example response:
+
+```json
+[
+ {
+ "name": "experimental_feature",
+ "state": "off",
+ "gates": [
+ {
+ "key": "boolean",
+ "value": false
+ }
+ ]
+ },
+ {
+ "name": "new_library",
+ "state": "on",
+ "gates": [
+ {
+ "key": "boolean",
+ "value": true
+ }
+ ]
+ }
+]
+```
+
+## Set or create a feature
+
+Set a feature's gate value. If a feature with the given name doesn't exist yet
+it will be created. The value can be a boolean, or an integer to indicate
+percentage of time.
+
+```
+POST /features/:name
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `name` | string | yes | Name of the feature to create or update |
+| `value` | integer/string | yes | `true` or `false` to enable/disable, or an integer for percentage of time |
+
+```bash
+curl --data "value=30" --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/features/new_library
+```
+
+Example response:
+
+```json
+{
+ "name": "new_library",
+ "state": "conditional",
+ "gates": [
+ {
+ "key": "boolean",
+ "value": false
+ },
+ {
+ "key": "percentage_of_time",
+ "value": 30
+ }
+ ]
+}
+```
diff --git a/doc/development/README.md b/doc/development/README.md
index 934c6849ff9..be013667684 100644
--- a/doc/development/README.md
+++ b/doc/development/README.md
@@ -42,6 +42,7 @@
- [Sidekiq debugging](sidekiq_debugging.md)
- [Object state models](object_state_models.md)
- [Building a package for testing purposes](build_test_package.md)
+- [Manage feature flags](feature_flags.md)
## Databases
diff --git a/doc/development/feature_flags.md b/doc/development/feature_flags.md
new file mode 100644
index 00000000000..5c6316b9ac6
--- /dev/null
+++ b/doc/development/feature_flags.md
@@ -0,0 +1,7 @@
+# Manage feature flags
+
+Starting from GitLab 9.3 we support feature flags via
+[Flipper](https://github.com/jnunemaker/flipper/). You should use the `Feature`
+class (defined in `lib/feature.rb`) in your code to get, set and list feature
+flags. During runtime you can set the values for the gates via the
+[admin API](../api/features.md).
diff --git a/lib/api/api.rb b/lib/api/api.rb
index bbdd2039f43..7ae2f3cad40 100644
--- a/lib/api/api.rb
+++ b/lib/api/api.rb
@@ -94,6 +94,7 @@ module API
mount ::API::DeployKeys
mount ::API::Deployments
mount ::API::Environments
+ mount ::API::Features
mount ::API::Files
mount ::API::Groups
mount ::API::Internal
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index e10bd230ae2..fc8183a62c1 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -753,6 +753,28 @@ module API
expose :impersonation
end
+ class FeatureGate < Grape::Entity
+ expose :key
+ expose :value
+ end
+
+ class Feature < Grape::Entity
+ expose :name
+ expose :state
+ expose :gates, using: FeatureGate do |model|
+ model.gates.map do |gate|
+ value = model.gate_values[gate.key]
+
+ # By default all gate values are populated. Only show relevant ones.
+ if (value.is_a?(Integer) && value.zero?) || (value.is_a?(Set) && value.empty?)
+ next
+ end
+
+ { key: gate.key, value: value }
+ end.compact
+ end
+ end
+
module JobRequest
class JobInfo < Grape::Entity
expose :name, :stage
diff --git a/lib/api/features.rb b/lib/api/features.rb
new file mode 100644
index 00000000000..cff0ba2ddff
--- /dev/null
+++ b/lib/api/features.rb
@@ -0,0 +1,36 @@
+module API
+ class Features < Grape::API
+ before { authenticated_as_admin! }
+
+ resource :features do
+ desc 'Get a list of all features' do
+ success Entities::Feature
+ end
+ get do
+ features = Feature.all
+
+ present features, with: Entities::Feature, current_user: current_user
+ end
+
+ desc 'Set the gate value for the given feature' do
+ success Entities::Feature
+ end
+ params do
+ requires :value, type: String, desc: '`true` or `false` to enable/disable, an integer for percentage of time'
+ end
+ post ':name' do
+ feature = Feature.get(params[:name])
+
+ if %w(0 false).include?(params[:value])
+ feature.disable
+ elsif params[:value] == 'true'
+ feature.enable
+ else
+ feature.enable_percentage_of_time(params[:value].to_i)
+ end
+
+ present feature, with: Entities::Feature, current_user: current_user
+ end
+ end
+ end
+end
diff --git a/lib/feature.rb b/lib/feature.rb
new file mode 100644
index 00000000000..2e2b343f82c
--- /dev/null
+++ b/lib/feature.rb
@@ -0,0 +1,41 @@
+require 'flipper/adapters/active_record'
+
+class Feature
+ # Classes to override flipper table names
+ class FlipperFeature < Flipper::Adapters::ActiveRecord::Feature
+ # Using `self.table_name` won't work. ActiveRecord bug?
+ superclass.table_name = 'features'
+ end
+
+ class FlipperGate < Flipper::Adapters::ActiveRecord::Gate
+ superclass.table_name = 'feature_gates'
+ end
+
+ class << self
+ def all
+ flipper.features.to_a
+ end
+
+ def get(key)
+ flipper.feature(key)
+ end
+
+ def persisted?(feature)
+ # Flipper creates on-memory features when asked for a not-yet-created one.
+ # If we want to check if a feature has been actually set, we look for it
+ # on the persisted features list.
+ all.map(&:name).include?(feature.name)
+ end
+
+ private
+
+ def flipper
+ @flipper ||= begin
+ adapter = Flipper::Adapters::ActiveRecord.new(
+ feature_class: FlipperFeature, gate_class: FlipperGate)
+
+ Flipper.new(adapter)
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index 72466700c05..2343446bf22 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -2,6 +2,12 @@ require 'gitaly'
module Gitlab
module GitalyClient
+ module MigrationStatus
+ DISABLED = 1
+ OPT_IN = 2
+ OPT_OUT = 3
+ end
+
SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'.freeze
MUTEX = Mutex.new
@@ -46,8 +52,20 @@ module Gitlab
Gitlab.config.gitaly.enabled
end
- def self.feature_enabled?(feature)
- enabled? && ENV["GITALY_#{feature.upcase}"] == '1'
+ def self.feature_enabled?(feature, status: MigrationStatus::OPT_IN)
+ return false if !enabled? || status == MigrationStatus::DISABLED
+
+ feature = Feature.get("gitaly_#{feature}")
+
+ # If the feature hasn't been set, turn it on if it's opt-out
+ return status == MigrationStatus::OPT_OUT unless Feature.persisted?(feature)
+
+ if feature.percentage_of_time_value > 0
+ # Probabilistically enable this feature
+ return Random.rand() * 100 < feature.percentage_of_time_value
+ end
+
+ feature.enabled?
end
def self.migrate(feature)
diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb
new file mode 100644
index 00000000000..1d92a5cb33f
--- /dev/null
+++ b/spec/lib/feature_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe Feature, lib: true do
+ describe '.get' do
+ let(:feature) { double(:feature) }
+ let(:key) { 'my_feature' }
+
+ it 'returns the Flipper feature' do
+ expect_any_instance_of(Flipper::DSL).to receive(:feature).with(key).
+ and_return(feature)
+
+ expect(described_class.get(key)).to be(feature)
+ end
+ end
+
+ describe '.all' do
+ let(:features) { Set.new }
+
+ it 'returns the Flipper features as an array' do
+ expect_any_instance_of(Flipper::DSL).to receive(:features).
+ and_return(features)
+
+ expect(described_class.all).to eq(features.to_a)
+ end
+ end
+end
diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb
index 08ee0dff6b2..95ecba67532 100644
--- a/spec/lib/gitlab/gitaly_client_spec.rb
+++ b/spec/lib/gitlab/gitaly_client_spec.rb
@@ -1,7 +1,10 @@
require 'spec_helper'
-describe Gitlab::GitalyClient, lib: true do
+# We stub Gitaly in `spec/support/gitaly.rb` for other tests. We don't want
+# those stubs while testing the GitalyClient itself.
+describe Gitlab::GitalyClient, lib: true, skip_gitaly_mock: true do
describe '.stub' do
+ # Notice that this is referring to gRPC "stubs", not rspec stubs
before { described_class.clear_stubs! }
context 'when passed a UNIX socket address' do
@@ -32,4 +35,81 @@ describe Gitlab::GitalyClient, lib: true do
end
end
end
+
+ describe 'feature_enabled?' do
+ let(:feature_name) { 'my_feature' }
+ let(:real_feature_name) { "gitaly_#{feature_name}" }
+
+ context 'when Gitaly is disabled' do
+ before { allow(described_class).to receive(:enabled?).and_return(false) }
+
+ it 'returns false' do
+ expect(described_class.feature_enabled?(feature_name)).to be(false)
+ end
+ end
+
+ context 'when the feature status is DISABLED' do
+ let(:feature_status) { Gitlab::GitalyClient::MigrationStatus::DISABLED }
+
+ it 'returns false' do
+ expect(described_class.feature_enabled?(feature_name, status: feature_status)).to be(false)
+ end
+ end
+
+ context 'when the feature_status is OPT_IN' do
+ let(:feature_status) { Gitlab::GitalyClient::MigrationStatus::OPT_IN }
+
+ context "when the feature flag hasn't been set" do
+ it 'returns false' do
+ expect(described_class.feature_enabled?(feature_name, status: feature_status)).to be(false)
+ end
+ end
+
+ context "when the feature flag is set to disable" do
+ before { Feature.get(real_feature_name).disable }
+
+ it 'returns false' do
+ expect(described_class.feature_enabled?(feature_name, status: feature_status)).to be(false)
+ end
+ end
+
+ context "when the feature flag is set to enable" do
+ before { Feature.get(real_feature_name).enable }
+
+ it 'returns true' do
+ expect(described_class.feature_enabled?(feature_name, status: feature_status)).to be(true)
+ end
+ end
+
+ context "when the feature flag is set to a percentage of time" do
+ before { Feature.get(real_feature_name).enable_percentage_of_time(70) }
+
+ it 'bases the result on pseudo-random numbers' do
+ expect(Random).to receive(:rand).and_return(0.3)
+ expect(described_class.feature_enabled?(feature_name, status: feature_status)).to be(true)
+
+ expect(Random).to receive(:rand).and_return(0.8)
+ expect(described_class.feature_enabled?(feature_name, status: feature_status)).to be(false)
+ end
+ end
+ end
+
+ context 'when the feature_status is OPT_OUT' do
+ let(:feature_status) { Gitlab::GitalyClient::MigrationStatus::OPT_OUT }
+
+ context "when the feature flag hasn't been set" do
+ it 'returns true' do
+ expect(described_class.feature_enabled?(feature_name, status: feature_status)).to be(true)
+ end
+ end
+
+ context "when the feature flag is set to disable" do
+ before { Feature.get(real_feature_name).disable }
+
+ it 'returns false' do
+ expect(described_class.feature_enabled?(feature_name, status: feature_status)).to be(false)
+ end
+ end
+ end
+ end
end
diff --git a/spec/requests/api/features_spec.rb b/spec/requests/api/features_spec.rb
new file mode 100644
index 00000000000..f169e6661d1
--- /dev/null
+++ b/spec/requests/api/features_spec.rb
@@ -0,0 +1,104 @@
+require 'spec_helper'
+
+describe API::Features do
+ let(:user) { create(:user) }
+ let(:admin) { create(:admin) }
+
+ describe 'GET /features' do
+ let(:expected_features) do
+ [
+ {
+ 'name' => 'feature_1',
+ 'state' => 'on',
+ 'gates' => [{ 'key' => 'boolean', 'value' => true }]
+ },
+ {
+ 'name' => 'feature_2',
+ 'state' => 'off',
+ 'gates' => [{ 'key' => 'boolean', 'value' => false }]
+ }
+ ]
+ end
+
+ before do
+ Feature.get('feature_1').enable
+ Feature.get('feature_2').disable
+ end
+
+ it 'returns a 401 for anonymous users' do
+ get api('/features')
+
+ expect(response).to have_http_status(401)
+ end
+
+ it 'returns a 403 for users' do
+ get api('/features', user)
+
+ expect(response).to have_http_status(403)
+ end
+
+ it 'returns the feature list for admins' do
+ get api('/features', admin)
+
+ expect(response).to have_http_status(200)
+ expect(json_response).to match_array(expected_features)
+ end
+ end
+
+ describe 'POST /feature' do
+ let(:feature_name) { 'my_feature' }
+ it 'returns a 401 for anonymous users' do
+ post api("/features/#{feature_name}")
+
+ expect(response).to have_http_status(401)
+ end
+
+ it 'returns a 403 for users' do
+ post api("/features/#{feature_name}", user)
+
+ expect(response).to have_http_status(403)
+ end
+
+ it 'creates an enabled feature if passed true' do
+ post api("/features/#{feature_name}", admin), value: 'true'
+
+ expect(response).to have_http_status(201)
+ expect(Feature.get(feature_name)).to be_enabled
+ end
+
+ it 'creates a feature with the given percentage if passed an integer' do
+ post api("/features/#{feature_name}", admin), value: '50'
+
+ expect(response).to have_http_status(201)
+ expect(Feature.get(feature_name).percentage_of_time_value).to be(50)
+ end
+
+ context 'when the feature exists' do
+ let(:feature) { Feature.get(feature_name) }
+
+ before do
+ feature.disable # This also persists the feature on the DB
+ end
+
+ it 'enables the feature if passed true' do
+ post api("/features/#{feature_name}", admin), value: 'true'
+
+ expect(response).to have_http_status(201)
+ expect(feature).to be_enabled
+ end
+
+ context 'with a pre-existing percentage value' do
+ before do
+ feature.enable_percentage_of_time(50)
+ end
+
+ it 'updates the percentage of time if passed an integer' do
+ post api("/features/#{feature_name}", admin), value: '30'
+
+ expect(response).to have_http_status(201)
+ expect(Feature.get(feature_name).percentage_of_time_value).to be(30)
+ end
+ end
+ end
+ end
+end
diff --git a/spec/support/gitaly.rb b/spec/support/gitaly.rb
index 7aca902fc61..2bf159002a0 100644
--- a/spec/support/gitaly.rb
+++ b/spec/support/gitaly.rb
@@ -1,6 +1,7 @@
if Gitlab::GitalyClient.enabled?
RSpec.configure do |config|
- config.before(:each) do
+ config.before(:each) do |example|
+ next if example.metadata[:skip_gitaly_mock]
allow(Gitlab::GitalyClient).to receive(:feature_enabled?).and_return(true)
end
end