Improve runner_type validations for Ci::Runner

app/models/ci/runner.rb

@@ -57,7 +57,9 @@ class Runner < ActiveRecord::Base
     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
@@ -254,13 +256,21 @@ def assignable_for?(project_id)
       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
-      if assigned_to_group? && assigned_to_project?
-        errors.add(:runner, 'can only be assigned either to projects or to a group')
+    end
+    def only_one_group
+      if groups.many?
+        errors.add(:runner, 'can only be assigned to one group')
       end
     end

spec/controllers/groups/runners_controller_spec.rb

@@ -3,7 +3,7 @@
 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
     {

spec/factories/ci/runners.rb

@@ -21,6 +21,19 @@
       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

spec/features/admin/admin_runners_spec.rb

@@ -62,7 +62,7 @@
     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

spec/features/runners_spec.rb

@@ -28,8 +28,8 @@
       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 @@
       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 @@
     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 @@
       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 @@
       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 @@
   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 @@
     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)

spec/models/ci/runner_spec.rb

@@ -21,60 +21,50 @@
       end
     end
-    context 'either_projects_or_group' do
+    context '#only_one_group' do
       let(:group) { create(:group) }
-      it 'disallows assigning to a group if already assigned to a group' do
-        runner = create(:ci_runner, groups: [group])
-
+      let(:runner) { create(:ci_runner, :group, 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
-      it 'disallows assigning to a group if already assigned to a project' do
-        project = create(:project)
-        runner = create(:ci_runner, projects: [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']
+    end
+    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) }
+      it 'disallows assigning group to project_type runner' do
+        project_runner.groups << build(: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)
-        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']
+      it 'disallows assigning group to instance_type runner' do
+        instance_runner.groups << build(: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)
-        expect(runner).to be_valid
+      it 'disallows assigning project to group_type runner' do
+        group_runner.projects << build(:project)
+        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)
-        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
+      it 'disallows assigning project to instance_type runner' do
+        instance_runner.projects << build(:project)
+        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 '.shared' do
     let(:group) { create(:group) }
     let(:project) { create(:project) }
-    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]
+    let!(:group_runner) { create(:ci_runner, :group) }
+    let!(:project_runner) { create(:ci_runner, :project) }
+    let!(:shared_runner) { create(:ci_runner, :instance) }
+    it 'returns only shared runners' do
+      expect(described_class.shared).to contain_exactly(shared_runner)
     end
   end
@@ -141,17 +126,17 @@
   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 @@
       # 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 @@
     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 @@ def expect_redis_update
     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 @@ def expect_redis_update
     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 }

spec/models/project_spec.rb

@@ -1367,7 +1367,7 @@ def create_pipeline
     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 }
@@ -1408,7 +1408,7 @@ def create_pipeline
   end
   describe '#shared_runners' do
-    let!(:runner) { create(:ci_runner, :shared) }
+    let!(:runner) { create(:ci_runner, :instance) }
     subject { project.shared_runners }

spec/requests/api/runners_spec.rb

@@ -27,7 +27,7 @@
     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

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

spec/services/ci/update_build_queue_service_spec.rb

@@ -6,7 +6,7 @@
   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 @@
   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 @@
   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