Commit ab489d29 authored by Dylan Griffith's avatar Dylan Griffith

Improve runner_type validations for Ci::Runner

parent ec1d3e10
......@@ -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
......
......@@ -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
{
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -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 }
......
......@@ -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 }
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment