From 860760120843ea5ad003cc2f52b28cf0fc7c647b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 9 May 2016 11:18:47 +0200 Subject: [PATCH 01/41] Add config field to runner to lock it on project --- db/migrate/20160509091049_add_locked_to_ci_runner.rb | 8 ++++++++ db/schema.rb | 1 + 2 files changed, 9 insertions(+) create mode 100644 db/migrate/20160509091049_add_locked_to_ci_runner.rb diff --git a/db/migrate/20160509091049_add_locked_to_ci_runner.rb b/db/migrate/20160509091049_add_locked_to_ci_runner.rb new file mode 100644 index 00000000000..6294fa9d86b --- /dev/null +++ b/db/migrate/20160509091049_add_locked_to_ci_runner.rb @@ -0,0 +1,8 @@ +class AddLockedToCiRunner < ActiveRecord::Migration + ## + # Downtime expected due to exclusive lock when setting default value. + # + def change + add_column :ci_runners, :locked, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index b7adf48fdb4..6e60c39fc22 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -285,6 +285,7 @@ ActiveRecord::Schema.define(version: 20160608155312) do t.string "platform" t.string "architecture" t.boolean "run_untagged", default: true, null: false + t.boolean "locked", default: false, null: false end add_index "ci_runners", ["description"], name: "index_ci_runners_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} From 4f7f3258c18dfc207b838401f5ed71a3197eb22d Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 1 Jun 2016 18:34:20 +0800 Subject: [PATCH 02/41] Implement the logic for locking runner --- app/models/ci/build.rb | 4 +-- app/models/ci/runner.rb | 18 +++++++++++++ spec/models/build_spec.rb | 55 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index b8ada6361ac..860ac16eefd 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -291,9 +291,7 @@ module Ci end def can_be_served?(runner) - return false unless has_tags? || runner.run_untagged? - - (tag_list - runner.tag_list).empty? + runner.can_serve?(self) end def has_tags? diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index adb65292208..d61a8c00634 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -91,6 +91,12 @@ module Ci !shared? end + def can_serve?(build) + not_locked_or_locked_to?(build.project) && + run_untagged_or_has_tags?(build) && + accepting_tags?(build.tag_list) + end + def only_for?(project) projects == [project] end @@ -111,5 +117,17 @@ module Ci 'can not be empty when runner is not allowed to pick untagged jobs') end end + + def not_locked_or_locked_to?(project) + !locked? || projects.exists?(id: project.id) + end + + def run_untagged_or_has_tags?(build) + run_untagged? || build.has_tags? + end + + def accepting_tags?(target_tags) + (target_tags - tag_list).empty? + end end end diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb index 7660ea2659c..8cd1ffae9ce 100644 --- a/spec/models/build_spec.rb +++ b/spec/models/build_spec.rb @@ -296,16 +296,67 @@ describe Ci::Build, models: true do it_behaves_like 'tagged build picker' end - context 'when runner can not pick untagged jobs' do + context 'when runner cannot pick untagged jobs' do before { runner.run_untagged = false } - it 'can not handle builds without tags' do + it 'cannot handle builds without tags' do expect(build.can_be_served?(runner)).to be_falsey end it_behaves_like 'tagged build picker' end end + + context 'when runner is locked' do + before { runner.locked = true } + + shared_examples 'locked build picker' do |serve_matching_tags| + context 'when runner cannot pick untagged jobs' do + before { runner.run_untagged = false } + + it 'cannot handle builds without tags' do + expect(build.can_be_served?(runner)).to be_falsey + end + end + + context 'when having runner tags' do + before { runner.tag_list = ['bb', 'cc'] } + + it "#{serve_matching_tags} handle it for matching tags" do + build.tag_list = ['bb'] + expected = if serve_matching_tags + be_truthy + else + be_falsey + end + expect(build.can_be_served?(runner)).to expected + end + + it 'cannot handle it for builds without matching tags' do + build.tag_list = ['aa'] + expect(build.can_be_served?(runner)).to be_falsey + end + end + end + + context 'when serving the same project' do + it 'can handle it' do + expect(build.can_be_served?(runner)).to be_truthy + end + + it_behaves_like 'locked build picker', true + end + + context 'serving a different project' do + before { runner.runner_projects.destroy_all } + + it 'cannot handle it' do + expect(build.can_be_served?(runner)).to be_falsey + end + + it_behaves_like 'locked build picker', false + end + end end describe '#has_tags?' do From 0eeb4bed497e5f6ba2af558869803432bee65f74 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 2 Jun 2016 18:41:26 +0800 Subject: [PATCH 03/41] Introduced Ci::Runner.specific_for for getting specific runners: for a particular project. --- .../projects/runners_controller.rb | 3 +- app/models/ci/runner.rb | 4 ++ spec/models/ci/runner_spec.rb | 54 +++++++++++++++++++ 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 0b4fa572501..bc4c5bd4575 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -7,8 +7,7 @@ class Projects::RunnersController < Projects::ApplicationController def index @runners = project.runners.ordered @specific_runners = current_user.ci_authorized_runners. - where.not(id: project.runners). - ordered.page(params[:page]).per(20) + specific_for(project).ordered.page(params[:page]).per(20) @shared_runners = Ci::Runner.shared.active @shared_runners_count = @shared_runners.count(:all) end diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index d61a8c00634..5c42c94e4dc 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -26,6 +26,10 @@ module Ci .where("ci_runner_projects.gl_project_id = :project_id OR ci_runners.is_shared = true", project_id: project_id) end + scope :specific_for, ->(project) do + where(locked: false).where.not(id: project.runners).specific + end + validate :tag_constraints acts_as_taggable diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 5d04d8ffcff..0d7ce5a020a 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -112,6 +112,60 @@ describe Ci::Runner, models: true do end end + describe :specific_for do + let(:runner) { create(:ci_runner) } + let(:project) { create(:project) } + let(:another_project) { create(:project) } + + before { project.runners << runner } + + context 'with shared runners' do + before { runner.update(is_shared: true) } + + context 'should not give owned runner' do + subject { Ci::Runner.specific_for(project) } + + it { is_expected.to be_empty } + end + + context 'should not give shared runner' do + subject { Ci::Runner.specific_for(another_project) } + + it { is_expected.to be_empty } + end + end + + context 'with unlocked runner' do + context 'should not give owned runner' do + subject { Ci::Runner.specific_for(project) } + + it { is_expected.to be_empty } + end + + context 'should give a specific runner' do + subject { Ci::Runner.specific_for(another_project) } + + it { is_expected.to contain_exactly(runner) } + end + end + + context 'with locked runner' do + before { runner.update(locked: true) } + + context 'should not give owned runner' do + subject { Ci::Runner.specific_for(project) } + + it { is_expected.to be_empty } + end + + context 'should not give a locked runner' do + subject { Ci::Runner.specific_for(another_project) } + + it { is_expected.to be_empty } + end + end + end + describe "belongs_to_one_project?" do it "returns false if there are two projects runner assigned to" do runner = FactoryGirl.create(:ci_runner) From 1c302d566b45c83c0c768354b40e86ea0446dfe6 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 2 Jun 2016 19:06:01 +0800 Subject: [PATCH 04/41] WIP, try to add views for locked runners --- app/models/ci/runner.rb | 2 +- app/views/projects/runners/_form.html.haml | 6 ++++++ app/views/projects/runners/show.html.haml | 3 +++ lib/api/entities.rb | 1 + lib/api/runners.rb | 2 +- lib/ci/api/runners.rb | 3 ++- 6 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 5c42c94e4dc..7a3dfaa4e61 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -4,7 +4,7 @@ module Ci LAST_CONTACT_TIME = 5.minutes.ago AVAILABLE_SCOPES = %w[specific shared active paused online] - FORM_EDITABLE = %i[description tag_list active run_untagged] + FORM_EDITABLE = %i[description tag_list active run_untagged locked] has_many :builds, class_name: 'Ci::Build' has_many :runner_projects, dependent: :destroy, class_name: 'Ci::RunnerProject' diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index d62f5c8f131..67839fafd28 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -12,6 +12,12 @@ .checkbox = f.check_box :run_untagged %span.light Indicates whether this runner can pick jobs without tags + .form-group + = label :locked, 'Exclusive to this project', class: 'control-label' + .col-sm-10 + .checkbox + = f.check_box :locked + %span.light Indicates whether this runner can be enabled for other projects .form-group = label_tag :token, class: 'control-label' do Token diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index f24e1b9144e..fc6424402ae 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -22,6 +22,9 @@ %tr %td Can run untagged jobs %td= @runner.run_untagged? ? 'Yes' : 'No' + %tr + %td Exclusive to this project + %td= @runner.locked? ? 'Yes' : 'No' %tr %td Tags %td diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 50d69274b2e..16eeca8c8ac 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -413,6 +413,7 @@ module API class RunnerDetails < Runner expose :tag_list expose :run_untagged + expose :locked expose :version, :revision, :platform, :architecture expose :contacted_at expose :token, if: lambda { |runner, options| options[:current_user].is_admin? || !runner.is_shared? } diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 4faba9dc87b..2d09b6193d9 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -49,7 +49,7 @@ module API runner = get_runner(params[:id]) authenticate_update_runner!(runner) - attrs = attributes_for_keys [:description, :active, :tag_list, :run_untagged] + attrs = attributes_for_keys [:description, :active, :tag_list, :run_untagged, :locked] if runner.update(attrs) present runner, with: Entities::RunnerDetails, current_user: current_user else diff --git a/lib/ci/api/runners.rb b/lib/ci/api/runners.rb index 0c41f22c7c5..b4b7261fa3b 100644 --- a/lib/ci/api/runners.rb +++ b/lib/ci/api/runners.rb @@ -29,7 +29,8 @@ module Ci required_attributes! [:token] attributes = { description: params[:description], - tag_list: params[:tag_list] } + tag_list: params[:tag_list], + locked: !!params[:locked] } unless params[:run_untagged].nil? attributes[:run_untagged] = params[:run_untagged] From 35954572b898e5a58cd209276e1bff499990867b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 2 Jun 2016 23:28:14 +0800 Subject: [PATCH 05/41] Prefer do and end for before/after: Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4404#note_12217415 --- spec/models/ci/runner_spec.rb | 44 ++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 0d7ce5a020a..1e87d7751b8 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -40,7 +40,9 @@ describe Ci::Runner, models: true do let!(:project) { FactoryGirl.create :empty_project } let!(:shared_runner) { FactoryGirl.create(:ci_runner, :shared) } - before { shared_runner.assign_to(project) } + before do + shared_runner.assign_to(project) + end it { expect(shared_runner).to be_specific } it { expect(shared_runner.projects).to eq([project]) } @@ -64,19 +66,25 @@ describe Ci::Runner, models: true do subject { runner.online? } context 'never contacted' do - before { runner.contacted_at = nil } + before do + runner.contacted_at = nil + end it { is_expected.to be_falsey } end context 'contacted long time ago time' do - before { runner.contacted_at = 1.year.ago } + before do + runner.contacted_at = 1.year.ago + end it { is_expected.to be_falsey } end context 'contacted 1s ago' do - before { runner.contacted_at = 1.second.ago } + before do + runner.contacted_at = 1.second.ago + end it { is_expected.to be_truthy } end @@ -88,25 +96,33 @@ describe Ci::Runner, models: true do subject { runner.status } context 'never connected' do - before { runner.contacted_at = nil } + before do + runner.contacted_at = nil + end it { is_expected.to eq(:not_connected) } end context 'contacted 1s ago' do - before { runner.contacted_at = 1.second.ago } + before do + runner.contacted_at = 1.second.ago + end it { is_expected.to eq(:online) } end context 'contacted long time ago' do - before { runner.contacted_at = 1.year.ago } + before do + runner.contacted_at = 1.year.ago + end it { is_expected.to eq(:offline) } end context 'inactive' do - before { runner.active = false } + before do + runner.active = false + end it { is_expected.to eq(:paused) } end @@ -117,10 +133,14 @@ describe Ci::Runner, models: true do let(:project) { create(:project) } let(:another_project) { create(:project) } - before { project.runners << runner } + before do + project.runners << runner + end context 'with shared runners' do - before { runner.update(is_shared: true) } + before do + runner.update(is_shared: true) + end context 'should not give owned runner' do subject { Ci::Runner.specific_for(project) } @@ -150,7 +170,9 @@ describe Ci::Runner, models: true do end context 'with locked runner' do - before { runner.update(locked: true) } + before do + runner.update(locked: true) + end context 'should not give owned runner' do subject { Ci::Runner.specific_for(project) } From 2ea0148c55fbefee1137c254a94b37c05dca41cf Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 6 Jun 2016 18:20:30 +0800 Subject: [PATCH 06/41] Update migration with add_column_with_default: Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12278454 --- db/migrate/20160509091049_add_locked_to_ci_runner.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/db/migrate/20160509091049_add_locked_to_ci_runner.rb b/db/migrate/20160509091049_add_locked_to_ci_runner.rb index 6294fa9d86b..43376304bd9 100644 --- a/db/migrate/20160509091049_add_locked_to_ci_runner.rb +++ b/db/migrate/20160509091049_add_locked_to_ci_runner.rb @@ -2,7 +2,12 @@ class AddLockedToCiRunner < ActiveRecord::Migration ## # Downtime expected due to exclusive lock when setting default value. # - def change - add_column :ci_runners, :locked, :boolean, default: false, null: false + def up + add_column_with_default(:ci_runners, :locked, :boolean, + default: false, allow_null: false) + end + + def down + remove_column(:ci_runners, :locked) end end From 894d22f49a24377aad7e693d0343397bc349e412 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 6 Jun 2016 18:48:18 +0800 Subject: [PATCH 07/41] include the helper --- db/migrate/20160509091049_add_locked_to_ci_runner.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/db/migrate/20160509091049_add_locked_to_ci_runner.rb b/db/migrate/20160509091049_add_locked_to_ci_runner.rb index 43376304bd9..3fbaef3b7f0 100644 --- a/db/migrate/20160509091049_add_locked_to_ci_runner.rb +++ b/db/migrate/20160509091049_add_locked_to_ci_runner.rb @@ -1,7 +1,7 @@ class AddLockedToCiRunner < ActiveRecord::Migration - ## - # Downtime expected due to exclusive lock when setting default value. - # + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + def up add_column_with_default(:ci_runners, :locked, :boolean, default: false, allow_null: false) From 781d35c191be62366f6c7855f3314a9c371aa0e6 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 7 Jun 2016 14:50:38 +0800 Subject: [PATCH 08/41] Prefer attributes_for_keys so that it ignores nils Also add a test for setting locked. --- lib/ci/api/runners.rb | 10 +++------- spec/requests/api/runners_spec.rb | 6 ++++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/lib/ci/api/runners.rb b/lib/ci/api/runners.rb index b4b7261fa3b..bcc82969eb3 100644 --- a/lib/ci/api/runners.rb +++ b/lib/ci/api/runners.rb @@ -28,13 +28,9 @@ module Ci post "register" do required_attributes! [:token] - attributes = { description: params[:description], - tag_list: params[:tag_list], - locked: !!params[:locked] } - - unless params[:run_untagged].nil? - attributes[:run_untagged] = params[:run_untagged] - end + attributes = attributes_for_keys( + [:description, :tag_list, :run_untagged, :locked] + ) runner = if runner_registration_token_valid? diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 73ae8ef631c..f4f9c417bbb 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -187,14 +187,16 @@ describe API::Runners, api: true do update_runner(shared_runner.id, admin, description: "#{description}_updated", active: !active, tag_list: ['ruby2.1', 'pgsql', 'mysql'], - run_untagged: 'false') + run_untagged: 'false', + locked: 'true') shared_runner.reload expect(response.status).to eq(200) expect(shared_runner.description).to eq("#{description}_updated") expect(shared_runner.active).to eq(!active) expect(shared_runner.tag_list).to include('ruby2.1', 'pgsql', 'mysql') - expect(shared_runner.run_untagged?).to be false + expect(shared_runner.run_untagged?).to be(false) + expect(shared_runner.locked?).to be(true) end end From b08912dbd37eb05243448b2e310939c5679d61e3 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 7 Jun 2016 15:00:41 +0800 Subject: [PATCH 09/41] Use block for before/after as we preferred --- spec/models/build_spec.rb | 89 +++++++++++++++++++++++++++++---------- 1 file changed, 67 insertions(+), 22 deletions(-) diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb index 8cd1ffae9ce..1d4b8bc4c98 100644 --- a/spec/models/build_spec.rb +++ b/spec/models/build_spec.rb @@ -36,32 +36,44 @@ describe Ci::Build, models: true do subject { build.ignored? } context 'if build is not allowed to fail' do - before { build.allow_failure = false } + before do + build.allow_failure = false + end context 'and build.status is success' do - before { build.status = 'success' } + before do + build.status = 'success' + end it { is_expected.to be_falsey } end context 'and build.status is failed' do - before { build.status = 'failed' } + before do + build.status = 'failed' + end it { is_expected.to be_falsey } end end context 'if build is allowed to fail' do - before { build.allow_failure = true } + before do + build.allow_failure = true + end context 'and build.status is success' do - before { build.status = 'success' } + before do + build.status = 'success' + end it { is_expected.to be_falsey } end context 'and build.status is failed' do - before { build.status = 'failed' } + before do + build.status = 'failed' + end it { is_expected.to be_truthy } end @@ -75,7 +87,9 @@ describe Ci::Build, models: true do context 'if build.trace contains text' do let(:text) { 'example output' } - before { build.trace = text } + before do + build.trace = text + end it { is_expected.to include(text) } it { expect(subject.length).to be >= text.length } @@ -188,7 +202,9 @@ describe Ci::Build, models: true do ] end - before { build.update_attributes(stage: 'stage') } + before do + build.update_attributes(stage: 'stage') + end it { is_expected.to eq(predefined_variables + yaml_variables) } @@ -199,7 +215,9 @@ describe Ci::Build, models: true do ] end - before { build.update_attributes(tag: true) } + before do + build.update_attributes(tag: true) + end it { is_expected.to eq(tag_variable + predefined_variables + yaml_variables) } end @@ -260,7 +278,9 @@ describe Ci::Build, models: true do describe '#can_be_served?' do let(:runner) { create(:ci_runner) } - before { build.project.runners << runner } + before do + build.project.runners << runner + end context 'when runner does not have tags' do it 'can handle builds without tags' do @@ -274,7 +294,9 @@ describe Ci::Build, models: true do end context 'when runner has tags' do - before { runner.tag_list = ['bb', 'cc'] } + before do + runner.tag_list = ['bb', 'cc'] + end shared_examples 'tagged build picker' do it 'can handle build with matching tags' do @@ -297,7 +319,9 @@ describe Ci::Build, models: true do end context 'when runner cannot pick untagged jobs' do - before { runner.run_untagged = false } + before do + runner.run_untagged = false + end it 'cannot handle builds without tags' do expect(build.can_be_served?(runner)).to be_falsey @@ -308,11 +332,15 @@ describe Ci::Build, models: true do end context 'when runner is locked' do - before { runner.locked = true } + before do + runner.locked = true + end shared_examples 'locked build picker' do |serve_matching_tags| context 'when runner cannot pick untagged jobs' do - before { runner.run_untagged = false } + before do + runner.run_untagged = false + end it 'cannot handle builds without tags' do expect(build.can_be_served?(runner)).to be_falsey @@ -320,7 +348,9 @@ describe Ci::Build, models: true do end context 'when having runner tags' do - before { runner.tag_list = ['bb', 'cc'] } + before do + runner.tag_list = ['bb', 'cc'] + end it "#{serve_matching_tags} handle it for matching tags" do build.tag_list = ['bb'] @@ -348,7 +378,9 @@ describe Ci::Build, models: true do end context 'serving a different project' do - before { runner.runner_projects.destroy_all } + before do + runner.runner_projects.destroy_all + end it 'cannot handle it' do expect(build.can_be_served?(runner)).to be_falsey @@ -411,7 +443,9 @@ describe Ci::Build, models: true do %w(pending).each do |state| context "if commit_status.status is #{state}" do - before { build.status = state } + before do + build.status = state + end it { is_expected.to be_truthy } @@ -430,7 +464,9 @@ describe Ci::Build, models: true do %w(success failed canceled running).each do |state| context "if commit_status.status is #{state}" do - before { build.status = state } + before do + build.status = state + end it { is_expected.to be_falsey } end @@ -441,7 +477,10 @@ describe Ci::Build, models: true do subject { build.artifacts? } context 'artifacts archive does not exist' do - before { build.update_attributes(artifacts_file: nil) } + before do + build.update_attributes(artifacts_file: nil) + end + it { is_expected.to be_falsy } end @@ -606,7 +645,9 @@ describe Ci::Build, models: true do let!(:build) { create(:ci_build, :trace, :success, :artifacts) } describe '#erase' do - before { build.erase(erased_by: user) } + before do + build.erase(erased_by: user) + end context 'erased by user' do let!(:user) { create(:user, username: 'eraser') } @@ -643,7 +684,9 @@ describe Ci::Build, models: true do end context 'build has been erased' do - before { build.erase } + before do + build.erase + end it { is_expected.to be true } end @@ -651,7 +694,9 @@ describe Ci::Build, models: true do context 'metadata and build trace are not available' do let!(:build) { create(:ci_build, :success, :artifacts) } - before { build.remove_artifacts_metadata! } + before do + build.remove_artifacts_metadata! + end describe '#erase' do it 'should not raise error' do From fc51bf324826799ec43c8a0b59ad94f0220df8e2 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 7 Jun 2016 15:58:44 +0800 Subject: [PATCH 10/41] Found a workaround for that weird SQL error: Without that, Rails would complain: > bind message supplies 5 parameters, but prepared statement "a4" requires 4 However without this workaround it would still work in Rails console, so I still think this is a Rails bug somewhere. --- app/models/ci/runner.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 7a3dfaa4e61..6565a12fe62 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -27,7 +27,9 @@ module Ci end scope :specific_for, ->(project) do - where(locked: false).where.not(id: project.runners).specific + # TODO: That `to_sql` is needed to workaround a weird Rails bug. + # Without that, placeholders would miss one and couldn't match. + where(locked: false).where.not(id: project.runners.to_sql).specific end validate :tag_constraints From 5bd077bd41653ead0925fd775fd07ba3fd26468f Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 7 Jun 2016 17:43:18 +0800 Subject: [PATCH 11/41] Manually build the SQL so that it properly skips Rails. --- app/models/ci/runner.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 6565a12fe62..bb5340a0f03 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -29,7 +29,8 @@ module Ci scope :specific_for, ->(project) do # TODO: That `to_sql` is needed to workaround a weird Rails bug. # Without that, placeholders would miss one and couldn't match. - where(locked: false).where.not(id: project.runners.to_sql).specific + where(locked: false). + where.not("id IN (#{project.runners.select(:id).to_sql})").specific end validate :tag_constraints From 7fa80a79cdf91782f68794e5a5b879abf6d986dc Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 7 Jun 2016 22:42:07 +0800 Subject: [PATCH 12/41] Fixed a typo (remove extra n) --- doc/ci/runners/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index b42d7a62ebc..40dc13d53b1 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -66,7 +66,7 @@ Now simply register the runner as any runner: sudo gitlab-runner register ``` -Shared runners are enabled by default as of GitLab 8.2, but can be disabled with the +Shared runners are enabled by default as of GitLab 8.2, but can be disabled with the `DISABLE SHARED RUNNERS` button. Previous versions of GitLab defaulted shared runners to disabled. @@ -128,7 +128,7 @@ the appropriate dependencies to run Rails test suites. ### Prevent runner with tags from picking jobs without tags You can configure a runner to prevent it from picking jobs with tags when -the runnner does not have tags assigned. This setting is available on each +the runner does not have tags assigned. This setting is available on each runner in *Project Settings* > *Runners*. ### Be careful with sensitive information From 9e14ebe6bce26fc16b7310b61870f41747e83d14 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 7 Jun 2016 22:48:16 +0800 Subject: [PATCH 13/41] Update CHANGELOG and doc for locked runner feature --- CHANGELOG | 1 + doc/ci/runners/README.md | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 0ea307a3713..a23eda6c026 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -39,6 +39,7 @@ v 8.9.0 (unreleased) - Projects pending deletion will render a 404 page - Measure queue duration between gitlab-workhorse and Rails - Make authentication service for Container Registry to be compatible with < Docker 1.11 + - Make it possible to lock a runner from being enabled for other projects - Add Application Setting to configure Container Registry token expire delay (default 5min) - Cache assigned issue and merge request counts in sidebar nav - Use Knapsack only in CI environment diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index 40dc13d53b1..bd654e655d6 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -96,6 +96,12 @@ To register the runner, run the command below and follow instructions: sudo gitlab-runner register ``` +### Lock a specific runner from being enabled from other projects + +You can configure a runner to prevent it from being enabled for the other +projects, so it's exclusive to current projects. This setting is available +on each runner in *Project Settings* > *Runners*. + ### Making an existing Shared Runner Specific If you are an admin on your GitLab instance, From de04004803b76cb9945ef027d04968cef7b41b2b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 8 Jun 2016 14:46:41 +0800 Subject: [PATCH 14/41] Prefer string for describe, feedback from: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12321029 --- spec/models/ci/runner_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 1e87d7751b8..c3b715dcb92 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -36,7 +36,7 @@ describe Ci::Runner, models: true do end end - describe :assign_to do + describe '#assign_to' do let!(:project) { FactoryGirl.create :empty_project } let!(:shared_runner) { FactoryGirl.create(:ci_runner, :shared) } @@ -49,7 +49,7 @@ describe Ci::Runner, models: true do it { expect(shared_runner.only_for?(project)).to be_truthy } end - describe :online do + describe '.online' do subject { Ci::Runner.online } before do @@ -60,7 +60,7 @@ describe Ci::Runner, models: true do it { is_expected.to eq([@runner2])} end - describe :online? do + describe '#online?' do let(:runner) { FactoryGirl.create(:ci_runner, :shared) } subject { runner.online? } @@ -90,7 +90,7 @@ describe Ci::Runner, models: true do end end - describe :status do + describe '#status' do let(:runner) { FactoryGirl.create(:ci_runner, :shared, contacted_at: 1.second.ago) } subject { runner.status } @@ -128,7 +128,7 @@ describe Ci::Runner, models: true do end end - describe :specific_for do + describe '.specific_for' do let(:runner) { create(:ci_runner) } let(:project) { create(:project) } let(:another_project) { create(:project) } From 268f7713a9c076204a8b6badb847d2e23bafdd71 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 8 Jun 2016 15:19:49 +0800 Subject: [PATCH 15/41] Remove Build#can_be_served? and rename Runner#can_serve? to can_pick? This also moves tests from build_spec.rb to runner_spec.rb --- app/models/ci/build.rb | 6 +- app/models/ci/runner.rb | 2 +- app/services/ci/register_build_service.rb | 2 +- spec/models/build_spec.rb | 118 +-------------------- spec/models/ci/runner_spec.rb | 119 ++++++++++++++++++++++ 5 files changed, 123 insertions(+), 124 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 860ac16eefd..a3a30fe17c2 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -290,16 +290,12 @@ module Ci project.valid_runners_token? token end - def can_be_served?(runner) - runner.can_serve?(self) - end - def has_tags? tag_list.any? end def any_runners_online? - project.any_runners? { |runner| runner.active? && runner.online? && can_be_served?(runner) } + project.any_runners? { |runner| runner.active? && runner.online? && runner.can_pick?(self) } end def stuck? diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index bb5340a0f03..44f820d100b 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -98,7 +98,7 @@ module Ci !shared? end - def can_serve?(build) + def can_pick?(build) not_locked_or_locked_to?(build.project) && run_untagged_or_has_tags?(build) && accepting_tags?(build.tag_list) diff --git a/app/services/ci/register_build_service.rb b/app/services/ci/register_build_service.rb index 4ff268a6f06..511505bc9a9 100644 --- a/app/services/ci/register_build_service.rb +++ b/app/services/ci/register_build_service.rb @@ -17,7 +17,7 @@ module Ci builds = builds.order('created_at ASC') build = builds.find do |build| - build.can_be_served?(current_runner) + current_runner.can_pick?(build) end if build diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb index 1d4b8bc4c98..e703a07013b 100644 --- a/spec/models/build_spec.rb +++ b/spec/models/build_spec.rb @@ -275,122 +275,6 @@ describe Ci::Build, models: true do end end - describe '#can_be_served?' do - let(:runner) { create(:ci_runner) } - - before do - build.project.runners << runner - end - - context 'when runner does not have tags' do - it 'can handle builds without tags' do - expect(build.can_be_served?(runner)).to be_truthy - end - - it 'cannot handle build with tags' do - build.tag_list = ['aa'] - expect(build.can_be_served?(runner)).to be_falsey - end - end - - context 'when runner has tags' do - before do - runner.tag_list = ['bb', 'cc'] - end - - shared_examples 'tagged build picker' do - it 'can handle build with matching tags' do - build.tag_list = ['bb'] - expect(build.can_be_served?(runner)).to be_truthy - end - - it 'cannot handle build without matching tags' do - build.tag_list = ['aa'] - expect(build.can_be_served?(runner)).to be_falsey - end - end - - context 'when runner can pick untagged jobs' do - it 'can handle builds without tags' do - expect(build.can_be_served?(runner)).to be_truthy - end - - it_behaves_like 'tagged build picker' - end - - context 'when runner cannot pick untagged jobs' do - before do - runner.run_untagged = false - end - - it 'cannot handle builds without tags' do - expect(build.can_be_served?(runner)).to be_falsey - end - - it_behaves_like 'tagged build picker' - end - end - - context 'when runner is locked' do - before do - runner.locked = true - end - - shared_examples 'locked build picker' do |serve_matching_tags| - context 'when runner cannot pick untagged jobs' do - before do - runner.run_untagged = false - end - - it 'cannot handle builds without tags' do - expect(build.can_be_served?(runner)).to be_falsey - end - end - - context 'when having runner tags' do - before do - runner.tag_list = ['bb', 'cc'] - end - - it "#{serve_matching_tags} handle it for matching tags" do - build.tag_list = ['bb'] - expected = if serve_matching_tags - be_truthy - else - be_falsey - end - expect(build.can_be_served?(runner)).to expected - end - - it 'cannot handle it for builds without matching tags' do - build.tag_list = ['aa'] - expect(build.can_be_served?(runner)).to be_falsey - end - end - end - - context 'when serving the same project' do - it 'can handle it' do - expect(build.can_be_served?(runner)).to be_truthy - end - - it_behaves_like 'locked build picker', true - end - - context 'serving a different project' do - before do - runner.runner_projects.destroy_all - end - - it 'cannot handle it' do - expect(build.can_be_served?(runner)).to be_falsey - end - - it_behaves_like 'locked build picker', false - end - end - end - describe '#has_tags?' do context 'when build has tags' do subject { create(:ci_build, tag_list: ['tag']) } @@ -431,7 +315,7 @@ describe Ci::Build, models: true do end it 'that cannot handle build' do - expect_any_instance_of(Ci::Build).to receive(:can_be_served?).and_return(false) + expect_any_instance_of(Ci::Runner).to receive(:can_pick?).and_return(false) is_expected.to be_falsey end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index c3b715dcb92..477a32a1f9e 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -90,6 +90,125 @@ describe Ci::Runner, models: true do end end + describe '#can_pick?' do + let(:project) { create(:project) } + let(:commit) { create(:ci_commit, project: project) } + let(:build) { create(:ci_build, commit: commit) } + let(:runner) { create(:ci_runner) } + + before do + build.project.runners << runner + end + + context 'when runner does not have tags' do + it 'can handle builds without tags' do + expect(runner.can_pick?(build)).to be_truthy + end + + it 'cannot handle build with tags' do + build.tag_list = ['aa'] + expect(runner.can_pick?(build)).to be_falsey + end + end + + context 'when runner has tags' do + before do + runner.tag_list = ['bb', 'cc'] + end + + shared_examples 'tagged build picker' do + it 'can handle build with matching tags' do + build.tag_list = ['bb'] + expect(runner.can_pick?(build)).to be_truthy + end + + it 'cannot handle build without matching tags' do + build.tag_list = ['aa'] + expect(runner.can_pick?(build)).to be_falsey + end + end + + context 'when runner can pick untagged jobs' do + it 'can handle builds without tags' do + expect(runner.can_pick?(build)).to be_truthy + end + + it_behaves_like 'tagged build picker' + end + + context 'when runner cannot pick untagged jobs' do + before do + runner.run_untagged = false + end + + it 'cannot handle builds without tags' do + expect(runner.can_pick?(build)).to be_falsey + end + + it_behaves_like 'tagged build picker' + end + end + + context 'when runner is locked' do + before do + runner.locked = true + end + + shared_examples 'locked build picker' do |serve_matching_tags| + context 'when runner cannot pick untagged jobs' do + before do + runner.run_untagged = false + end + + it 'cannot handle builds without tags' do + expect(runner.can_pick?(build)).to be_falsey + end + end + + context 'when having runner tags' do + before do + runner.tag_list = ['bb', 'cc'] + end + + it "#{serve_matching_tags} handle it for matching tags" do + build.tag_list = ['bb'] + expected = if serve_matching_tags + be_truthy + else + be_falsey + end + expect(runner.can_pick?(build)).to expected + end + + it 'cannot handle it for builds without matching tags' do + build.tag_list = ['aa'] + expect(runner.can_pick?(build)).to be_falsey + end + end + end + + context 'when serving the same project' do + it 'can handle it' do + expect(runner.can_pick?(build)).to be_truthy + end + + it_behaves_like 'locked build picker', true + end + + context 'serving a different project' do + before do + runner.runner_projects.destroy_all + end + + it 'cannot handle it' do + expect(runner.can_pick?(build)).to be_falsey + end + + it_behaves_like 'locked build picker', false + end + end + end + describe '#status' do let(:runner) { FactoryGirl.create(:ci_runner, :shared, contacted_at: 1.second.ago) } From d7b08024afa41d59c5d41c0e1972e2580e019996 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 8 Jun 2016 15:38:34 +0800 Subject: [PATCH 16/41] Renamed to available_for? Feedback from: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12321240 --- app/models/ci/runner.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 44f820d100b..9bd4d3ab0a5 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -99,7 +99,7 @@ module Ci end def can_pick?(build) - not_locked_or_locked_to?(build.project) && + available_for?(build.project) && run_untagged_or_has_tags?(build) && accepting_tags?(build.tag_list) end @@ -125,7 +125,7 @@ module Ci end end - def not_locked_or_locked_to?(project) + def available_for?(project) !locked? || projects.exists?(id: project.id) end From 8b34687a36c1ec4e558724513bcabdfa3b706854 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 8 Jun 2016 15:43:43 +0800 Subject: [PATCH 17/41] Merge conditions. Not worth an additional pointless method: Feedback from: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12321267 --- app/models/ci/runner.rb | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 9bd4d3ab0a5..bb1cffdcae6 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -99,9 +99,7 @@ module Ci end def can_pick?(build) - available_for?(build.project) && - run_untagged_or_has_tags?(build) && - accepting_tags?(build.tag_list) + available_for?(build.project) && accepting_tags?(build) end def only_for?(project) @@ -129,12 +127,8 @@ module Ci !locked? || projects.exists?(id: project.id) end - def run_untagged_or_has_tags?(build) - run_untagged? || build.has_tags? - end - - def accepting_tags?(target_tags) - (target_tags - tag_list).empty? + def accepting_tags?(build) + (run_untagged? || build.has_tags?) && (build.tag_list - tag_list).empty? end end end From b0908d873921c30ea58da703bf74b4149b6fc145 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 8 Jun 2016 16:07:08 +0800 Subject: [PATCH 18/41] Extra tests inside shared_examples: Feedback from: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12321421 The advantage of this is that tests are less about logic and more straightforward, thus could be easier to reason about each individual tests. The disadvantage of this is that we write more duplicated codes and once something changed we might need to change all places and it's harder to reason all tests as a whole. Because now we need to look at more places to figure out how it should work under another option! --- spec/models/ci/runner_spec.rb | 38 +++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 477a32a1f9e..9a6bc6b96c1 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -154,7 +154,7 @@ describe Ci::Runner, models: true do runner.locked = true end - shared_examples 'locked build picker' do |serve_matching_tags| + shared_examples 'locked build picker' do context 'when runner cannot pick untagged jobs' do before do runner.run_untagged = false @@ -170,16 +170,6 @@ describe Ci::Runner, models: true do runner.tag_list = ['bb', 'cc'] end - it "#{serve_matching_tags} handle it for matching tags" do - build.tag_list = ['bb'] - expected = if serve_matching_tags - be_truthy - else - be_falsey - end - expect(runner.can_pick?(build)).to expected - end - it 'cannot handle it for builds without matching tags' do build.tag_list = ['aa'] expect(runner.can_pick?(build)).to be_falsey @@ -192,7 +182,18 @@ describe Ci::Runner, models: true do expect(runner.can_pick?(build)).to be_truthy end - it_behaves_like 'locked build picker', true + it_behaves_like 'locked build picker' + + context 'when having runner tags' do + before do + runner.tag_list = ['bb', 'cc'] + build.tag_list = ['bb'] + end + + it 'can handle it for matching tags' do + expect(runner.can_pick?(build)).to be_truthy + end + end end context 'serving a different project' do @@ -204,7 +205,18 @@ describe Ci::Runner, models: true do expect(runner.can_pick?(build)).to be_falsey end - it_behaves_like 'locked build picker', false + it_behaves_like 'locked build picker' + + context 'when having runner tags' do + before do + runner.tag_list = ['bb', 'cc'] + build.tag_list = ['bb'] + end + + it 'cannot handle it for matching tags' do + expect(runner.can_pick?(build)).to be_falsey + end + end end end end From f59d972d5f51792bf3d432908eeb09c6865c977c Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 9 Jun 2016 15:49:34 +0800 Subject: [PATCH 19/41] Updated description for the form --- app/views/projects/runners/_form.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 67839fafd28..0946e5e327a 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -13,11 +13,11 @@ = f.check_box :run_untagged %span.light Indicates whether this runner can pick jobs without tags .form-group - = label :locked, 'Exclusive to this project', class: 'control-label' + = label :locked, 'Lock to this project', class: 'control-label' .col-sm-10 .checkbox = f.check_box :locked - %span.light Indicates whether this runner can be enabled for other projects + %span.light When a runner is locked, it cannot be enabled for other projects .form-group = label_tag :token, class: 'control-label' do Token From 4d277b9c747cb0bf14b6c6a88de228083be7f580 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 9 Jun 2016 16:06:02 +0800 Subject: [PATCH 20/41] Updated doc description according to feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12345562 --- doc/ci/runners/README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index bd654e655d6..49b283c43c8 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -98,9 +98,10 @@ sudo gitlab-runner register ### Lock a specific runner from being enabled from other projects -You can configure a runner to prevent it from being enabled for the other -projects, so it's exclusive to current projects. This setting is available -on each runner in *Project Settings* > *Runners*. +You can configure a runner to assign it exclusively to the one project. +When a runner is locked on project this way, it can no longer be enabled +for other projects. This setting is available on each runner in +*Project Settings* > *Runners*. ### Making an existing Shared Runner Specific From d936c32ed97894a333f27cdc9b9c8c9c4f64697d Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 9 Jun 2016 16:29:10 +0800 Subject: [PATCH 21/41] Tweak the wordings and grammar slightly --- doc/ci/runners/README.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index 49b283c43c8..36556f8e670 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -96,12 +96,11 @@ To register the runner, run the command below and follow instructions: sudo gitlab-runner register ``` -### Lock a specific runner from being enabled from other projects +### Lock a specific runner from being enabled for other projects -You can configure a runner to assign it exclusively to the one project. -When a runner is locked on project this way, it can no longer be enabled -for other projects. This setting is available on each runner in -*Project Settings* > *Runners*. +You can configure a runner to assign it exclusively to a project. When a +runner is locked this way, it can no longer be enabled for other projects. +This setting is available on each runner in *Project Settings* > *Runners*. ### Making an existing Shared Runner Specific From 53121601f38155e926eed300160f79dd4bc0768b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 9 Jun 2016 17:07:09 +0800 Subject: [PATCH 22/41] Add a small locked icon if it's locked: This is probably not the way we add icons, but well. Not sure if this should be put next to the status icon or edit icon, my first thought was put this next to status, but it looks a bit better next to edit button. Please tweak this accordingly. I don't have strong opinion regarding views. Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12345567 --- app/views/projects/runners/_runner.html.haml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 96e2aac451f..08389528dc9 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -4,6 +4,8 @@ %span.monospace - if @runners.include?(runner) = link_to runner.short_sha, runner_path(runner) + - if runner.locked? + %small{title: 'Exclusive to this project'} 🔒 %small = link_to edit_namespace_project_runner_path(@project.namespace, @project, runner) do %i.fa.fa-edit.btn From cbd6ca6985c1a7eefcfa5b3ca170fdf1865aee45 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Jun 2016 16:07:57 +0800 Subject: [PATCH 23/41] Rename specific_for to available_for: Feedback from: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12413950 --- app/controllers/projects/runners_controller.rb | 2 +- app/models/ci/runner.rb | 2 +- spec/models/ci/runner_spec.rb | 14 +++++++------- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index bc4c5bd4575..798d668f251 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -7,7 +7,7 @@ class Projects::RunnersController < Projects::ApplicationController def index @runners = project.runners.ordered @specific_runners = current_user.ci_authorized_runners. - specific_for(project).ordered.page(params[:page]).per(20) + available_for(project).ordered.page(params[:page]).per(20) @shared_runners = Ci::Runner.shared.active @shared_runners_count = @shared_runners.count(:all) end diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index bb1cffdcae6..101817e1f56 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -26,7 +26,7 @@ module Ci .where("ci_runner_projects.gl_project_id = :project_id OR ci_runners.is_shared = true", project_id: project_id) end - scope :specific_for, ->(project) do + scope :available_for, ->(project) do # TODO: That `to_sql` is needed to workaround a weird Rails bug. # Without that, placeholders would miss one and couldn't match. where(locked: false). diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 9a6bc6b96c1..51e60ef8ada 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -259,7 +259,7 @@ describe Ci::Runner, models: true do end end - describe '.specific_for' do + describe '.available_for' do let(:runner) { create(:ci_runner) } let(:project) { create(:project) } let(:another_project) { create(:project) } @@ -274,13 +274,13 @@ describe Ci::Runner, models: true do end context 'should not give owned runner' do - subject { Ci::Runner.specific_for(project) } + subject { Ci::Runner.available_for(project) } it { is_expected.to be_empty } end context 'should not give shared runner' do - subject { Ci::Runner.specific_for(another_project) } + subject { Ci::Runner.available_for(another_project) } it { is_expected.to be_empty } end @@ -288,13 +288,13 @@ describe Ci::Runner, models: true do context 'with unlocked runner' do context 'should not give owned runner' do - subject { Ci::Runner.specific_for(project) } + subject { Ci::Runner.available_for(project) } it { is_expected.to be_empty } end context 'should give a specific runner' do - subject { Ci::Runner.specific_for(another_project) } + subject { Ci::Runner.available_for(another_project) } it { is_expected.to contain_exactly(runner) } end @@ -306,13 +306,13 @@ describe Ci::Runner, models: true do end context 'should not give owned runner' do - subject { Ci::Runner.specific_for(project) } + subject { Ci::Runner.available_for(project) } it { is_expected.to be_empty } end context 'should not give a locked runner' do - subject { Ci::Runner.specific_for(another_project) } + subject { Ci::Runner.available_for(another_project) } it { is_expected.to be_empty } end From 5f887344c033d9a5e07a784b56d048a1f33045ab Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Jun 2016 16:36:54 +0800 Subject: [PATCH 24/41] Prefer Runner#assign_to instead of creating directly --- lib/api/runners.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 4faba9dc87b..be92355d9a6 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -96,7 +96,7 @@ module API runner = get_runner(params[:runner_id]) authenticate_enable_runner!(runner) - Ci::RunnerProject.create(runner: runner, project: user_project) + runner.assign_to(user_project) present runner, with: Entities::Runner end From 1b03c5807fab6d2122e5a590cebfb2e7978a6659 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Jun 2016 17:28:11 +0800 Subject: [PATCH 25/41] We're checking return value rather than rescuing exceptions --- app/models/ci/runner.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index adb65292208..288e044fb86 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -56,7 +56,7 @@ module Ci def assign_to(project, current_user = nil) self.is_shared = false if shared? self.save - project.runner_projects.create!(runner_id: self.id) + project.runner_projects.create(runner_id: self.id) end def display_name From f74f42386029077d0f12ac407fc6ee39aaeeaf53 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Jun 2016 22:17:01 +0800 Subject: [PATCH 26/41] Give 409 Conflict whenever the runner was already enabled --- CHANGELOG | 1 + app/models/ci/runner.rb | 2 +- lib/api/runners.rb | 7 +++++-- spec/requests/api/runners_spec.rb | 2 +- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 3387394de5b..7abe41bc81d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -40,6 +40,7 @@ v 8.9.0 (unreleased) - Added artifacts:when to .gitlab-ci.yml - this requires GitLab Runner 1.3 - Todos will display target state if issuable target is 'Closed' or 'Merged' - Fix bug when sorting issues by milestone due date and filtering by two or more labels + - POST to API /projects/:id/runners/:runner_id would give 409 if the runner was already enabled for this project - Add support for using Yubikeys (U2F) for two-factor authentication - Link to blank group icon doesn't throw a 404 anymore - Remove 'main language' feature diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 288e044fb86..a5a55e0a2cd 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -56,7 +56,7 @@ module Ci def assign_to(project, current_user = nil) self.is_shared = false if shared? self.save - project.runner_projects.create(runner_id: self.id) + project.runner_projects.create(runner_id: self.id).persisted? end def display_name diff --git a/lib/api/runners.rb b/lib/api/runners.rb index be92355d9a6..7bea4386e03 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -96,9 +96,12 @@ module API runner = get_runner(params[:runner_id]) authenticate_enable_runner!(runner) - runner.assign_to(user_project) - present runner, with: Entities::Runner + if runner.assign_to(user_project) + present runner, with: Entities::Runner + else + conflict!("Runner was already enabled for this project") + end end # Disable project's runner diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 73ae8ef631c..7aae0192fcb 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -375,7 +375,7 @@ describe API::Runners, api: true do expect do post api("/projects/#{project.id}/runners", user), runner_id: specific_runner.id end.to change{ project.runners.count }.by(0) - expect(response.status).to eq(201) + expect(response.status).to eq(409) end it 'should not enable shared runner' do From 1b8f52d9206bdf19c0dde04505c4c0b1cf46cfbe Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Jun 2016 22:58:38 +0800 Subject: [PATCH 27/41] Avoid enabling locked runners. Give 403 in this case --- .../admin/runner_projects_controller.rb | 2 ++ .../projects/runner_projects_controller.rb | 1 + lib/api/runners.rb | 1 + spec/requests/api/runners_spec.rb | 16 ++++++++++++++-- 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/runner_projects_controller.rb b/app/controllers/admin/runner_projects_controller.rb index d25619d94e0..29307aeab6d 100644 --- a/app/controllers/admin/runner_projects_controller.rb +++ b/app/controllers/admin/runner_projects_controller.rb @@ -9,6 +9,8 @@ class Admin::RunnerProjectsController < Admin::ApplicationController def create @runner = Ci::Runner.find(params[:runner_project][:runner_id]) + return head(403) if runner.is_shared? || runner.is_locked? + if @runner.assign_to(@project, current_user) redirect_to admin_runner_path(@runner) else diff --git a/app/controllers/projects/runner_projects_controller.rb b/app/controllers/projects/runner_projects_controller.rb index bedeb4a295c..4c013303269 100644 --- a/app/controllers/projects/runner_projects_controller.rb +++ b/app/controllers/projects/runner_projects_controller.rb @@ -6,6 +6,7 @@ class Projects::RunnerProjectsController < Projects::ApplicationController def create @runner = Ci::Runner.find(params[:runner_project][:runner_id]) + return head(403) if runner.is_shared? || runner.is_locked? return head(403) unless current_user.ci_authorized_runners.include?(@runner) path = runners_path(project) diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 2d09b6193d9..3ae228d61d8 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -163,6 +163,7 @@ module API def authenticate_enable_runner!(runner) forbidden!("Runner is shared") if runner.is_shared? + forbidden!("Runner is locked") if runner.locked? return if current_user.is_admin? forbidden!("No access granted") unless user_can_access_runner?(runner) end diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index f4f9c417bbb..26dfa7bed05 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -362,11 +362,13 @@ describe API::Runners, api: true do describe 'POST /projects/:id/runners' do context 'authorized user' do - it 'should enable specific runner' do - specific_runner2 = create(:ci_runner).tap do |runner| + let(:specific_runner2) do + create(:ci_runner).tap do |runner| create(:ci_runner_project, runner: runner, project: project2) end + end + it 'should enable specific runner' do expect do post api("/projects/#{project.id}/runners", user), runner_id: specific_runner2.id end.to change{ project.runners.count }.by(+1) @@ -380,6 +382,16 @@ describe API::Runners, api: true do expect(response.status).to eq(201) end + it 'should not enable locked runner' do + specific_runner2.update(locked: true) + + expect do + post api("/projects/#{project.id}/runners", user), runner_id: specific_runner2.id + end.to change{ project.runners.count }.by(0) + + expect(response.status).to eq(403) + end + it 'should not enable shared runner' do post api("/projects/#{project.id}/runners", user), runner_id: shared_runner.id From 9cf45b058627f039040165519de9c2074dda141f Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 14 Jun 2016 23:11:43 +0800 Subject: [PATCH 28/41] Return the association and check it in controller instead: Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4641#note_12444891 --- app/controllers/admin/runner_projects_controller.rb | 4 +++- app/controllers/projects/runner_projects_controller.rb | 3 ++- app/models/ci/runner.rb | 2 +- lib/api/runners.rb | 4 +++- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/controllers/admin/runner_projects_controller.rb b/app/controllers/admin/runner_projects_controller.rb index 29307aeab6d..5383afdbd20 100644 --- a/app/controllers/admin/runner_projects_controller.rb +++ b/app/controllers/admin/runner_projects_controller.rb @@ -11,7 +11,9 @@ class Admin::RunnerProjectsController < Admin::ApplicationController return head(403) if runner.is_shared? || runner.is_locked? - if @runner.assign_to(@project, current_user) + runner_project = @runner.assign_to(@project, current_user) + + if runner_project.persisted? redirect_to admin_runner_path(@runner) else redirect_to admin_runner_path(@runner), alert: 'Failed adding runner to project' diff --git a/app/controllers/projects/runner_projects_controller.rb b/app/controllers/projects/runner_projects_controller.rb index 4c013303269..2413e583d7b 100644 --- a/app/controllers/projects/runner_projects_controller.rb +++ b/app/controllers/projects/runner_projects_controller.rb @@ -10,8 +10,9 @@ class Projects::RunnerProjectsController < Projects::ApplicationController return head(403) unless current_user.ci_authorized_runners.include?(@runner) path = runners_path(project) + runner_project = @runner.assign_to(project, current_user) - if @runner.assign_to(project, current_user) + if runner_project.persisted? redirect_to path else redirect_to path, alert: 'Failed adding runner to project' diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index df343fc957a..8149929f492 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -63,7 +63,7 @@ module Ci def assign_to(project, current_user = nil) self.is_shared = false if shared? self.save - project.runner_projects.create(runner_id: self.id).persisted? + project.runner_projects.create(runner_id: self.id) end def display_name diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 2c2610fc2e7..ecc8f2fc5a2 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -97,7 +97,9 @@ module API runner = get_runner(params[:runner_id]) authenticate_enable_runner!(runner) - if runner.assign_to(user_project) + runner_project = runner.assign_to(user_project) + + if runner_project.persisted? present runner, with: Entities::Runner else conflict!("Runner was already enabled for this project") From c866b906e67d42d9ad9537e8a6d6a254c3f74d8b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 15 Jun 2016 17:29:07 +0800 Subject: [PATCH 29/41] Fix typo. It's ivar and the column was called locked --- app/controllers/projects/runner_projects_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/projects/runner_projects_controller.rb b/app/controllers/projects/runner_projects_controller.rb index 2413e583d7b..dc1a18f8d42 100644 --- a/app/controllers/projects/runner_projects_controller.rb +++ b/app/controllers/projects/runner_projects_controller.rb @@ -6,7 +6,7 @@ class Projects::RunnerProjectsController < Projects::ApplicationController def create @runner = Ci::Runner.find(params[:runner_project][:runner_id]) - return head(403) if runner.is_shared? || runner.is_locked? + return head(403) if @runner.is_shared? || @runner.locked? return head(403) unless current_user.ci_authorized_runners.include?(@runner) path = runners_path(project) From cd3f112f88c2362bb64961e52e6272314287a80b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 15 Jun 2016 17:34:44 +0800 Subject: [PATCH 30/41] Adopt the rename from ci_commits to ci_pipelines --- spec/factories/ci/{commits.rb => pipelines.rb} | 0 spec/models/ci/runner_spec.rb | 4 ++-- 2 files changed, 2 insertions(+), 2 deletions(-) rename spec/factories/ci/{commits.rb => pipelines.rb} (100%) diff --git a/spec/factories/ci/commits.rb b/spec/factories/ci/pipelines.rb similarity index 100% rename from spec/factories/ci/commits.rb rename to spec/factories/ci/pipelines.rb diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 51e60ef8ada..c3638bf407f 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -92,8 +92,8 @@ describe Ci::Runner, models: true do describe '#can_pick?' do let(:project) { create(:project) } - let(:commit) { create(:ci_commit, project: project) } - let(:build) { create(:ci_build, commit: commit) } + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, pipeline: pipeline) } let(:runner) { create(:ci_runner) } before do From 5d76c2553898382531889f001a506eaafcaea56d Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 15 Jun 2016 19:05:39 +0800 Subject: [PATCH 31/41] Use font awesome instead of Unicode. Feedback from: https://gitlab.com/gitlab-org/gitlab-ce/commit/53121601f38155e926eed300160f79dd4bc0768b#note_12465224 --- app/views/projects/runners/_runner.html.haml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 08389528dc9..fbd94c34591 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -5,7 +5,8 @@ - if @runners.include?(runner) = link_to runner.short_sha, runner_path(runner) - if runner.locked? - %small{title: 'Exclusive to this project'} 🔒 + %small{title: 'Exclusive to this project'} + = icon('lock') %small = link_to edit_namespace_project_runner_path(@project.namespace, @project, runner) do %i.fa.fa-edit.btn From 4852acef9256ee60d76564ff7509dc72e016a863 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 16 Jun 2016 23:20:13 +0800 Subject: [PATCH 32/41] Use FIXME instead, feedback from: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093/diffs#note_12501400 --- app/models/ci/runner.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 8149929f492..fa5cf03baec 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -27,8 +27,8 @@ module Ci end scope :available_for, ->(project) do - # TODO: That `to_sql` is needed to workaround a weird Rails bug. - # Without that, placeholders would miss one and couldn't match. + # FIXME: That `to_sql` is needed to workaround a weird Rails bug. + # Without that, placeholders would miss one and couldn't match. where(locked: false). where.not("id IN (#{project.runners.select(:id).to_sql})").specific end From ae0e1e402e1f754943ad200958f85bc90ca82b52 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 16 Jun 2016 23:26:49 +0800 Subject: [PATCH 33/41] blank line between setup and expectation, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093/diffs#note_12501266 and: https://robots.thoughtbot.com/four-phase-test --- spec/models/ci/runner_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index c3638bf407f..2b21c3561db 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -107,6 +107,7 @@ describe Ci::Runner, models: true do it 'cannot handle build with tags' do build.tag_list = ['aa'] + expect(runner.can_pick?(build)).to be_falsey end end @@ -119,11 +120,13 @@ describe Ci::Runner, models: true do shared_examples 'tagged build picker' do it 'can handle build with matching tags' do build.tag_list = ['bb'] + expect(runner.can_pick?(build)).to be_truthy end it 'cannot handle build without matching tags' do build.tag_list = ['aa'] + expect(runner.can_pick?(build)).to be_falsey end end @@ -172,6 +175,7 @@ describe Ci::Runner, models: true do it 'cannot handle it for builds without matching tags' do build.tag_list = ['aa'] + expect(runner.can_pick?(build)).to be_falsey end end From 314befded7e30981bf858e8e4aebd3df7f8d7d54 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 16 Jun 2016 23:31:54 +0800 Subject: [PATCH 34/41] Fix typo. It's ivar and the column was called locked Again! For admin. --- app/controllers/admin/runner_projects_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/admin/runner_projects_controller.rb b/app/controllers/admin/runner_projects_controller.rb index 5383afdbd20..20f30e70c8a 100644 --- a/app/controllers/admin/runner_projects_controller.rb +++ b/app/controllers/admin/runner_projects_controller.rb @@ -9,7 +9,7 @@ class Admin::RunnerProjectsController < Admin::ApplicationController def create @runner = Ci::Runner.find(params[:runner_project][:runner_id]) - return head(403) if runner.is_shared? || runner.is_locked? + return head(403) if @runner.is_shared? || @runner.locked? runner_project = @runner.assign_to(@project, current_user) From 8a0aeab846f5c97ba56d34644834e38f2378c7ce Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 16 Jun 2016 23:33:53 +0800 Subject: [PATCH 35/41] Use active tense, feedback from: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12501303 --- spec/models/ci/runner_spec.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 2b21c3561db..c7248ef1384 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -20,17 +20,17 @@ describe Ci::Runner, models: true do end describe '#display_name' do - it 'should return the description if it has a value' do + it 'returns the description if it has a value' do runner = FactoryGirl.build(:ci_runner, description: 'Linux/Ruby-1.9.3-p448') expect(runner.display_name).to eq 'Linux/Ruby-1.9.3-p448' end - it 'should return the token if it does not have a description' do + it 'returns the token if it does not have a description' do runner = FactoryGirl.create(:ci_runner) expect(runner.display_name).to eq runner.description end - it 'should return the token if the description is an empty string' do + it 'returns the token if the description is an empty string' do runner = FactoryGirl.build(:ci_runner, description: '', token: 'token') expect(runner.display_name).to eq runner.token end @@ -277,13 +277,13 @@ describe Ci::Runner, models: true do runner.update(is_shared: true) end - context 'should not give owned runner' do + context 'does not give owned runner' do subject { Ci::Runner.available_for(project) } it { is_expected.to be_empty } end - context 'should not give shared runner' do + context 'does not give shared runner' do subject { Ci::Runner.available_for(another_project) } it { is_expected.to be_empty } @@ -291,13 +291,13 @@ describe Ci::Runner, models: true do end context 'with unlocked runner' do - context 'should not give owned runner' do + context 'does not give owned runner' do subject { Ci::Runner.available_for(project) } it { is_expected.to be_empty } end - context 'should give a specific runner' do + context 'does give a specific runner' do subject { Ci::Runner.available_for(another_project) } it { is_expected.to contain_exactly(runner) } @@ -309,13 +309,13 @@ describe Ci::Runner, models: true do runner.update(locked: true) end - context 'should not give owned runner' do + context 'does not give owned runner' do subject { Ci::Runner.available_for(project) } it { is_expected.to be_empty } end - context 'should not give a locked runner' do + context 'does not give a locked runner' do subject { Ci::Runner.available_for(another_project) } it { is_expected.to be_empty } From dbd534a8537ba02b93dcf673aee0b62d51bf129d Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 17 Jun 2016 21:04:09 +0800 Subject: [PATCH 36/41] Admin::RunnerProjectsController#index is not used --- app/controllers/admin/runner_projects_controller.rb | 5 ----- config/routes.rb | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/app/controllers/admin/runner_projects_controller.rb b/app/controllers/admin/runner_projects_controller.rb index 20f30e70c8a..bf20c5305a7 100644 --- a/app/controllers/admin/runner_projects_controller.rb +++ b/app/controllers/admin/runner_projects_controller.rb @@ -1,11 +1,6 @@ class Admin::RunnerProjectsController < Admin::ApplicationController before_action :project, only: [:create] - def index - @runner_projects = project.runner_projects.all - @runner_project = project.runner_projects.new - end - def create @runner = Ci::Runner.find(params[:runner_project][:runner_id]) diff --git a/config/routes.rb b/config/routes.rb index d52cbb22428..cdfbafbd730 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -283,7 +283,7 @@ Rails.application.routes.draw do post :repository_check end - resources :runner_projects + resources :runner_projects, only: [:create, :destroy] end end From 045dad3cf17f7e5562dde847ae6d9e9118cfa5ba Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 17 Jun 2016 22:17:20 +0800 Subject: [PATCH 37/41] Test for enabling/disabling runners from admin runner page --- app/views/admin/runners/show.html.haml | 4 +-- spec/features/admin/admin_runners_spec.rb | 34 +++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/app/views/admin/runners/show.html.haml b/app/views/admin/runners/show.html.haml index e049b40bfab..61abfc6ecbe 100644 --- a/app/views/admin/runners/show.html.haml +++ b/app/views/admin/runners/show.html.haml @@ -28,7 +28,7 @@ .col-md-6 %h4 Restrict projects for this runner - if @runner.projects.any? - %table.table + %table.table.assigned-projects %thead %tr %th Assigned projects @@ -44,7 +44,7 @@ .pull-right = link_to 'Disable', [:admin, project.namespace.becomes(Namespace), project, runner_project], method: :delete, class: 'btn btn-danger btn-xs' - %table.table + %table.table.unassigned-projects %thead %tr %th Project diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index 9499cd4e025..2d297776cb0 100644 --- a/spec/features/admin/admin_runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -60,6 +60,40 @@ describe "Admin Runners" do it { expect(page).to have_content(@project1.name_with_namespace) } it { expect(page).not_to have_content(@project2.name_with_namespace) } end + + describe 'enable/create' do + before do + @project1.runners << runner + visit admin_runner_path(runner) + end + + it 'enables specific runner for project' do + within '.unassigned-projects' do + click_on 'Enable' + end + + assigned_project = page.find('.assigned-projects') + + expect(assigned_project).to have_content(@project2.path) + end + end + + describe 'disable/destroy' do + before do + @project1.runners << runner + visit admin_runner_path(runner) + end + + it 'enables specific runner for project' do + within '.assigned-projects' do + click_on 'Disable' + end + + new_runner_project = page.find('.unassigned-projects') + + expect(new_runner_project).to have_content(@project1.path) + end + end end describe 'runners registration token' do From 60ef0dd20c91fd8d8f04ec139edaa07dbab86d1b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 20 Jun 2016 16:03:52 +0800 Subject: [PATCH 38/41] Use .has-tooltip as suggested at: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12533926 --- app/views/projects/runners/_runner.html.haml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index fbd94c34591..13113f9b88f 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -5,8 +5,7 @@ - if @runners.include?(runner) = link_to runner.short_sha, runner_path(runner) - if runner.locked? - %small{title: 'Exclusive to this project'} - = icon('lock') + = icon('lock', class: 'has-tooltip', title: 'Exclusive to this project') %small = link_to edit_namespace_project_runner_path(@project.namespace, @project, runner) do %i.fa.fa-edit.btn From 4efc1c4a68538126bc4cfad6d9b60710bee36f14 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 20 Jun 2016 16:52:05 +0800 Subject: [PATCH 39/41] Rename according to: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12563922 For clarification and consistency --- app/controllers/projects/runners_controller.rb | 6 +++--- app/models/ci/runner.rb | 6 +++--- app/views/projects/runners/_runner.html.haml | 4 ++-- .../projects/runners/_specific_runners.html.haml | 10 +++++----- spec/models/ci/runner_spec.rb | 14 +++++++------- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 798d668f251..53c36635efe 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -5,9 +5,9 @@ class Projects::RunnersController < Projects::ApplicationController layout 'project_settings' def index - @runners = project.runners.ordered - @specific_runners = current_user.ci_authorized_runners. - available_for(project).ordered.page(params[:page]).per(20) + @project_runners = project.runners.ordered + @assignable_runners = current_user.ci_authorized_runners. + assignable_for(project).ordered.page(params[:page]).per(20) @shared_runners = Ci::Runner.shared.active @shared_runners_count = @shared_runners.count(:all) end diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index fa5cf03baec..b64ec79ec2b 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -26,7 +26,7 @@ module Ci .where("ci_runner_projects.gl_project_id = :project_id OR ci_runners.is_shared = true", project_id: project_id) end - scope :available_for, ->(project) do + scope :assignable_for, ->(project) do # FIXME: That `to_sql` is needed to workaround a weird Rails bug. # Without that, placeholders would miss one and couldn't match. where(locked: false). @@ -99,7 +99,7 @@ module Ci end def can_pick?(build) - available_for?(build.project) && accepting_tags?(build) + assignable_for?(build.project) && accepting_tags?(build) end def only_for?(project) @@ -123,7 +123,7 @@ module Ci end end - def available_for?(project) + def assignable_for?(project) !locked? || projects.exists?(id: project.id) end diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 13113f9b88f..3cf66bbcb4a 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -2,7 +2,7 @@ %h4 = runner_status_icon(runner) %span.monospace - - if @runners.include?(runner) + - if @project_runners.include?(runner) = link_to runner.short_sha, runner_path(runner) - if runner.locked? = icon('lock', class: 'has-tooltip', title: 'Exclusive to this project') @@ -13,7 +13,7 @@ = runner.short_sha .pull-right - - if @runners.include?(runner) + - if @project_runners.include?(runner) - if runner.belongs_to_one_project? = link_to 'Remove runner', runner_path(runner), data: { confirm: "Are you sure?" }, method: :delete, class: 'btn btn-danger btn-sm' - else diff --git a/app/views/projects/runners/_specific_runners.html.haml b/app/views/projects/runners/_specific_runners.html.haml index 8ae9f0d95f7..d469dda5b81 100644 --- a/app/views/projects/runners/_specific_runners.html.haml +++ b/app/views/projects/runners/_specific_runners.html.haml @@ -17,13 +17,13 @@ Start runner! -- if @runners.any? +- if @project_runners.any? %h4.underlined-title Runners activated for this project %ul.bordered-list.activated-specific-runners - = render partial: 'runner', collection: @runners, as: :runner + = render partial: 'runner', collection: @project_runners, as: :runner -- if @specific_runners.any? +- if @assignable_runners.any? %h4.underlined-title Available specific runners %ul.bordered-list.available-specific-runners - = render partial: 'runner', collection: @specific_runners, as: :runner - = paginate @specific_runners + = render partial: 'runner', collection: @assignable_runners, as: :runner + = paginate @assignable_runners diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index c7248ef1384..ef65eb99328 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -263,7 +263,7 @@ describe Ci::Runner, models: true do end end - describe '.available_for' do + describe '.assignable_for' do let(:runner) { create(:ci_runner) } let(:project) { create(:project) } let(:another_project) { create(:project) } @@ -278,13 +278,13 @@ describe Ci::Runner, models: true do end context 'does not give owned runner' do - subject { Ci::Runner.available_for(project) } + subject { Ci::Runner.assignable_for(project) } it { is_expected.to be_empty } end context 'does not give shared runner' do - subject { Ci::Runner.available_for(another_project) } + subject { Ci::Runner.assignable_for(another_project) } it { is_expected.to be_empty } end @@ -292,13 +292,13 @@ describe Ci::Runner, models: true do context 'with unlocked runner' do context 'does not give owned runner' do - subject { Ci::Runner.available_for(project) } + subject { Ci::Runner.assignable_for(project) } it { is_expected.to be_empty } end context 'does give a specific runner' do - subject { Ci::Runner.available_for(another_project) } + subject { Ci::Runner.assignable_for(another_project) } it { is_expected.to contain_exactly(runner) } end @@ -310,13 +310,13 @@ describe Ci::Runner, models: true do end context 'does not give owned runner' do - subject { Ci::Runner.available_for(project) } + subject { Ci::Runner.assignable_for(project) } it { is_expected.to be_empty } end context 'does not give a locked runner' do - subject { Ci::Runner.available_for(another_project) } + subject { Ci::Runner.assignable_for(another_project) } it { is_expected.to be_empty } end From c628eeb773a962c66bcb4f73bfee60cdc28d2435 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 20 Jun 2016 12:03:21 +0000 Subject: [PATCH 40/41] Add index for ci_runners.locked, feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12571602 and https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12571670 --- .../20160620115026_add_index_on_runners_locked.rb | 12 ++++++++++++ db/schema.rb | 3 ++- 2 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20160620115026_add_index_on_runners_locked.rb diff --git a/db/migrate/20160620115026_add_index_on_runners_locked.rb b/db/migrate/20160620115026_add_index_on_runners_locked.rb new file mode 100644 index 00000000000..dfa5110dea4 --- /dev/null +++ b/db/migrate/20160620115026_add_index_on_runners_locked.rb @@ -0,0 +1,12 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndexOnRunnersLocked < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def change + add_concurrent_index :ci_runners, :locked + end +end diff --git a/db/schema.rb b/db/schema.rb index daf66f1b4dd..cf9637b04b7 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: 20160616084004) do +ActiveRecord::Schema.define(version: 20160620115026) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -291,6 +291,7 @@ ActiveRecord::Schema.define(version: 20160616084004) do end add_index "ci_runners", ["description"], name: "index_ci_runners_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} + add_index "ci_runners", ["locked"], name: "index_ci_runners_on_locked", using: :btree add_index "ci_runners", ["token"], name: "index_ci_runners_on_token", using: :btree add_index "ci_runners", ["token"], name: "index_ci_runners_on_token_trigram", using: :gin, opclasses: {"token"=>"gin_trgm_ops"} From cdcbc8d7219a669c016aaba89a1d47faebdd2208 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 21 Jun 2016 16:31:37 +0800 Subject: [PATCH 41/41] Update wordings according to: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4093#note_12594802 --- app/views/projects/runners/_form.html.haml | 4 ++-- app/views/projects/runners/_runner.html.haml | 2 +- app/views/projects/runners/show.html.haml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 0946e5e327a..c45a9d4f81f 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -13,11 +13,11 @@ = f.check_box :run_untagged %span.light Indicates whether this runner can pick jobs without tags .form-group - = label :locked, 'Lock to this project', class: 'control-label' + = label :locked, 'Lock to current projects', class: 'control-label' .col-sm-10 .checkbox = f.check_box :locked - %span.light When a runner is locked, it cannot be enabled for other projects + %span.light When a runner is locked, it cannot be assigned to other projects .form-group = label_tag :token, class: 'control-label' do Token diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 3cf66bbcb4a..85225857758 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -5,7 +5,7 @@ - if @project_runners.include?(runner) = link_to runner.short_sha, runner_path(runner) - if runner.locked? - = icon('lock', class: 'has-tooltip', title: 'Exclusive to this project') + = icon('lock', class: 'has-tooltip', title: 'Locked to current projects') %small = link_to edit_namespace_project_runner_path(@project.namespace, @project, runner) do %i.fa.fa-edit.btn diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index fc6424402ae..61b99f35d74 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -23,7 +23,7 @@ %td Can run untagged jobs %td= @runner.run_untagged? ? 'Yes' : 'No' %tr - %td Exclusive to this project + %td Locked to this project %td= @runner.locked? ? 'Yes' : 'No' %tr %td Tags