diff options
author | Dylan Griffith <dyl.griffith@gmail.com> | 2018-05-11 12:03:40 +0200 |
---|---|---|
committer | Dylan Griffith <dyl.griffith@gmail.com> | 2018-05-31 10:46:19 +0200 |
commit | ab489d293d6ee3e30673817ce4652c7b413988c0 (patch) | |
tree | e197ccd80fefd1f4ae610e96efeebf139c69b302 | |
parent | ec1d3e104afddf7c8a5f6f5d8bf1ffee99a8f551 (diff) | |
download | gitlab-ce-ab489d293d6ee3e30673817ce4652c7b413988c0.tar.gz |
Improve runner_type validations for Ci::Runner
-rw-r--r-- | app/models/ci/runner.rb | 22 | ||||
-rw-r--r-- | spec/controllers/groups/runners_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/factories/ci/runners.rb | 13 | ||||
-rw-r--r-- | spec/features/admin/admin_runners_spec.rb | 2 | ||||
-rw-r--r-- | spec/features/runners_spec.rb | 22 | ||||
-rw-r--r-- | spec/models/ci/runner_spec.rb | 99 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 4 | ||||
-rw-r--r-- | spec/requests/api/runners_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/ci/register_job_service_spec.rb | 30 | ||||
-rw-r--r-- | spec/services/ci/update_build_queue_service_spec.rb | 10 |
10 files changed, 107 insertions, 99 deletions
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 530eacf4be0..2119b70c18c 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -56,7 +56,9 @@ module Ci end validate :tag_constraints - validate :either_projects_or_group + validate :no_projects, unless: :project_type? + validate :no_groups, unless: :group_type? + validate :only_one_group, if: :group_type? validates :access_level, presence: true validates :runner_type, presence: true @@ -253,13 +255,21 @@ module Ci self.class.owned_or_shared(project_id).where(id: self.id).any? end - def either_projects_or_group - if groups.many? - errors.add(:runner, 'can only be assigned to one group') + def no_projects + if projects.any? + errors.add(:runner, 'cannot assign project to a non-project runner') + end + end + + def no_groups + if groups.any? + errors.add(:runner, 'cannot assign group to a non-group runner') end + end - if assigned_to_group? && assigned_to_project? - errors.add(:runner, 'can only be assigned either to projects or to a group') + def only_one_group + if groups.many? + errors.add(:runner, 'can only be assigned to one group') end end diff --git a/spec/controllers/groups/runners_controller_spec.rb b/spec/controllers/groups/runners_controller_spec.rb index 6d31b0ce959..49cb45ff7b0 100644 --- a/spec/controllers/groups/runners_controller_spec.rb +++ b/spec/controllers/groups/runners_controller_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Groups::RunnersController do let(:user) { create(:user) } let(:group) { create(:group) } - let(:runner) { create(:ci_runner) } + let(:runner) { create(:ci_runner, :group) } let(:params) do { diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index cdc170b9ccb..9d106250d08 100644 --- a/spec/factories/ci/runners.rb +++ b/spec/factories/ci/runners.rb @@ -21,6 +21,19 @@ FactoryBot.define do is_shared false end + trait :group do + runner_type :group_type + end + + trait :project do + runner_type :project_type + end + + trait :instance do + is_shared true + runner_type :instance_type + end + trait :inactive do active false end diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index c33014cbb31..5c9bad8fb2c 100644 --- a/spec/features/admin/admin_runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -62,7 +62,7 @@ describe "Admin Runners" do context 'group runner' do let(:group) { create(:group) } - let!(:runner) { create(:ci_runner, groups: [group], runner_type: :group_type) } + let!(:runner) { create(:ci_runner, :group, groups: [group]) } it 'shows the label and does not show the project count' do visit admin_runners_path diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index e0cd963fe39..f905e6d4f5e 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -28,8 +28,8 @@ feature 'Runners' do project.add_master(user) end - context 'when a specific runner is activated on the project' do - given(:specific_runner) { create(:ci_runner, :specific) } + context 'when a project_type runner is activated on the project' do + given(:specific_runner) { create(:ci_runner, :project) } background do project.runners << specific_runner @@ -114,7 +114,7 @@ feature 'Runners' do end context 'when a shared runner is activated on the project' do - given!(:shared_runner) { create(:ci_runner, :shared) } + given!(:shared_runner) { create(:ci_runner, :instance) } scenario 'user sees CI/CD setting page' do visit project_runners_path(project) @@ -126,7 +126,7 @@ feature 'Runners' do context 'when a specific runner exists in another project' do given(:another_project) { create(:project) } - given(:specific_runner) { create(:ci_runner, :specific) } + given(:specific_runner) { create(:ci_runner, :project) } background do another_project.add_master(user) @@ -220,8 +220,8 @@ feature 'Runners' do end context 'project with a group but no group runner' do - given(:group) { create :group } - given(:project) { create :project, group: group } + given(:group) { create(:group) } + given(:project) { create(:project, group: group) } scenario 'group runners are not available' do visit project_runners_path(project) @@ -234,9 +234,9 @@ feature 'Runners' do end context 'project with a group and a group runner' do - given(:group) { create :group } - given(:project) { create :project, group: group } - given!(:ci_runner) { create :ci_runner, groups: [group], description: 'group-runner' } + given(:group) { create(:group) } + given(:project) { create(:project, group: group) } + given!(:ci_runner) { create(:ci_runner, :group, groups: [group], description: 'group-runner') } scenario 'group runners are available' do visit project_runners_path(project) @@ -263,7 +263,7 @@ feature 'Runners' do end context 'group runners in group settings' do - given(:group) { create :group } + given(:group) { create(:group) } background do group.add_master(user) end @@ -277,7 +277,7 @@ feature 'Runners' do end context 'group with a runner' do - let!(:runner) { create :ci_runner, groups: [group], description: 'group-runner' } + let!(:runner) { create(:ci_runner, :group, groups: [group], description: 'group-runner') } scenario 'the runner is visible' do visit group_settings_ci_cd_path(group) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 0fbc934f669..4dc990350d2 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -21,60 +21,50 @@ describe Ci::Runner do end end - context 'either_projects_or_group' do + context '#only_one_group' do let(:group) { create(:group) } + let(:runner) { create(:ci_runner, :group, groups: [group]) } - it 'disallows assigning to a group if already assigned to a group' do - runner = create(:ci_runner, groups: [group]) - + it 'disallows assigning group if already assigned to a group' do runner.groups << build(:group) expect(runner).not_to be_valid - expect(runner.errors.full_messages).to eq ['Runner can only be assigned to one group'] + expect(runner.errors.full_messages).to include('Runner can only be assigned to one group') end + end - it 'disallows assigning to a group if already assigned to a project' do - project = create(:project) - runner = create(:ci_runner, projects: [project]) + context 'runner_type validations' do + let(:group) { create(:group) } + let(:group_runner) { create(:ci_runner, :group, groups: [group]) } + let(:project_runner) { create(:ci_runner, :project) } + let(:instance_runner) { create(:ci_runner, :instance) } - runner.groups << build(:group) + it 'disallows assigning group to project_type runner' do + project_runner.groups << build(:group) - expect(runner).not_to be_valid - expect(runner.errors.full_messages).to eq ['Runner can only be assigned either to projects or to a group'] + expect(project_runner).not_to be_valid + expect(project_runner.errors.full_messages).to include('Runner cannot assign group to a non-group runner') end - it 'disallows assigning to a project if already assigned to a group' do - runner = create(:ci_runner, groups: [group]) - - runner.projects << build(:project) + it 'disallows assigning group to instance_type runner' do + instance_runner.groups << build(:group) - expect(runner).not_to be_valid - expect(runner.errors.full_messages).to eq ['Runner can only be assigned either to projects or to a group'] + expect(instance_runner).not_to be_valid + expect(instance_runner.errors.full_messages).to include('Runner cannot assign group to a non-group runner') end - it 'allows assigning to a group if not assigned to a group nor a project' do - runner = create(:ci_runner) - - runner.groups << build(:group) + it 'disallows assigning project to group_type runner' do + group_runner.projects << build(:project) - expect(runner).to be_valid + expect(group_runner).not_to be_valid + expect(group_runner.errors.full_messages).to include('Runner cannot assign project to a non-project runner') end - it 'allows assigning to a project if not assigned to a group nor a project' do - runner = create(:ci_runner) - - runner.projects << build(:project) + it 'disallows assigning project to instance_type runner' do + instance_runner.projects << build(:project) - expect(runner).to be_valid - end - - it 'allows assigning to a project if already assigned to a project' do - project = create(:project) - runner = create(:ci_runner, projects: [project]) - - runner.projects << build(:project) - - expect(runner).to be_valid + expect(instance_runner).not_to be_valid + expect(instance_runner.errors.full_messages).to include('Runner cannot assign project to a non-project runner') end end end @@ -110,17 +100,12 @@ describe Ci::Runner do describe '.shared' do let(:group) { create(:group) } let(:project) { create(:project) } + let!(:group_runner) { create(:ci_runner, :group) } + let!(:project_runner) { create(:ci_runner, :project) } + let!(:shared_runner) { create(:ci_runner, :instance) } - it 'returns the shared group runner' do - runner = create(:ci_runner, :shared, groups: [group]) - - expect(described_class.shared).to eq [runner] - end - - it 'returns the shared project runner' do - runner = create(:ci_runner, :shared, projects: [project]) - - expect(described_class.shared).to eq [runner] + it 'returns only shared runners' do + expect(described_class.shared).to contain_exactly(shared_runner) end end @@ -141,17 +126,17 @@ describe Ci::Runner do describe '.belonging_to_parent_group_of_project' do let(:project) { create(:project, group: group) } let(:group) { create(:group) } - let(:runner) { create(:ci_runner, :specific, groups: [group]) } + let(:runner) { create(:ci_runner, :group, groups: [group]) } let!(:unrelated_group) { create(:group) } let!(:unrelated_project) { create(:project, group: unrelated_group) } - let!(:unrelated_runner) { create(:ci_runner, :specific, groups: [unrelated_group]) } + let!(:unrelated_runner) { create(:ci_runner, :group, groups: [unrelated_group]) } it 'returns the specific group runner' do expect(described_class.belonging_to_parent_group_of_project(project.id)).to contain_exactly(runner) end context 'with a parent group with a runner', :nested_groups do - let(:runner) { create(:ci_runner, :specific, groups: [parent_group]) } + let(:runner) { create(:ci_runner, :group, groups: [parent_group]) } let(:project) { create(:project, group: group) } let(:group) { create(:group, parent: parent_group) } let(:parent_group) { create(:group) } @@ -167,13 +152,13 @@ describe Ci::Runner do # group specific group = create(:group) project = create(:project, group: group) - group_runner = create(:ci_runner, :specific, groups: [group]) + group_runner = create(:ci_runner, :group, groups: [group]) # project specific - project_runner = create(:ci_runner, :specific, projects: [project]) + project_runner = create(:ci_runner, :project, projects: [project]) # globally shared - shared_runner = create(:ci_runner, :shared) + shared_runner = create(:ci_runner, :instance) expect(described_class.owned_or_shared(project.id)).to contain_exactly( group_runner, project_runner, shared_runner @@ -216,7 +201,7 @@ describe Ci::Runner do end context 'with group runner' do - let!(:runner) { FactoryBot.create(:ci_runner, runner_type: :group_type) } + let!(:runner) { FactoryBot.create(:ci_runner, :group) } it 'raises an error' do expect { subject } @@ -727,7 +712,7 @@ describe Ci::Runner do context 'when group runner' do let(:group) { create(:group) } - let(:runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } + let(:runner) { create(:ci_runner, :group, description: 'Group runner', groups: [group]) } it { is_expected.to be_truthy } end @@ -737,18 +722,18 @@ describe Ci::Runner do subject { runner.assigned_to_project? } context 'when group runner' do - let(:runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } + let(:runner) { create(:ci_runner, :group, description: 'Group runner', groups: [group]) } let(:group) { create(:group) } it { is_expected.to be_falsey } end context 'when shared runner' do - let(:runner) { create(:ci_runner, :shared, description: 'Shared runner') } + let(:runner) { create(:ci_runner, :instance, description: 'Shared runner') } it { is_expected.to be_falsey } end context 'when project runner' do - let(:runner) { create(:ci_runner, description: 'Group runner', projects: [project]) } + let(:runner) { create(:ci_runner, description: 'Project runner', projects: [project]) } let(:project) { create(:project) } it { is_expected.to be_truthy } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index af2240f4f89..be471f198ff 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1238,7 +1238,7 @@ describe Project do context 'group runners' do let(:project) { create(:project, group_runners_enabled: group_runners_enabled) } let(:group) { create(:group, projects: [project]) } - let(:group_runner) { create(:ci_runner, groups: [group]) } + let(:group_runner) { create(:ci_runner, :group, groups: [group]) } context 'for group runners disabled' do let(:group_runners_enabled) { false } @@ -1279,7 +1279,7 @@ describe Project do end describe '#shared_runners' do - let!(:runner) { create(:ci_runner, :shared) } + let!(:runner) { create(:ci_runner, :instance) } subject { project.shared_runners } diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index c7587c877fc..7050418238c 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -27,7 +27,7 @@ describe API::Runners do end end - let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group], runner_type: :group_type) } + let!(:group_runner) { create(:ci_runner, :group, description: 'Group runner', groups: [group]) } before do # Set project access for users diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 8063bc7e1ac..5fed6e0f395 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -7,7 +7,7 @@ module Ci set(:pipeline) { create(:ci_pipeline, project: project) } let!(:shared_runner) { create(:ci_runner, is_shared: true) } let!(:specific_runner) { create(:ci_runner, is_shared: false) } - let!(:group_runner) { create(:ci_runner, groups: [group], runner_type: :group_type) } + let!(:group_runner) { create(:ci_runner, :group, groups: [group]) } let!(:pending_job) { create(:ci_build, pipeline: pipeline) } before do @@ -181,24 +181,24 @@ module Ci end context 'for multiple builds' do - let!(:project2) { create :project, group_runners_enabled: true, group: group } - let!(:pipeline2) { create :ci_pipeline, project: project2 } - let!(:project3) { create :project, group_runners_enabled: true, group: group } - let!(:pipeline3) { create :ci_pipeline, project: project3 } + let!(:project2) { create(:project, group_runners_enabled: true, group: group) } + let!(:pipeline2) { create(:ci_pipeline, project: project2) } + let!(:project3) { create(:project, group_runners_enabled: true, group: group) } + let!(:pipeline3) { create(:ci_pipeline, project: project3) } let!(:build1_project1) { pending_job } - let!(:build2_project1) { create :ci_build, pipeline: pipeline } - let!(:build3_project1) { create :ci_build, pipeline: pipeline } - let!(:build1_project2) { create :ci_build, pipeline: pipeline2 } - let!(:build2_project2) { create :ci_build, pipeline: pipeline2 } - let!(:build1_project3) { create :ci_build, pipeline: pipeline3 } + let!(:build2_project1) { create(:ci_build, pipeline: pipeline) } + let!(:build3_project1) { create(:ci_build, pipeline: pipeline) } + let!(:build1_project2) { create(:ci_build, pipeline: pipeline2) } + let!(:build2_project2) { create(:ci_build, pipeline: pipeline2) } + let!(:build1_project3) { create(:ci_build, pipeline: pipeline3) } # these shouldn't influence the scheduling - let!(:unrelated_group) { create :group } - let!(:unrelated_project) { create :project, group_runners_enabled: true, group: unrelated_group } - let!(:unrelated_pipeline) { create :ci_pipeline, project: unrelated_project } - let!(:build1_unrelated_project) { create :ci_build, pipeline: unrelated_pipeline } - let!(:unrelated_group_runner) { create :ci_runner, groups: [unrelated_group] } + let!(:unrelated_group) { create(:group) } + let!(:unrelated_project) { create(:project, group_runners_enabled: true, group: unrelated_group) } + let!(:unrelated_pipeline) { create(:ci_pipeline, project: unrelated_project) } + let!(:build1_unrelated_project) { create(:ci_build, pipeline: unrelated_pipeline) } + let!(:unrelated_group_runner) { create(:ci_runner, :group, groups: [unrelated_group]) } it 'does not consider builds from other group runners' do expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 6 diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb index 74a23ed2a3f..d7c5d2b91aa 100644 --- a/spec/services/ci/update_build_queue_service_spec.rb +++ b/spec/services/ci/update_build_queue_service_spec.rb @@ -6,7 +6,7 @@ describe Ci::UpdateBuildQueueService do let(:pipeline) { create(:ci_pipeline, project: project) } context 'when updating specific runners' do - let(:runner) { create(:ci_runner) } + let(:runner) { create(:ci_runner, :project) } context 'when there is a runner that can pick build' do before do @@ -26,7 +26,7 @@ describe Ci::UpdateBuildQueueService do end context 'when updating shared runners' do - let(:runner) { create(:ci_runner, :shared) } + let(:runner) { create(:ci_runner, :instance) } context 'when there is no runner that can pick build' do it 'ticks runner queue value' do @@ -56,9 +56,9 @@ describe Ci::UpdateBuildQueueService do end context 'when updating group runners' do - let(:group) { create :group } - let(:project) { create :project, group: group } - let(:runner) { create :ci_runner, groups: [group] } + let(:group) { create(:group) } + let(:project) { create(:project, group: group) } + let(:runner) { create(:ci_runner, :group, groups: [group]) } context 'when there is a runner that can pick build' do it 'ticks runner queue value' do |