From 0c7dd30c78043ea3d4629e1e5739ccfcc7d968fe Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 30 May 2017 16:19:29 -0500 Subject: [PATCH 1/7] Make .gitmodules parsing more resilient to syntax errors --- .../unreleased/dm-gitmodules-parsing.yml | 4 ++ lib/gitlab/git/repository.rb | 44 ++++++++++++------- spec/lib/gitlab/git/repository_spec.rb | 13 ++++++ 3 files changed, 46 insertions(+), 15 deletions(-) create mode 100644 changelogs/unreleased/dm-gitmodules-parsing.yml diff --git a/changelogs/unreleased/dm-gitmodules-parsing.yml b/changelogs/unreleased/dm-gitmodules-parsing.yml new file mode 100644 index 00000000000..a7d755d6c4d --- /dev/null +++ b/changelogs/unreleased/dm-gitmodules-parsing.yml @@ -0,0 +1,4 @@ +--- +title: Make .gitmodules parsing more resilient to syntax errors +merge_request: +author: diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index b9f1ac144b6..c3bf32ca1cc 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1008,25 +1008,34 @@ module Gitlab def parse_gitmodules(commit, content) results = {} - current = "" - content.split("\n").each do |txt| - if txt =~ /^\s*\[/ - current = txt.match(/(?<=").*(?=")/)[0] - results[current] = {} - else - next unless results[current] - match_data = txt.match(/(\w+)\s*=\s*(.*)/) - next unless match_data - target = match_data[2].chomp - results[current][match_data[1]] = target + name = nil + entry = nil + content.each_line do |line| + case line.strip + when /\A\[submodule "(?[^"]+)"\]\z/ # Submodule header + name = $~[:name] + entry = results[name] = {} + when /\A(?\w+)\s*=\s*(?.*)\z/ # Key/value pair + key = $~[:key] + value = $~[:value].chomp - if match_data[1] == "path" + next unless name && entry + + entry[key] = value + + if key == 'path' begin - results[current]["id"] = blob_content(commit, target) + entry['id'] = blob_content(commit, value) rescue InvalidBlobName - results.delete(current) + # The current entry is invalid + results.delete(name) + name = entry = nil end end + when /\A#/ # Comment + next + else # Invalid line + name = entry = nil end end @@ -1086,7 +1095,12 @@ module Gitlab elsif tmp_entry.nil? return nil else - tmp_entry = rugged.lookup(tmp_entry[:oid]) + begin + tmp_entry = rugged.lookup(tmp_entry[:oid]) + rescue Rugged::OdbError, Rugged::InvalidError, Rugged::ReferenceError + return nil + end + return nil unless tmp_entry.type == :tree tmp_entry = tmp_entry[dir] end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index cb107c6d1f9..9d0e95d5b19 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -381,6 +381,19 @@ describe Gitlab::Git::Repository, seed_helper: true do } ]) end + + it 'should not break on invalid syntax' do + allow(repository).to receive(:blob_content).and_return(<<-GITMODULES.strip_heredoc) + [submodule "six"] + path = six + url = git://github.com/randx/six.git + + [submodule] + foo = bar + GITMODULES + + expect(submodules).to have_key('six') + end end context 'where repo doesn\'t have submodules' do From 161af17c1b69e7e00aefcd4f540a55755259ceda Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 24 May 2017 15:13:51 +0200 Subject: [PATCH 2/7] Introduce source to pipeline entity --- .../projects/pipelines_controller.rb | 2 +- app/models/ci/pipeline.rb | 15 ++++++++--- app/models/project.rb | 5 ---- app/serializers/pipeline_entity.rb | 2 +- app/services/ci/create_pipeline_service.rb | 3 ++- .../ci/create_trigger_request_service.rb | 2 +- app/services/git_push_service.rb | 2 +- app/services/git_tag_push_service.rb | 2 +- app/workers/pipeline_schedule_worker.rb | 2 +- .../introduce-source-to-pipelines.yml | 4 +++ db/fixtures/development/14_pipelines.rb | 2 +- db/fixtures/development/17_cycle_analytics.rb | 2 +- ...0170524125940_add_source_to_ci_pipeline.rb | 9 +++++++ db/schema.rb | 3 ++- features/steps/project/pages.rb | 2 +- lib/api/commit_statuses.rb | 9 ++++++- lib/api/pipelines.rb | 2 +- spec/factories/ci/pipelines.rb | 1 + .../projects/pipelines/pipelines_spec.rb | 2 ++ .../import_export/safe_model_attributes.yml | 1 + spec/models/ci/pipeline_spec.rb | 26 +++++++++++++++++-- spec/requests/api/commit_statuses_spec.rb | 4 +-- spec/requests/api/commits_spec.rb | 4 +-- spec/requests/api/v3/commits_spec.rb | 4 +-- spec/serializers/pipeline_entity_spec.rb | 4 +-- .../ci/create_pipeline_service_spec.rb | 5 ++-- .../ci/create_trigger_request_service_spec.rb | 2 ++ spec/services/git_push_service_spec.rb | 13 ++++++++++ spec/services/git_tag_push_service_spec.rb | 14 ++++++++++ spec/workers/pipeline_schedule_worker_spec.rb | 3 ++- 30 files changed, 116 insertions(+), 35 deletions(-) create mode 100644 changelogs/unreleased/introduce-source-to-pipelines.yml create mode 100644 db/migrate/20170524125940_add_source_to_ci_pipeline.rb diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index 602d3dd8c1c..87ec0df257a 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -58,7 +58,7 @@ class Projects::PipelinesController < Projects::ApplicationController def create @pipeline = Ci::CreatePipelineService .new(project, current_user, create_params) - .execute(ignore_skip_ci: true, save_on_errors: false) + .execute(:web, ignore_skip_ci: true, save_on_errors: false) if @pipeline.persisted? redirect_to namespace_project_pipeline_path(project.namespace, project, @pipeline) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 81c30b0e077..425ca9278eb 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -30,6 +30,7 @@ module Ci delegate :id, to: :project, prefix: true + validates :source, exclusion: { in: %w(unknown), unless: :importing? }, on: :create validates :sha, presence: { unless: :importing? } validates :ref, presence: { unless: :importing? } validates :status, presence: { unless: :importing? } @@ -37,6 +38,16 @@ module Ci after_create :keep_around_commits, unless: :importing? + enum source: { + unknown: nil, + push: 1, + web: 2, + trigger: 3, + schedule: 4, + api: 5, + external: 6 + } + state_machine :status, initial: :created do event :enqueue do transition created: :pending @@ -269,10 +280,6 @@ module Ci commit.sha == sha end - def triggered? - trigger_requests.any? - end - def retried @retried ||= (statuses.order(id: :desc) - statuses.latest) end diff --git a/app/models/project.rb b/app/models/project.rb index a59095cb25c..6d0b722319c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1064,11 +1064,6 @@ class Project < ActiveRecord::Base pipelines.order(id: :desc).find_by(sha: sha, ref: ref) end - def ensure_pipeline(ref, sha, current_user = nil) - pipeline_for(ref, sha) || - pipelines.create(sha: sha, ref: ref, user: current_user) - end - def enable_ci project_feature.update_attribute(:builds_access_level, ProjectFeature::ENABLED) end diff --git a/app/serializers/pipeline_entity.rb b/app/serializers/pipeline_entity.rb index ea57cc97a7e..486f8c36fbd 100644 --- a/app/serializers/pipeline_entity.rb +++ b/app/serializers/pipeline_entity.rb @@ -5,6 +5,7 @@ class PipelineEntity < Grape::Entity expose :user, using: UserEntity expose :active?, as: :active expose :coverage + expose :source expose :path do |pipeline| namespace_project_pipeline_path( @@ -24,7 +25,6 @@ class PipelineEntity < Grape::Entity expose :flags do expose :latest?, as: :latest - expose :triggered?, as: :triggered expose :stuck?, as: :stuck expose :has_yaml_errors?, as: :yaml_errors expose :can_retry?, as: :retryable diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index a98b7167765..8227a78a650 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -2,8 +2,9 @@ module Ci class CreatePipelineService < BaseService attr_reader :pipeline - def execute(ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil) + def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil) @pipeline = Ci::Pipeline.new( + source: source, project: project, ref: ref, sha: sha, diff --git a/app/services/ci/create_trigger_request_service.rb b/app/services/ci/create_trigger_request_service.rb index 8362f01ddb8..beb27a5a597 100644 --- a/app/services/ci/create_trigger_request_service.rb +++ b/app/services/ci/create_trigger_request_service.rb @@ -4,7 +4,7 @@ module Ci trigger_request = trigger.trigger_requests.create(variables: variables) pipeline = Ci::CreatePipelineService.new(project, trigger.owner, ref: ref). - execute(ignore_skip_ci: true, trigger_request: trigger_request) + execute(:trigger, ignore_skip_ci: true, trigger_request: trigger_request) trigger_request if pipeline.persisted? end diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb index d22236b961b..f080e6326a1 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -106,7 +106,7 @@ class GitPushService < BaseService EventCreateService.new.push(@project, current_user, build_push_data) @project.execute_hooks(build_push_data.dup, :push_hooks) @project.execute_services(build_push_data.dup, :push_hooks) - Ci::CreatePipelineService.new(@project, current_user, build_push_data).execute + Ci::CreatePipelineService.new(@project, current_user, build_push_data).execute(:push) if push_remove_branch? AfterBranchDeleteService diff --git a/app/services/git_tag_push_service.rb b/app/services/git_tag_push_service.rb index 96432837481..7c424fba428 100644 --- a/app/services/git_tag_push_service.rb +++ b/app/services/git_tag_push_service.rb @@ -11,7 +11,7 @@ class GitTagPushService < BaseService SystemHooksService.new.execute_hooks(build_system_push_data.dup, :tag_push_hooks) project.execute_hooks(@push_data.dup, :tag_push_hooks) project.execute_services(@push_data.dup, :tag_push_hooks) - Ci::CreatePipelineService.new(project, current_user, @push_data).execute + Ci::CreatePipelineService.new(project, current_user, @push_data).execute(:push) ProjectCacheWorker.perform_async(project.id, [], [:commit_count, :repository_size]) true diff --git a/app/workers/pipeline_schedule_worker.rb b/app/workers/pipeline_schedule_worker.rb index 7eb0e84acb2..7b485b3363c 100644 --- a/app/workers/pipeline_schedule_worker.rb +++ b/app/workers/pipeline_schedule_worker.rb @@ -14,7 +14,7 @@ class PipelineScheduleWorker Ci::CreatePipelineService.new(schedule.project, schedule.owner, ref: schedule.ref) - .execute(save_on_errors: false, schedule: schedule) + .execute(:schedule, save_on_errors: false, schedule: schedule) rescue => e Rails.logger.error "#{schedule.id}: Failed to create a scheduled pipeline: #{e.message}" ensure diff --git a/changelogs/unreleased/introduce-source-to-pipelines.yml b/changelogs/unreleased/introduce-source-to-pipelines.yml new file mode 100644 index 00000000000..7898bd31b39 --- /dev/null +++ b/changelogs/unreleased/introduce-source-to-pipelines.yml @@ -0,0 +1,4 @@ +--- +title: Introduce source to Pipeline entity +merge_request: +author: diff --git a/db/fixtures/development/14_pipelines.rb b/db/fixtures/development/14_pipelines.rb index 3c42f7db6d5..68767f0e585 100644 --- a/db/fixtures/development/14_pipelines.rb +++ b/db/fixtures/development/14_pipelines.rb @@ -98,7 +98,7 @@ class Gitlab::Seeder::Pipelines def create_pipeline!(project, ref, commit) - project.pipelines.create(sha: commit.id, ref: ref) + project.pipelines.create(sha: commit.id, ref: ref, source: :push) end def build_create!(pipeline, opts = {}) diff --git a/db/fixtures/development/17_cycle_analytics.rb b/db/fixtures/development/17_cycle_analytics.rb index 0d7eb1a7c93..75457b2d369 100644 --- a/db/fixtures/development/17_cycle_analytics.rb +++ b/db/fixtures/development/17_cycle_analytics.rb @@ -190,7 +190,7 @@ class Gitlab::Seeder::CycleAnalytics service = Ci::CreatePipelineService.new(merge_request.project, @user, ref: "refs/heads/#{merge_request.source_branch}") - pipeline = service.execute(ignore_skip_ci: true, save_on_errors: false) + pipeline = service.execute(:push, ignore_skip_ci: true, save_on_errors: false) pipeline.run! Timecop.travel rand(1..6).hours.from_now diff --git a/db/migrate/20170524125940_add_source_to_ci_pipeline.rb b/db/migrate/20170524125940_add_source_to_ci_pipeline.rb new file mode 100644 index 00000000000..1fa3d48037b --- /dev/null +++ b/db/migrate/20170524125940_add_source_to_ci_pipeline.rb @@ -0,0 +1,9 @@ +class AddSourceToCiPipeline < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_pipelines, :source, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 4c73f74ef1f..7371ce8847f 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: 20170524125940) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -283,6 +283,7 @@ ActiveRecord::Schema.define(version: 20170523091700) do t.integer "lock_version" t.integer "auto_canceled_by_id" t.integer "pipeline_schedule_id" + t.integer "source" end add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree diff --git a/features/steps/project/pages.rb b/features/steps/project/pages.rb index fea82d9fb57..4e6830f738b 100644 --- a/features/steps/project/pages.rb +++ b/features/steps/project/pages.rb @@ -35,7 +35,7 @@ class Spinach::Features::ProjectPages < Spinach::FeatureSteps end step 'pages are deployed' do - pipeline = @project.ensure_pipeline('HEAD', @project.commit('HEAD').sha) + pipeline = @project.pipelines.create(ref: 'HEAD', sha: @project.commit('HEAD').sha) build = build(:ci_build, project: @project, pipeline: pipeline, diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 827a38d33da..10f2d5ef6a3 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -68,7 +68,14 @@ module API name = params[:name] || params[:context] || 'default' - pipeline = @project.ensure_pipeline(ref, commit.sha, current_user) + pipeline = @project.pipeline_for(ref, commit.sha) + unless pipeline + pipeline = @project.pipelines.create!( + source: :external, + sha: commit.sha, + ref: ref, + user: current_user) + end status = GenericCommitStatus.running_or_pending.find_or_initialize_by( project: @project, diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 9117704aa46..e505cae3992 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -47,7 +47,7 @@ module API new_pipeline = Ci::CreatePipelineService.new(user_project, current_user, declared_params(include_missing: false)) - .execute(ignore_skip_ci: true, save_on_errors: false) + .execute(:api, ignore_skip_ci: true, save_on_errors: false) if new_pipeline.persisted? present new_pipeline, with: Entities::Pipeline else diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb index 361c5b9a49e..03e3c62effe 100644 --- a/spec/factories/ci/pipelines.rb +++ b/spec/factories/ci/pipelines.rb @@ -1,5 +1,6 @@ FactoryGirl.define do factory :ci_empty_pipeline, class: Ci::Pipeline do + source :push ref 'master' sha '97de212e80737a608d939f648d959671fb0a0142' status 'pending' diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index a97a92aa64f..05c2bf350f1 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -442,6 +442,8 @@ describe 'Pipelines', :feature, :js do it 'creates a new pipeline' do expect { click_on 'Create pipeline' } .to change { Ci::Pipeline.count }.by(1) + + expect(Ci::Pipeline.last).to be_web end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 96054c996fd..54ce8051f30 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -191,6 +191,7 @@ Ci::Pipeline: - lock_version - auto_canceled_by_id - pipeline_schedule_id +- source CommitStatus: - id - project_id diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index c8023dc13b1..ae1b01b76ab 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -21,13 +21,35 @@ describe Ci::Pipeline, models: true do it { is_expected.to have_many(:auto_canceled_pipelines) } it { is_expected.to have_many(:auto_canceled_jobs) } - it { is_expected.to validate_presence_of :sha } - it { is_expected.to validate_presence_of :status } + it { is_expected.to validate_presence_of(:sha) } + it { is_expected.to validate_presence_of(:status) } it { is_expected.to respond_to :git_author_name } it { is_expected.to respond_to :git_author_email } it { is_expected.to respond_to :short_sha } + describe '#source' do + context 'when creating new pipeline' do + let(:pipeline) do + build(:ci_empty_pipeline, status: :created, project: project, source: nil) + end + + it "prevents from creating an object" do + expect(pipeline).not_to be_valid + end + end + + context 'when updating existing pipeline' do + before do + pipeline.update_attribute(:source, nil) + end + + it "object is valid" do + expect(pipeline).to be_valid + end + end + end + describe '#block' do it 'changes pipeline status to manual' do expect(pipeline.block).to be true diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index 1c163cee152..6b637a03b6f 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -16,8 +16,8 @@ describe API::CommitStatuses do let(:get_url) { "/projects/#{project.id}/repository/commits/#{sha}/statuses" } context 'ci commit exists' do - let!(:master) { project.pipelines.create(sha: commit.id, ref: 'master') } - let!(:develop) { project.pipelines.create(sha: commit.id, ref: 'develop') } + let!(:master) { project.pipelines.create(source: :push, sha: commit.id, ref: 'master') } + let!(:develop) { project.pipelines.create(source: :push, sha: commit.id, ref: 'develop') } context "reporter user" do let(:statuses_id) { json_response.map { |status| status['id'] } } diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index b84361d3abd..b0c265b6453 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -485,7 +485,7 @@ describe API::Commits do end it "returns status for CI" do - pipeline = project.ensure_pipeline('master', project.repository.commit.sha) + pipeline = project.pipelines.create(source: :push, ref: 'master', sha: project.repository.commit.sha) pipeline.update(status: 'success') get api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user) @@ -495,7 +495,7 @@ describe API::Commits do end it "returns status for CI when pipeline is created" do - project.ensure_pipeline('master', project.repository.commit.sha) + project.pipelines.create(source: :push, ref: 'master', sha: project.repository.commit.sha) get api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user) diff --git a/spec/requests/api/v3/commits_spec.rb b/spec/requests/api/v3/commits_spec.rb index 386f60065ad..4a4a5dc5c7c 100644 --- a/spec/requests/api/v3/commits_spec.rb +++ b/spec/requests/api/v3/commits_spec.rb @@ -386,7 +386,7 @@ describe API::V3::Commits do end it "returns status for CI" do - pipeline = project.ensure_pipeline('master', project.repository.commit.sha) + pipeline = project.pipelines.create(source: :push, ref: 'master', sha: project.repository.commit.sha) pipeline.update(status: 'success') get v3_api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user) @@ -396,7 +396,7 @@ describe API::V3::Commits do end it "returns status for CI when pipeline is created" do - project.ensure_pipeline('master', project.repository.commit.sha) + project.pipelines.create(source: :push, ref: 'master', sha: project.repository.commit.sha) get v3_api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user) diff --git a/spec/serializers/pipeline_entity_spec.rb b/spec/serializers/pipeline_entity_spec.rb index d2482ac434b..88ec4ed2952 100644 --- a/spec/serializers/pipeline_entity_spec.rb +++ b/spec/serializers/pipeline_entity_spec.rb @@ -19,7 +19,7 @@ describe PipelineEntity do let(:pipeline) { create(:ci_empty_pipeline) } it 'contains required fields' do - expect(subject).to include :id, :user, :path, :coverage + expect(subject).to include :id, :user, :path, :coverage, :source expect(subject).to include :ref, :commit expect(subject).to include :updated_at, :created_at end @@ -36,7 +36,7 @@ describe PipelineEntity do it 'contains flags' do expect(subject).to include :flags expect(subject[:flags]) - .to include :latest, :triggered, :stuck, + .to include :latest, :stuck, :yaml_errors, :retryable, :cancelable end end diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 030912b9f45..8bf02f56282 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -9,13 +9,13 @@ describe Ci::CreatePipelineService, services: true do end describe '#execute' do - def execute_service(after: project.commit.id, message: 'Message', ref: 'refs/heads/master') + def execute_service(source: :push, after: project.commit.id, message: 'Message', ref: 'refs/heads/master') params = { ref: ref, before: '00000000', after: after, commits: [{ message: message }] } - described_class.new(project, user, params).execute + described_class.new(project, user, params).execute(source) end context 'valid params' do @@ -30,6 +30,7 @@ describe Ci::CreatePipelineService, services: true do it 'creates a pipeline' do expect(pipeline).to be_kind_of(Ci::Pipeline) expect(pipeline).to be_valid + expect(pipeline).to be_push expect(pipeline).to eq(project.pipelines.last) expect(pipeline).to have_attributes(user: user) expect(pipeline).to have_attributes(status: 'pending') diff --git a/spec/services/ci/create_trigger_request_service_spec.rb b/spec/services/ci/create_trigger_request_service_spec.rb index 5a20102872a..f2956262f4b 100644 --- a/spec/services/ci/create_trigger_request_service_spec.rb +++ b/spec/services/ci/create_trigger_request_service_spec.rb @@ -16,6 +16,7 @@ describe Ci::CreateTriggerRequestService, services: true do context 'without owner' do it { expect(subject).to be_kind_of(Ci::TriggerRequest) } it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) } + it { expect(subject.pipeline).to be_trigger } it { expect(subject.builds.first).to be_kind_of(Ci::Build) } end @@ -25,6 +26,7 @@ describe Ci::CreateTriggerRequestService, services: true do it { expect(subject).to be_kind_of(Ci::TriggerRequest) } it { expect(subject.pipeline).to be_kind_of(Ci::Pipeline) } + it { expect(subject.pipeline).to be_trigger } it { expect(subject.pipeline.user).to eq(owner) } it { expect(subject.builds.first).to be_kind_of(Ci::Build) } it { expect(subject.builds.first.user).to eq(owner) } diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 9f5a8beac16..bcd1fb64ab9 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -131,6 +131,19 @@ describe GitPushService, services: true do end end + describe "Pipelines" do + subject { execute_service(project, user, @oldrev, @newrev, @ref) } + + before do + stub_ci_pipeline_to_return_yaml_file + end + + it "creates a new pipeline" do + expect{ subject }.to change{ Ci::Pipeline.count } + expect(Ci::Pipeline.last).to be_push + end + end + describe "Push Event" do before do service = execute_service(project, user, @oldrev, @newrev, @ref ) diff --git a/spec/services/git_tag_push_service_spec.rb b/spec/services/git_tag_push_service_spec.rb index b73beb3f6fc..1fdcb420a8b 100644 --- a/spec/services/git_tag_push_service_spec.rb +++ b/spec/services/git_tag_push_service_spec.rb @@ -30,6 +30,20 @@ describe GitTagPushService, services: true do end end + describe "Pipelines" do + subject { service.execute } + + before do + stub_ci_pipeline_to_return_yaml_file + project.team << [user, :developer] + end + + it "creates a new pipeline" do + expect{ subject }.to change{ Ci::Pipeline.count } + expect(Ci::Pipeline.last).to be_push + end + end + describe "Git Tag Push Data" do subject { @push_data } let(:tag) { project.repository.find_tag(tag_name) } diff --git a/spec/workers/pipeline_schedule_worker_spec.rb b/spec/workers/pipeline_schedule_worker_spec.rb index 9c650354d72..14ed8b7811e 100644 --- a/spec/workers/pipeline_schedule_worker_spec.rb +++ b/spec/workers/pipeline_schedule_worker_spec.rb @@ -23,7 +23,8 @@ describe PipelineScheduleWorker do context 'when there is a scheduled pipeline within next_run_at' do it 'creates a new pipeline' do - expect { subject }.to change { project.pipelines.count }.by(1) + expect{ subject }.to change { project.pipelines.count }.by(1) + expect(Ci::Pipeline.last).to be_schedule end it 'updates the next_run_at field' do From ea7269e4c66a51eb4c66b9022ba4c2b35d8ad012 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 31 May 2017 11:19:56 -0500 Subject: [PATCH 3/7] Remove entry variable --- lib/gitlab/git/repository.rb | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index c3bf32ca1cc..9d6adbdb4ac 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1006,40 +1006,39 @@ module Gitlab # Parses the contents of a .gitmodules file and returns a hash of # submodule information. def parse_gitmodules(commit, content) - results = {} + modules = {} name = nil - entry = nil content.each_line do |line| case line.strip when /\A\[submodule "(?[^"]+)"\]\z/ # Submodule header name = $~[:name] - entry = results[name] = {} + modules[name] = {} when /\A(?\w+)\s*=\s*(?.*)\z/ # Key/value pair key = $~[:key] value = $~[:value].chomp - next unless name && entry + next unless name && modules[name] - entry[key] = value + modules[name][key] = value if key == 'path' begin - entry['id'] = blob_content(commit, value) + modules[name]['id'] = blob_content(commit, value) rescue InvalidBlobName # The current entry is invalid - results.delete(name) - name = entry = nil + modules.delete(name) + name = nil end end when /\A#/ # Comment next else # Invalid line - name = entry = nil + name = nil end end - results + modules end # Returns true if +commit+ introduced changes to +path+, using commit From c3dd2d1117743cff15181caf1b12e35daa31a33f Mon Sep 17 00:00:00 2001 From: Tim Anglade Date: Wed, 31 May 2017 18:27:29 +0000 Subject: [PATCH 4/7] Add GitLab Resources to University --- doc/university/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/university/README.md b/doc/university/README.md index c1661f0b52b..399d54bcf23 100644 --- a/doc/university/README.md +++ b/doc/university/README.md @@ -65,6 +65,7 @@ The curriculum is composed of GitLab videos, screencasts, presentations, project 1. [Using Innersourcing to Improve Collaboration](https://about.gitlab.com/2014/09/05/innersourcing-using-the-open-source-workflow-to-improve-collaboration-within-an-organization/) 1. [The Software Development Market and GitLab - Video](https://www.youtube.com/watch?v=sXlhgPK1NTY&list=PLFGfElNsQthbQu_IWlNOxul0TbS_2JH-e&index=6) - [Slides](https://docs.google.com/presentation/d/1vCU-NbZWz8NTNK8Vu3y4zGMAHb5DpC8PE5mHtw1PWfI/edit) 1. [The GitLab Book Club](bookclub/index.md) +1. [GitLab Resources](https://about.gitlab.com/resources/) #### 1.7 Community and Support From 671284ba375109becbfa2a288032cdc7301b157b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 31 May 2017 21:06:01 +0000 Subject: [PATCH 5/7] Add feature toggles through Flipper --- Gemfile | 4 + Gemfile.lock | 6 + .../unreleased/feature-flags-flipper.yml | 4 + .../20170525174156_create_feature_tables.rb | 26 +++++ db/schema.rb | 20 +++- doc/api/features.md | 83 ++++++++++++++ doc/development/README.md | 1 + doc/development/feature_flags.md | 7 ++ lib/api/api.rb | 1 + lib/api/entities.rb | 22 ++++ lib/api/features.rb | 36 ++++++ lib/feature.rb | 41 +++++++ lib/gitlab/gitaly_client.rb | 22 +++- spec/lib/feature_spec.rb | 26 +++++ spec/lib/gitlab/gitaly_client_spec.rb | 82 +++++++++++++- spec/requests/api/features_spec.rb | 104 ++++++++++++++++++ spec/support/gitaly.rb | 3 +- 17 files changed, 483 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/feature-flags-flipper.yml create mode 100644 db/migrate/20170525174156_create_feature_tables.rb create mode 100644 doc/api/features.md create mode 100644 doc/development/feature_flags.md create mode 100644 lib/api/features.rb create mode 100644 lib/feature.rb create mode 100644 spec/lib/feature_spec.rb create mode 100644 spec/requests/api/features_spec.rb 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 From 0d55def4d3cf8f976fe09a05301afce828b95a96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 31 May 2017 21:17:30 +0000 Subject: [PATCH 6/7] Update GITLAB_SHELL_VERSION This change continues with the plan to change the parameters passed around GitLab components to allow for independence of mount points. See https://gitlab.com/gitlab-org/gitlab-shell/merge_requests/130#note_28747613 --- GITLAB_SHELL_VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index 2d6c0bcf19c..ab0fa336dd0 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -5.0.4 +5.0.5 From 18aebf0a989a8a6cee3ff20babd3c8d5d928f0ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Wed, 31 May 2017 17:18:41 -0400 Subject: [PATCH 7/7] Revert "Update GITLAB_SHELL_VERSION" This reverts commit 0d55def4d3cf8f976fe09a05301afce828b95a96. --- GITLAB_SHELL_VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index ab0fa336dd0..2d6c0bcf19c 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -5.0.5 +5.0.4