From ab489d293d6ee3e30673817ce4652c7b413988c0 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Fri, 11 May 2018 12:03:40 +0200
Subject: [PATCH 01/26] Improve runner_type validations for Ci::Runner

---
 app/models/ci/runner.rb                       |  22 +++-
 .../groups/runners_controller_spec.rb         |   2 +-
 spec/factories/ci/runners.rb                  |  13 +++
 spec/features/admin/admin_runners_spec.rb     |   2 +-
 spec/features/runners_spec.rb                 |  22 ++--
 spec/models/ci/runner_spec.rb                 | 107 ++++++++----------
 spec/models/project_spec.rb                   |   4 +-
 spec/requests/api/runners_spec.rb             |   2 +-
 spec/services/ci/register_job_service_spec.rb |  30 ++---
 .../ci/update_build_queue_service_spec.rb     |  10 +-
 10 files changed, 111 insertions(+), 103 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,14 +255,22 @@ module Ci
       self.class.owned_or_shared(project_id).where(id: self.id).any?
     end
 
-    def either_projects_or_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
+
+    def only_one_group
       if groups.many?
         errors.add(:runner, 'can only be assigned to one group')
       end
-
-      if assigned_to_group? && assigned_to_project?
-        errors.add(:runner, 'can only be assigned either to projects or to a group')
-      end
     end
 
     def accepting_tags?(build)
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
+
+    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 group if already assigned to a project' do
-        project = create(:project)
-        runner = create(:ci_runner, projects: [project])
+      it 'disallows assigning group to instance_type runner' do
+        instance_runner.groups << build(:group)
 
-        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 'disallows assigning to a project if already assigned to a group' do
-        runner = create(:ci_runner, groups: [group])
+      it 'disallows assigning project to group_type runner' do
+        group_runner.projects << build(:project)
 
-        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']
+        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 group if not assigned to a group nor a project' do
-        runner = create(:ci_runner)
+      it 'disallows assigning project to instance_type runner' do
+        instance_runner.projects << build(:project)
 
-        runner.groups << build(:group)
-
-        expect(runner).to be_valid
-      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
+        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

From 0a2f5065f26f259ed66359f3754ecc21505fea0d Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Mon, 14 May 2018 15:54:47 +0200
Subject: [PATCH 02/26] Ensure we validate Runner#runner_type when persisting
 RunnerNamespace

---
 app/models/ci/runner_namespace.rb | 2 +-
 spec/models/ci/runner_spec.rb     | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/app/models/ci/runner_namespace.rb b/app/models/ci/runner_namespace.rb
index 3269f86e8ca..420e34df091 100644
--- a/app/models/ci/runner_namespace.rb
+++ b/app/models/ci/runner_namespace.rb
@@ -2,7 +2,7 @@ module Ci
   class RunnerNamespace < ActiveRecord::Base
     extend Gitlab::Ci::Model
 
-    belongs_to :runner
+    belongs_to :runner, validate: true
     belongs_to :namespace, class_name: '::Namespace'
     belongs_to :group, class_name: '::Group', foreign_key: :namespace_id
   end
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 4dc990350d2..51d5e666cd6 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -66,6 +66,13 @@ describe Ci::Runner do
         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
+
+      it 'should fail to save a group assigned to a project runner even if the runner is already saved' do
+        group_runner
+
+        expect { create(:group, runners: [project_runner]) }
+          .to raise_error(ActiveRecord::RecordInvalid)
+      end
     end
   end
 

From 5c34c3fcd5f100b401b59d5a0f8e4fa0c899c8f5 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Sun, 27 May 2018 13:49:43 +0200
Subject: [PATCH 03/26] Fix weird Rails bug that leads to `runner_id=null` in
 SQL query

---
 app/services/ci/register_job_service.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb
index 4291631913a..9d288ca8038 100644
--- a/app/services/ci/register_job_service.rb
+++ b/app/services/ci/register_job_service.rb
@@ -89,7 +89,9 @@ module Ci
     end
 
     def builds_for_group_runner
-      hierarchy_groups = Gitlab::GroupHierarchy.new(runner.groups).base_and_descendants
+      # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL`
+      groups = Group.joins(:runner_namespaces).merge(runner.runner_namespaces)
+      hierarchy_groups = Gitlab::GroupHierarchy.new(groups).base_and_descendants
       projects = Project.where(namespace_id: hierarchy_groups)
         .with_group_runners_enabled
         .with_builds_enabled

From 051f385e7e82130e6978cd3956e5c48fbdc83b2e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Wed, 23 May 2018 13:23:49 +0200
Subject: [PATCH 04/26] Refactor validations and make runner factory by default
 to be instance-wide runner

---
 app/models/ci/runner.rb                       |  35 +++--
 .../groups/runners_controller_spec.rb         |   3 +-
 .../projects/runners_controller_spec.rb       |   3 +-
 .../settings/ci_cd_controller_spec.rb         |   8 +-
 spec/factories/ci/runner_projects.rb          |   2 +-
 spec/factories/ci/runners.rb                  |  34 ++---
 spec/features/admin/admin_runners_spec.rb     |  17 ++-
 spec/features/runners_spec.rb                 |   9 +-
 spec/finders/runner_jobs_finder_spec.rb       |   2 +-
 spec/models/ci/build_spec.rb                  |  10 +-
 spec/models/ci/pipeline_spec.rb               |   2 +-
 spec/models/ci/runner_spec.rb                 | 128 ++++++++++--------
 spec/models/project_spec.rb                   |  10 +-
 spec/models/user_spec.rb                      |   3 +-
 spec/requests/api/runner_spec.rb              |   8 +-
 spec/requests/api/runners_spec.rb             |   2 +-
 spec/serializers/runner_entity_spec.rb        |   4 +-
 spec/services/ci/register_job_service_spec.rb |   2 +-
 .../ci/update_build_queue_service_spec.rb     |   6 +-
 19 files changed, 141 insertions(+), 147 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 2119b70c18c..ac9f04bb3d4 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -56,12 +56,15 @@ module Ci
     end
 
     validate :tag_constraints
-    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
 
+    validate :no_projects, unless: :project_type?
+    validate :no_groups, unless: :group_type?
+    validate :any_project, if: :project_type?
+    validate :exactly_one_group, if: :group_type?
+    validate :is_shared_is_valid
+
     acts_as_taggable
 
     after_destroy :cleanup_runner_queue
@@ -117,8 +120,8 @@ module Ci
         raise ArgumentError, 'Transitioning a group runner to a project runner is not supported'
       end
 
-      self.save
-      project.runner_projects.create(runner_id: self.id)
+      self.projects << project
+      self.save!
     end
 
     def display_name
@@ -257,19 +260,31 @@ module Ci
 
     def no_projects
       if projects.any?
-        errors.add(:runner, 'cannot assign project to a non-project runner')
+        errors.add(:runner, 'cannot have projects assigned')
       end
     end
 
     def no_groups
       if groups.any?
-        errors.add(:runner, 'cannot assign group to a non-group runner')
+        errors.add(:runner, 'cannot have groups assigned')
       end
     end
 
-    def only_one_group
-      if groups.many?
-        errors.add(:runner, 'can only be assigned to one group')
+    def any_project
+      unless projects.any?
+        errors.add(:runner, 'needs to be assigned to at least one project')
+      end
+    end
+
+    def exactly_one_group
+      unless groups.one?
+        errors.add(:runner, 'needs to be assigned to exactly one group')
+      end
+    end
+
+    def is_shared_is_valid
+      unless is_shared? == instance_type?
+        errors.add(:is_shared, 'is not equal to instance_type?')
       end
     end
 
diff --git a/spec/controllers/groups/runners_controller_spec.rb b/spec/controllers/groups/runners_controller_spec.rb
index 49cb45ff7b0..5770d15557c 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, :group) }
+  let(:runner) { create(:ci_runner, :group, groups: [group]) }
 
   let(:params) do
     {
@@ -15,7 +15,6 @@ describe Groups::RunnersController do
   before do
     sign_in(user)
     group.add_master(user)
-    group.runners << runner
   end
 
   describe '#update' do
diff --git a/spec/controllers/projects/runners_controller_spec.rb b/spec/controllers/projects/runners_controller_spec.rb
index 89a13f3c976..2082dd2cff0 100644
--- a/spec/controllers/projects/runners_controller_spec.rb
+++ b/spec/controllers/projects/runners_controller_spec.rb
@@ -3,7 +3,7 @@ require 'spec_helper'
 describe Projects::RunnersController do
   let(:user) { create(:user) }
   let(:project) { create(:project) }
-  let(:runner) { create(:ci_runner) }
+  let(:runner) { create(:ci_runner, :project, projects: [project]) }
 
   let(:params) do
     {
@@ -16,7 +16,6 @@ describe Projects::RunnersController do
   before do
     sign_in(user)
     project.add_master(user)
-    project.runners << runner
   end
 
   describe '#update' do
diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb
index f1810763d2d..d53fe9bf734 100644
--- a/spec/controllers/projects/settings/ci_cd_controller_spec.rb
+++ b/spec/controllers/projects/settings/ci_cd_controller_spec.rb
@@ -19,12 +19,12 @@ describe Projects::Settings::CiCdController do
     end
 
     context 'with group runners' do
-      let(:group_runner) { create(:ci_runner, runner_type: :group_type) }
       let(:parent_group) { create(:group) }
-      let(:group) { create(:group, runners: [group_runner], parent: parent_group) }
+      let(:group) { create(:group, parent: parent_group) }
+      let(:group_runner) { create(:ci_runner, :group, groups: [group]) }
       let(:other_project) { create(:project, group: group) }
-      let!(:project_runner) { create(:ci_runner, projects: [other_project], runner_type: :project_type) }
-      let!(:shared_runner) { create(:ci_runner, :shared) }
+      let!(:project_runner) { create(:ci_runner, :project, projects: [other_project]) }
+      let!(:shared_runner) { create(:ci_runner, :instance) }
 
       it 'sets assignable project runners only' do
         group.add_master(user)
diff --git a/spec/factories/ci/runner_projects.rb b/spec/factories/ci/runner_projects.rb
index f605e90ceed..ec15972c423 100644
--- a/spec/factories/ci/runner_projects.rb
+++ b/spec/factories/ci/runner_projects.rb
@@ -1,6 +1,6 @@
 FactoryBot.define do
   factory :ci_runner_project, class: Ci::RunnerProject do
-    runner factory: :ci_runner
+    runner factory: [:ci_runner, :project]
     project
   end
 end
diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb
index 9d106250d08..354aa0f65fc 100644
--- a/spec/factories/ci/runners.rb
+++ b/spec/factories/ci/runners.rb
@@ -3,37 +3,27 @@ FactoryBot.define do
     sequence(:description) { |n| "My runner#{n}" }
 
     platform  "darwin"
-    is_shared false
     active    true
     access_level :not_protected
-    runner_type :project_type
 
-    trait :online do
-      contacted_at Time.now
-    end
-
-    trait :shared do
-      is_shared true
-      runner_type :instance_type
-    end
-
-    trait :specific do
-      is_shared false
-    end
-
-    trait :group do
-      runner_type :group_type
-    end
-
-    trait :project do
-      runner_type :project_type
-    end
+    is_shared true
+    runner_type :instance_type
 
     trait :instance do
       is_shared true
       runner_type :instance_type
     end
 
+    trait :group do
+      is_shared false
+      runner_type :group_type
+    end
+
+    trait :project do
+      is_shared false
+      runner_type :project_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 5c9bad8fb2c..be8754a5315 100644
--- a/spec/features/admin/admin_runners_spec.rb
+++ b/spec/features/admin/admin_runners_spec.rb
@@ -76,7 +76,7 @@ describe "Admin Runners" do
 
     context 'shared runner' do
       it 'shows the label and does not show the project count' do
-        runner = create :ci_runner, :shared
+        runner = create :ci_runner, :instance
 
         visit admin_runners_path
 
@@ -90,7 +90,7 @@ describe "Admin Runners" do
     context 'specific runner' do
       it 'shows the label and the project count' do
         project = create :project
-        runner = create :ci_runner, projects: [project]
+        runner = create :ci_runner, :project, projects: [project]
 
         visit admin_runners_path
 
@@ -149,8 +149,9 @@ describe "Admin Runners" do
       end
 
       context 'with specific runner' do
+        let(:runner) { create(:ci_runner, :project, projects: [@project1]) }
+
         before do
-          @project1.runners << runner
           visit admin_runner_path(runner)
         end
 
@@ -158,9 +159,9 @@ describe "Admin Runners" do
       end
 
       context 'with locked runner' do
+        let(:runner) { create(:ci_runner, :project, projects: [@project1], locked: true) }
+
         before do
-          runner.update(locked: true)
-          @project1.runners << runner
           visit admin_runner_path(runner)
         end
 
@@ -168,9 +169,10 @@ describe "Admin Runners" do
       end
 
       context 'with shared runner' do
+        let(:runner) { create(:ci_runner, :instance) }
+
         before do
           @project1.destroy
-          runner.update(is_shared: true)
           visit admin_runner_path(runner)
         end
 
@@ -179,8 +181,9 @@ describe "Admin Runners" do
     end
 
     describe 'disable/destroy' do
+      let(:runner) { create(:ci_runner, :project, projects: [@project1]) }
+
       before do
-        @project1.runners << runner
         visit admin_runner_path(runner)
       end
 
diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb
index f905e6d4f5e..9942de526d8 100644
--- a/spec/features/runners_spec.rb
+++ b/spec/features/runners_spec.rb
@@ -29,11 +29,7 @@ feature 'Runners' do
     end
 
     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
-      end
+      given(:specific_runner) { create(:ci_runner, :project, projects: [specific_runner]) }
 
       scenario 'user sees the specific runner' do
         visit project_runners_path(project)
@@ -126,11 +122,10 @@ feature 'Runners' do
 
     context 'when a specific runner exists in another project' do
       given(:another_project) { create(:project) }
-      given(:specific_runner) { create(:ci_runner, :project) }
+      given(:specific_runner) { create(:ci_runner, :project, projects: [another_project]) }
 
       background do
         another_project.add_master(user)
-        another_project.runners << specific_runner
       end
 
       scenario 'user enables and disables a specific runner' do
diff --git a/spec/finders/runner_jobs_finder_spec.rb b/spec/finders/runner_jobs_finder_spec.rb
index 4275b1a7ff1..97304170c4e 100644
--- a/spec/finders/runner_jobs_finder_spec.rb
+++ b/spec/finders/runner_jobs_finder_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe RunnerJobsFinder do
   let(:project) { create(:project) }
-  let(:runner) { create(:ci_runner, :shared) }
+  let(:runner) { create(:ci_runner, :instance) }
 
   subject { described_class.new(runner, params).execute }
 
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 77179028ede..b5eac913639 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -148,10 +148,9 @@ describe Ci::Build do
     end
 
     context 'when there are runners' do
-      let(:runner) { create(:ci_runner) }
+      let(:runner) { create(:ci_runner, :project, projects: [build.project]) }
 
       before do
-        build.project.runners << runner
         runner.update_attributes(contacted_at: 1.second.ago)
       end
 
@@ -1388,12 +1387,7 @@ describe Ci::Build do
       it { is_expected.to be_truthy }
 
       context "and there are specific runner" do
-        let(:runner) { create(:ci_runner, contacted_at: 1.second.ago) }
-
-        before do
-          build.project.runners << runner
-          runner.save
-        end
+        let!(:runner) { create(:ci_runner, :project, projects: [build.project], contacted_at: 1.second.ago) }
 
         it { is_expected.to be_falsey }
       end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index e4f4c62bd22..f5295bec65b 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -1589,7 +1589,7 @@ describe Ci::Pipeline, :mailer do
 
     context 'when pipeline is not stuck' do
       before do
-        create(:ci_runner, :shared, :online)
+        create(:ci_runner, :instance, :online)
       end
 
       it 'is not stuck' do
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 51d5e666cd6..2bd6e16a6cd 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -21,7 +21,7 @@ describe Ci::Runner do
       end
     end
 
-    context '#only_one_group' do
+    context '#exactly_one_group' do
       let(:group) { create(:group) }
       let(:runner) { create(:ci_runner, :group, groups: [group]) }
 
@@ -29,42 +29,43 @@ describe Ci::Runner do
         runner.groups << build(:group)
 
         expect(runner).not_to be_valid
-        expect(runner.errors.full_messages).to include('Runner can only be assigned to one group')
+        expect(runner.errors.full_messages).to include('Runner needs to be assigned to exactly one group')
       end
     end
 
     context 'runner_type validations' do
-      let(:group) { create(:group) }
+      set(:group) { create(:group) }
+      set(:project) { create(:project) }
       let(:group_runner) { create(:ci_runner, :group, groups: [group]) }
-      let(:project_runner) { create(:ci_runner, :project) }
+      let(:project_runner) { create(:ci_runner, :project, projects: [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')
+        expect(project_runner.errors.full_messages).to include('Runner cannot have groups assigned')
       end
 
       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')
+        expect(instance_runner.errors.full_messages).to include('Runner cannot have groups assigned')
       end
 
       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')
+        expect(group_runner.errors.full_messages).to include('Runner cannot have projects assigned')
       end
 
       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')
+        expect(instance_runner.errors.full_messages).to include('Runner cannot have projects assigned')
       end
 
       it 'should fail to save a group assigned to a project runner even if the runner is already saved' do
@@ -107,8 +108,8 @@ 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!(:group_runner) { create(:ci_runner, :group, groups: [group]) }
+    let!(:project_runner) { create(:ci_runner, :project, projects: [project]) }
     let!(:shared_runner) { create(:ci_runner, :instance) }
 
     it 'returns only shared runners' do
@@ -120,11 +121,11 @@ describe Ci::Runner do
     it 'returns the specific project runner' do
       # own
       specific_project = create(:project)
-      specific_runner = create(:ci_runner, :specific, projects: [specific_project])
+      specific_runner = create(:ci_runner, :project, projects: [specific_project])
 
       # other
       other_project = create(:project)
-      create(:ci_runner, :specific, projects: [other_project])
+      create(:ci_runner, :project, projects: [other_project])
 
       expect(described_class.belonging_to_project(specific_project.id)).to eq [specific_runner]
     end
@@ -175,31 +176,32 @@ describe Ci::Runner do
 
   describe '#display_name' do
     it 'returns the description if it has a value' do
-      runner = FactoryBot.build(:ci_runner, description: 'Linux/Ruby-1.9.3-p448')
+      runner = build(:ci_runner, description: 'Linux/Ruby-1.9.3-p448')
       expect(runner.display_name).to eq 'Linux/Ruby-1.9.3-p448'
     end
 
     it 'returns the token if it does not have a description' do
-      runner = FactoryBot.create(:ci_runner)
+      runner = create(:ci_runner)
       expect(runner.display_name).to eq runner.description
     end
 
     it 'returns the token if the description is an empty string' do
-      runner = FactoryBot.build(:ci_runner, description: '', token: 'token')
+      runner = build(:ci_runner, description: '', token: 'token')
       expect(runner.display_name).to eq runner.token
     end
   end
 
   describe '#assign_to' do
-    let!(:project) { FactoryBot.create(:project) }
+    let(:project) { create(:project) }
 
     subject { runner.assign_to(project) }
 
     context 'with shared_runner' do
-      let!(:runner) { FactoryBot.create(:ci_runner, :shared) }
+      let(:runner) { create(:ci_runner, :instance) }
 
       it 'transitions shared runner to project runner and assigns project' do
         subject
+
         expect(runner).to be_specific
         expect(runner).to be_project_type
         expect(runner.projects).to eq([project])
@@ -208,7 +210,8 @@ describe Ci::Runner do
     end
 
     context 'with group runner' do
-      let!(:runner) { FactoryBot.create(:ci_runner, :group) }
+      let(:group) { create(:group) }
+      let(:runner) { create(:ci_runner, :group, groups: [group]) }
 
       it 'raises an error' do
         expect { subject }
@@ -221,15 +224,15 @@ describe Ci::Runner do
     subject { described_class.online }
 
     before do
-      @runner1 = FactoryBot.create(:ci_runner, :shared, contacted_at: 1.year.ago)
-      @runner2 = FactoryBot.create(:ci_runner, :shared, contacted_at: 1.second.ago)
+      @runner1 = create(:ci_runner, :instance, contacted_at: 1.year.ago)
+      @runner2 = create(:ci_runner, :instance, contacted_at: 1.second.ago)
     end
 
     it { is_expected.to eq([@runner2])}
   end
 
   describe '#online?' do
-    let(:runner) { FactoryBot.create(:ci_runner, :shared) }
+    let(:runner) { create(:ci_runner, :instance) }
 
     subject { runner.online? }
 
@@ -299,21 +302,20 @@ describe Ci::Runner do
   end
 
   describe '#can_pick?' do
-    let(:pipeline) { create(:ci_pipeline) }
+    set(:pipeline) { create(:ci_pipeline) }
     let(:build) { create(:ci_build, pipeline: pipeline) }
-    let(:runner) { create(:ci_runner, tag_list: tag_list, run_untagged: run_untagged) }
+    let(:runner_project) { build.project }
+    let(:runner) { create(:ci_runner, :project, projects: [runner_project], tag_list: tag_list, run_untagged: run_untagged) }
     let(:tag_list) { [] }
     let(:run_untagged) { true }
 
     subject { runner.can_pick?(build) }
 
-    before do
-      build.project.runners << runner
-    end
-
     context 'a different runner' do
+      let(:other_project) { create(:project) }
+      let(:other_runner) { create(:ci_runner, :project, projects: [other_project], tag_list: tag_list, run_untagged: run_untagged) }
+ 
       it 'cannot handle builds' do
-        other_runner = create(:ci_runner)
         expect(other_runner.can_pick?(build)).to be_falsey
       end
     end
@@ -367,18 +369,14 @@ describe Ci::Runner do
     end
 
     context 'when runner is shared' do
-      let(:runner) { create(:ci_runner, :shared) }
-
-      before do
-        build.project.runners = []
-      end
+      let(:runner) { create(:ci_runner, :instance) }
 
       it 'can handle builds' do
         expect(runner.can_pick?(build)).to be_truthy
       end
 
       context 'when runner is locked' do
-        let(:runner) { create(:ci_runner, :shared, locked: true) }
+        let(:runner) { create(:ci_runner, :instance, locked: true) }
 
         it 'can handle builds' do
           expect(runner.can_pick?(build)).to be_truthy
@@ -393,10 +391,8 @@ describe Ci::Runner do
         end
       end
 
-      context 'when runner is not assigned to a project' do
-        before do
-          build.project.runners = []
-        end
+      context 'when runner is assigned to another project' do
+        let(:runner_project) { create(:project) }
 
         it 'cannot handle builds' do
           expect(runner.can_pick?(build)).to be_falsey
@@ -404,10 +400,8 @@ describe Ci::Runner do
       end
 
       context 'when runner is assigned to a group' do
-        before do
-          build.project.runners = []
-          runner.groups << create(:group, projects: [build.project])
-        end
+        let(:group) { create(:group, projects: [build.project]) }
+        let(:runner) { create(:ci_runner, :group, tag_list: tag_list, run_untagged: run_untagged, groups: [group]) }
 
         it 'can handle builds' do
           expect(runner.can_pick?(build)).to be_truthy
@@ -461,7 +455,7 @@ describe Ci::Runner do
   end
 
   describe '#status' do
-    let(:runner) { FactoryBot.create(:ci_runner, :shared, contacted_at: 1.second.ago) }
+    let(:runner) { create(:ci_runner, :instance, contacted_at: 1.second.ago) }
 
     subject { runner.status }
 
@@ -618,15 +612,32 @@ describe Ci::Runner do
   end
 
   describe '.assignable_for' do
-    let!(:unlocked_project_runner) { create(:ci_runner, runner_type: :project_type, projects: [project]) }
-    let!(:locked_project_runner) { create(:ci_runner, runner_type: :project_type, locked: true, projects: [project]) }
-    let!(:group_runner) { create(:ci_runner, runner_type: :group_type) }
-    let!(:instance_runner) { create(:ci_runner, :shared) }
+    let(:runner) { create(:ci_runner, :project, projects: [project]) }
     let(:project) { create(:project) }
     let(:another_project) { create(:project) }
 
-    context 'with already assigned project' do
-      subject { described_class.assignable_for(project) }
+    context 'with shared runners' do
+      let(:runner) { create(:ci_runner, :instance) }
+
+      context 'does not give owned runner' do
+        subject { described_class.assignable_for(project) }
+
+        it { is_expected.to be_empty }
+      end
+
+      context 'does not give shared runner' do
+        subject { described_class.assignable_for(another_project) }
+
+        it { is_expected.to be_empty }
+      end
+    end
+
+    context 'with unlocked runner' do
+      context 'does not give owned runner' do
+        subject { described_class.assignable_for(project) }
+
+        it { is_expected.to be_empty }
+      end
 
       it { is_expected.to be_empty }
     end
@@ -643,19 +654,16 @@ describe Ci::Runner do
 
   describe "belongs_to_one_project?" do
     it "returns false if there are two projects runner assigned to" do
-      runner = FactoryBot.create(:ci_runner)
-      project = FactoryBot.create(:project)
-      project1 = FactoryBot.create(:project)
-      project.runners << runner
-      project1.runners << runner
+      project1 = create(:project)
+      project2 = create(:project)
+      runner = create(:ci_runner, :project, projects: [project1, project2])
 
       expect(runner.belongs_to_one_project?).to be_falsey
     end
 
     it "returns true" do
-      runner = FactoryBot.create(:ci_runner)
-      project = FactoryBot.create(:project)
-      project.runners << runner
+      project = create(:project)
+      runner = create(:ci_runner, :project, projects: [project])
 
       expect(runner.belongs_to_one_project?).to be_truthy
     end
@@ -705,14 +713,14 @@ describe Ci::Runner do
     subject { runner.assigned_to_group? }
 
     context 'when project runner' do
-      let(:runner) { create(:ci_runner, description: 'Project runner', projects: [project]) }
+      let(:runner) { create(:ci_runner, :project, description: 'Project runner', projects: [project]) }
       let(:project) { create(:project) }
 
       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
@@ -740,7 +748,7 @@ describe Ci::Runner do
     end
 
     context 'when project runner' do
-      let(:runner) { create(:ci_runner, description: 'Project runner', projects: [project]) }
+      let(:runner) { create(:ci_runner, :project, 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 be471f198ff..9a76452a808 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1177,8 +1177,8 @@ describe Project do
   describe '#any_runners?' do
     context 'shared runners' do
       let(:project) { create(:project, shared_runners_enabled: shared_runners_enabled) }
-      let(:specific_runner) { create(:ci_runner) }
-      let(:shared_runner) { create(:ci_runner, :shared) }
+      let(:specific_runner) { create(:ci_runner, :project, projects: [project]) }
+      let(:shared_runner) { create(:ci_runner, :instance) }
 
       context 'for shared runners disabled' do
         let(:shared_runners_enabled) { false }
@@ -1188,7 +1188,7 @@ describe Project do
         end
 
         it 'has a specific runner' do
-          project.runners << specific_runner
+          specific_runner
 
           expect(project.any_runners?).to be_truthy
         end
@@ -1200,13 +1200,13 @@ describe Project do
         end
 
         it 'checks the presence of specific runner' do
-          project.runners << specific_runner
+          specific_runner
 
           expect(project.any_runners? { |runner| runner == specific_runner }).to be_truthy
         end
 
         it 'returns false if match cannot be found' do
-          project.runners << specific_runner
+          specific_runner
 
           expect(project.any_runners? { false }).to be_falsey
         end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 6a2f4a39f09..a5c364b3543 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1858,8 +1858,7 @@ describe User do
 
   describe '#ci_owned_runners' do
     let(:user) { create(:user) }
-    let(:runner_1) { create(:ci_runner) }
-    let(:runner_2) { create(:ci_runner) }
+    let(:runner) { create(:ci_runner, :project, projects: [project]) }
 
     context 'without any projects nor groups' do
       let!(:project) { create(:project, runners: [runner_1]) }
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index 6aadf839dbd..c3c8d95dded 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -262,16 +262,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
   describe '/api/v4/jobs' do
     let(:project) { create(:project, shared_runners_enabled: false) }
     let(:pipeline) { create(:ci_pipeline_without_jobs, project: project, ref: 'master') }
-    let(:runner) { create(:ci_runner) }
+    let(:runner) { create(:ci_runner, :project, projects: [project]) }
     let(:job) do
       create(:ci_build, :artifacts, :extended_options,
              pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0, commands: "ls\ndate")
     end
 
-    before do
-      project.runners << runner
-    end
-
     describe 'POST /api/v4/jobs/request' do
       let!(:last_update) {}
       let!(:new_update) { }
@@ -379,7 +375,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
         end
 
         context 'when shared runner requests job for project without shared_runners_enabled' do
-          let(:runner) { create(:ci_runner, :shared) }
+          let(:runner) { create(:ci_runner, :instance) }
 
           it_behaves_like 'no jobs available'
         end
diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb
index 7050418238c..532afde2db1 100644
--- a/spec/requests/api/runners_spec.rb
+++ b/spec/requests/api/runners_spec.rb
@@ -11,7 +11,7 @@ describe API::Runners do
   let(:group) { create(:group).tap { |group| group.add_owner(user) } }
   let(:group2) { create(:group).tap { |group| group.add_owner(user) } }
 
-  let!(:shared_runner) { create(:ci_runner, :shared, description: 'Shared runner') }
+  let!(:shared_runner) { create(:ci_runner, :instance, description: 'Shared runner') }
   let!(:unused_project_runner) { create(:ci_runner) }
 
   let!(:project_runner) do
diff --git a/spec/serializers/runner_entity_spec.rb b/spec/serializers/runner_entity_spec.rb
index 439ba2cbca2..ba99d568eba 100644
--- a/spec/serializers/runner_entity_spec.rb
+++ b/spec/serializers/runner_entity_spec.rb
@@ -1,10 +1,10 @@
 require 'spec_helper'
 
 describe RunnerEntity do
-  let(:runner) { create(:ci_runner, :specific) }
+  let(:project) { create(:project) }
+  let(:runner) { create(:ci_runner, :project, projects: [project]) }
   let(:entity) { described_class.new(runner, request: request, current_user: user) }
   let(:request) { double('request') }
-  let(:project) { create(:project) }
   let(:user) { create(:admin) }
 
   before do
diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb
index 5fed6e0f395..ab0266ad9b9 100644
--- a/spec/services/ci/register_job_service_spec.rb
+++ b/spec/services/ci/register_job_service_spec.rb
@@ -292,7 +292,7 @@ module Ci
       end
 
       context 'when access_level of runner is not_protected' do
-        let!(:specific_runner) { create(:ci_runner, :specific) }
+        let!(:specific_runner) { create(:ci_runner, :project, projects: [project]) }
 
         context 'when a job is protected' do
           let!(:pending_job) { create(:ci_build, :protected, pipeline: pipeline) }
diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb
index d7c5d2b91aa..e4b92956e48 100644
--- a/spec/services/ci/update_build_queue_service_spec.rb
+++ b/spec/services/ci/update_build_queue_service_spec.rb
@@ -6,13 +6,9 @@ describe Ci::UpdateBuildQueueService do
   let(:pipeline) { create(:ci_pipeline, project: project) }
 
   context 'when updating specific runners' do
-    let(:runner) { create(:ci_runner, :project) }
+    let(:runner) { create(:ci_runner, :project, projects: [project]) }
 
     context 'when there is a runner that can pick build' do
-      before do
-        build.project.runners << runner
-      end
-
       it 'ticks runner queue value' do
         expect { subject.execute(build) }.to change { runner.ensure_runner_queue_value }
       end

From 8d5d6ada5ed4630edf618f468565721be842e748 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Wed, 23 May 2018 13:55:59 +0200
Subject: [PATCH 05/26] Fix weird Rails bug that leads to `runner_id=null` in
 SQL query

---
 app/services/ci/register_job_service.rb       | 2 +-
 spec/services/ci/register_job_service_spec.rb | 8 ++------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb
index 9d288ca8038..28b97e3da67 100644
--- a/app/services/ci/register_job_service.rb
+++ b/app/services/ci/register_job_service.rb
@@ -90,7 +90,7 @@ module Ci
 
     def builds_for_group_runner
       # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL`
-      groups = Group.joins(:runner_namespaces).merge(runner.runner_namespaces)
+      groups = Group.joins(:runner_namespaces).where(runner_namespaces: { runner_id: runner })
       hierarchy_groups = Gitlab::GroupHierarchy.new(groups).base_and_descendants
       projects = Project.where(namespace_id: hierarchy_groups)
         .with_group_runners_enabled
diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb
index ab0266ad9b9..35374a09bd9 100644
--- a/spec/services/ci/register_job_service_spec.rb
+++ b/spec/services/ci/register_job_service_spec.rb
@@ -5,15 +5,11 @@ module Ci
     set(:group) { create(:group) }
     set(:project) { create(:project, group: group, shared_runners_enabled: false, group_runners_enabled: false) }
     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!(:shared_runner) { create(:ci_runner, :instance) }
+    let!(:specific_runner) { create(:ci_runner, :project, projects: [project]) }
     let!(:group_runner) { create(:ci_runner, :group, groups: [group]) }
     let!(:pending_job) { create(:ci_build, pipeline: pipeline) }
 
-    before do
-      specific_runner.assign_to(project)
-    end
-
     describe '#execute' do
       context 'runner follow tag list' do
         it "picks build with the same tag" do

From 1fcc9ad7bb60f83824e1e7ead8a0b07c459ab545 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Wed, 23 May 2018 14:03:01 +0200
Subject: [PATCH 06/26] Fix `register_job_service_spec` failures

---
 app/services/ci/register_job_service.rb       | 3 ++-
 spec/services/ci/register_job_service_spec.rb | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb
index 28b97e3da67..317d1defbba 100644
--- a/app/services/ci/register_job_service.rb
+++ b/app/services/ci/register_job_service.rb
@@ -90,7 +90,8 @@ module Ci
 
     def builds_for_group_runner
       # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL`
-      groups = Group.joins(:runner_namespaces).where(runner_namespaces: { runner_id: runner })
+      groups = Group.joins(:runner_namespaces).merge(runner.runner_namespaces)
+
       hierarchy_groups = Gitlab::GroupHierarchy.new(groups).base_and_descendants
       projects = Project.where(namespace_id: hierarchy_groups)
         .with_group_runners_enabled
diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb
index 35374a09bd9..3816bd0deb5 100644
--- a/spec/services/ci/register_job_service_spec.rb
+++ b/spec/services/ci/register_job_service_spec.rb
@@ -320,7 +320,7 @@ module Ci
       end
 
       context 'when access_level of runner is ref_protected' do
-        let!(:specific_runner) { create(:ci_runner, :ref_protected, :specific) }
+        let!(:specific_runner) { create(:ci_runner, :project, :ref_protected, projects: [project]) }
 
         context 'when a job is protected' do
           let!(:pending_job) { create(:ci_build, :protected, pipeline: pipeline) }

From 03ff1da278117ce36aaec4e0af267bbc07dc571c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Wed, 23 May 2018 15:29:48 +0200
Subject: [PATCH 07/26] Fix some failures

---
 app/models/clusters/applications/runner.rb |  5 +--
 spec/factories/ci/runners.rb               |  4 +++
 spec/requests/api/runners_spec.rb          | 40 ++--------------------
 3 files changed, 9 insertions(+), 40 deletions(-)

diff --git a/app/models/clusters/applications/runner.rb b/app/models/clusters/applications/runner.rb
index b881b4eaf36..e6f795f3e0b 100644
--- a/app/models/clusters/applications/runner.rb
+++ b/app/models/clusters/applications/runner.rb
@@ -43,7 +43,7 @@ module Clusters
 
       def create_and_assign_runner
         transaction do
-          project.runners.create!(runner_create_params).tap do |runner|
+          Ci::Runner.create!(runner_create_params).tap do |runner|
             update!(runner_id: runner.id)
           end
         end
@@ -53,7 +53,8 @@ module Clusters
         {
           name: 'kubernetes-cluster',
           runner_type: :project_type,
-          tag_list: %w(kubernetes cluster)
+          tag_list: %w(kubernetes cluster),
+          projects: [project]
         }
       end
 
diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb
index 354aa0f65fc..e9bbb9f36e8 100644
--- a/spec/factories/ci/runners.rb
+++ b/spec/factories/ci/runners.rb
@@ -9,6 +9,10 @@ FactoryBot.define do
     is_shared true
     runner_type :instance_type
 
+    trait :online do
+      contacted_at Time.now
+    end
+
     trait :instance do
       is_shared true
       runner_type :instance_type
diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb
index 532afde2db1..41cd93ac672 100644
--- a/spec/requests/api/runners_spec.rb
+++ b/spec/requests/api/runners_spec.rb
@@ -12,21 +12,8 @@ describe API::Runners do
   let(:group2) { create(:group).tap { |group| group.add_owner(user) } }
 
   let!(:shared_runner) { create(:ci_runner, :instance, description: 'Shared runner') }
-  let!(:unused_project_runner) { create(:ci_runner) }
-
-  let!(:project_runner) do
-    create(:ci_runner, description: 'Project runner').tap do |runner|
-      create(:ci_runner_project, runner: runner, project: project)
-    end
-  end
-
-  let!(:two_projects_runner) do
-    create(:ci_runner, description: 'Two projects runner').tap do |runner|
-      create(:ci_runner_project, runner: runner, project: project)
-      create(:ci_runner_project, runner: runner, project: project2)
-    end
-  end
-
+  let!(:project_runner) { create(:ci_runner, :project, description: 'Project runner', projects: [project]) }
+  let!(:two_projects_runner) { create(:ci_runner, :project, description: 'Two projects runner', projects: [project, project2])
   let!(:group_runner) { create(:ci_runner, :group, description: 'Group runner', groups: [group]) }
 
   before do
@@ -310,14 +297,6 @@ describe API::Runners do
       end
 
       context 'when runner is not shared' do
-        it 'deletes unused runner' do
-          expect do
-            delete api("/runners/#{unused_project_runner.id}", admin)
-
-            expect(response).to have_gitlab_http_status(204)
-          end.to change { Ci::Runner.specific.count }.by(-1)
-        end
-
         it 'deletes used project runner' do
           expect do
             delete api("/runners/#{project_runner.id}", admin)
@@ -586,13 +565,6 @@ describe API::Runners do
       end
 
       context 'user is admin' do
-        it 'enables any specific runner' do
-          expect do
-            post api("/projects/#{project.id}/runners", admin), runner_id: unused_project_runner.id
-          end.to change { project.runners.count }.by(+1)
-          expect(response).to have_gitlab_http_status(201)
-        end
-
         it 'enables a shared runner' do
           expect do
             post api("/projects/#{project.id}/runners", admin), runner_id: shared_runner.id
@@ -603,14 +575,6 @@ describe API::Runners do
         end
       end
 
-      context 'user is not admin' do
-        it 'does not enable runner without access to' do
-          post api("/projects/#{project.id}/runners", user), runner_id: unused_project_runner.id
-
-          expect(response).to have_gitlab_http_status(403)
-        end
-      end
-
       it 'raises an error when no runner_id param is provided' do
         post api("/projects/#{project.id}/runners", admin)
 

From da75618bebe57132f847df733625cc4757f8084d Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Thu, 24 May 2018 12:24:41 +0200
Subject: [PATCH 08/26] Make Ci::Runner#assign_to keep returning RunnerProject

---
 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 ac9f04bb3d4..5cec88660f8 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -120,8 +120,9 @@ module Ci
         raise ArgumentError, 'Transitioning a group runner to a project runner is not supported'
       end
 
-      self.projects << project
+      runner_project = project.runner_projects.create(runner_id: self.id)
       self.save!
+      runner_project
     end
 
     def display_name

From 85949ac151e77086d586c6e14007349fcb9680ce Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Thu, 24 May 2018 12:27:28 +0200
Subject: [PATCH 09/26] Fix spec/features/runners_spec.rb

---
 spec/features/runners_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb
index 9942de526d8..9ce7d538004 100644
--- a/spec/features/runners_spec.rb
+++ b/spec/features/runners_spec.rb
@@ -29,7 +29,7 @@ feature 'Runners' do
     end
 
     context 'when a project_type runner is activated on the project' do
-      given(:specific_runner) { create(:ci_runner, :project, projects: [specific_runner]) }
+      given!(:specific_runner) { create(:ci_runner, :project, projects: [project]) }
 
       scenario 'user sees the specific runner' do
         visit project_runners_path(project)
@@ -122,7 +122,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, :project, projects: [another_project]) }
+      given!(:specific_runner) { create(:ci_runner, :project, projects: [another_project]) }
 
       background do
         another_project.add_master(user)

From ec614728f676a684819087f9d08c153054639166 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Thu, 24 May 2018 12:43:48 +0200
Subject: [PATCH 10/26] Fix the conflict resolution in
 spec/models/ci/runner_spec.rb

---
 spec/models/ci/runner_spec.rb | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)

diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 2bd6e16a6cd..08578c27e37 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -612,32 +612,16 @@ describe Ci::Runner do
   end
 
   describe '.assignable_for' do
-    let(:runner) { create(:ci_runner, :project, projects: [project]) }
     let(:project) { create(:project) }
+    let(:group) { create(:group) }
     let(:another_project) { create(:project) }
+    let!(:unlocked_project_runner) { create(:ci_runner, :project, projects: [project]) }
+    let!(:locked_project_runner) { create(:ci_runner, :project, locked: true, projects: [project]) }
+    let!(:group_runner) { create(:ci_runner, :group, groups: [group]) }
+    let!(:instance_runner) { create(:ci_runner, :instance) }
 
-    context 'with shared runners' do
-      let(:runner) { create(:ci_runner, :instance) }
-
-      context 'does not give owned runner' do
-        subject { described_class.assignable_for(project) }
-
-        it { is_expected.to be_empty }
-      end
-
-      context 'does not give shared runner' do
-        subject { described_class.assignable_for(another_project) }
-
-        it { is_expected.to be_empty }
-      end
-    end
-
-    context 'with unlocked runner' do
-      context 'does not give owned runner' do
-        subject { described_class.assignable_for(project) }
-
-        it { is_expected.to be_empty }
-      end
+    context 'with already assigned project' do
+      subject { described_class.assignable_for(project) }
 
       it { is_expected.to be_empty }
     end

From bbd4cd118fc07df25c852058927c61ce582f19f9 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Sun, 27 May 2018 13:25:53 +0200
Subject: [PATCH 11/26] Fix spec for User#ci_owned_runners

---
 spec/models/user_spec.rb | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index a5c364b3543..decf53c44eb 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1858,12 +1858,10 @@ describe User do
 
   describe '#ci_owned_runners' do
     let(:user) { create(:user) }
+    let!(:project) { create(:project) }
     let(:runner) { create(:ci_runner, :project, projects: [project]) }
 
     context 'without any projects nor groups' do
-      let!(:project) { create(:project, runners: [runner_1]) }
-      let!(:group) { create(:group) }
-
       it 'does not load' do
         expect(user.ci_owned_runners).to be_empty
       end
@@ -1871,38 +1869,40 @@ describe User do
 
     context 'with personal projects runners' do
       let(:namespace) { create(:namespace, owner: user) }
-      let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) }
+      let!(:project) { create(:project, namespace: namespace) }
 
       it 'loads' do
-        expect(user.ci_owned_runners).to contain_exactly(runner_1)
+        expect(user.ci_owned_runners).to contain_exactly(runner)
       end
     end
 
     context 'with personal group runner' do
-      let!(:project) { create(:project, runners: [runner_1]) }
+      let!(:project) { create(:project) }
+      let(:group_runner) { create(:ci_runner, :group, groups: [group]) }
       let!(:group) do
-        create(:group, runners: [runner_2]).tap do |group|
+        create(:group).tap do |group|
           group.add_owner(user)
         end
       end
 
       it 'loads' do
-        expect(user.ci_owned_runners).to contain_exactly(runner_2)
+        expect(user.ci_owned_runners).to contain_exactly(group_runner)
       end
     end
 
     context 'with personal project and group runner' do
       let(:namespace) { create(:namespace, owner: user) }
-      let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) }
+      let!(:project) { create(:project, namespace: namespace) }
+      let!(:group_runner) { create(:ci_runner, :group, groups: [group]) }
 
       let!(:group) do
-        create(:group, runners: [runner_2]).tap do |group|
+        create(:group).tap do |group|
           group.add_owner(user)
         end
       end
 
       it 'loads' do
-        expect(user.ci_owned_runners).to contain_exactly(runner_1, runner_2)
+        expect(user.ci_owned_runners).to contain_exactly(runner, group_runner)
       end
     end
 
@@ -1913,7 +1913,7 @@ describe User do
         end
 
         it 'loads' do
-          expect(user.ci_owned_runners).to contain_exactly(runner_1)
+          expect(user.ci_owned_runners).to contain_exactly(runner)
         end
       end
 
@@ -1930,7 +1930,7 @@ describe User do
 
     context 'with groups projects runners' do
       let(:group) { create(:group) }
-      let!(:project) { create(:project, group: group, runners: [runner_1]) }
+      let!(:project) { create(:project, group: group) }
 
       def add_user(access)
         group.add_user(user, access)
@@ -1940,11 +1940,8 @@ describe User do
     end
 
     context 'with groups runners' do
-      let!(:group) do
-        create(:group, runners: [runner_1]).tap do |group|
-          group.add_owner(user)
-        end
-      end
+      let!(:runner) { create(:ci_runner, :group, groups: [group]) }
+      let!(:group) { create(:group) }
 
       def add_user(access)
         group.add_user(user, access)
@@ -1954,7 +1951,7 @@ describe User do
     end
 
     context 'with other projects runners' do
-      let!(:project) { create(:project, runners: [runner_1]) }
+      let!(:project) { create(:project) }
 
       def add_user(access)
         project.add_role(user, access)
@@ -1967,7 +1964,7 @@ describe User do
       let(:group) { create(:group) }
       let(:another_user) { create(:user) }
       let(:subgroup) { create(:group, parent: group) }
-      let!(:project) { create(:project, group: subgroup, runners: [runner_1]) }
+      let!(:project) { create(:project, group: subgroup) }
 
       def add_user(access)
         group.add_user(user, access)

From d9251f2ea047d359d1d7d4799d1ba84da5896f64 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Sun, 27 May 2018 13:43:05 +0200
Subject: [PATCH 12/26] Fix specs api/runners_spec.rb, api/v3/runners_spec.rb
 update_build_queue_service_spec.rb

---
 spec/requests/api/runners_spec.rb                   | 2 +-
 spec/services/ci/update_build_queue_service_spec.rb | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb
index 41cd93ac672..4dfc3b8cfc4 100644
--- a/spec/requests/api/runners_spec.rb
+++ b/spec/requests/api/runners_spec.rb
@@ -13,7 +13,7 @@ describe API::Runners do
 
   let!(:shared_runner) { create(:ci_runner, :instance, description: 'Shared runner') }
   let!(:project_runner) { create(:ci_runner, :project, description: 'Project runner', projects: [project]) }
-  let!(:two_projects_runner) { create(:ci_runner, :project, description: 'Two projects runner', projects: [project, project2])
+  let!(:two_projects_runner) { create(:ci_runner, :project, description: 'Two projects runner', projects: [project, project2]) }
   let!(:group_runner) { create(:ci_runner, :group, description: 'Group runner', groups: [group]) }
 
   before do
diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb
index e4b92956e48..ca0c6be5da6 100644
--- a/spec/services/ci/update_build_queue_service_spec.rb
+++ b/spec/services/ci/update_build_queue_service_spec.rb
@@ -15,6 +15,9 @@ describe Ci::UpdateBuildQueueService do
     end
 
     context 'when there is no runner that can pick build' do
+      let(:another_project) { create(:project) }
+      let(:runner) { create(:ci_runner, :project, projects: [another_project]) }
+
       it 'does not tick runner queue value' do
         expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value }
       end

From c6e95b04405f1e07f76505b03c6c096f4c4d084b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 12:49:08 +0200
Subject: [PATCH 13/26] Improve `Ci::Runner#assign_to` to return a flag whether
 it succeeded or not

---
 app/controllers/admin/runner_projects_controller.rb    | 4 +---
 app/controllers/projects/runner_projects_controller.rb | 3 +--
 app/models/ci/runner.rb                                | 5 ++---
 lib/api/runners.rb                                     | 4 +---
 spec/models/ci/runner_spec.rb                          | 2 +-
 5 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/app/controllers/admin/runner_projects_controller.rb b/app/controllers/admin/runner_projects_controller.rb
index 7ed2de71028..7aba77d8129 100644
--- a/app/controllers/admin/runner_projects_controller.rb
+++ b/app/controllers/admin/runner_projects_controller.rb
@@ -4,9 +4,7 @@ class Admin::RunnerProjectsController < Admin::ApplicationController
   def create
     @runner = Ci::Runner.find(params[:runner_project][:runner_id])
 
-    runner_project = @runner.assign_to(@project, current_user)
-
-    if runner_project.persisted?
+    if @runner.assign_to(@project, current_user)
       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 0ec2490655f..a080724634b 100644
--- a/app/controllers/projects/runner_projects_controller.rb
+++ b/app/controllers/projects/runner_projects_controller.rb
@@ -9,9 +9,8 @@ class Projects::RunnerProjectsController < Projects::ApplicationController
     return head(403) unless can?(current_user, :assign_runner, @runner)
 
     path = project_runners_path(project)
-    runner_project = @runner.assign_to(project, current_user)
 
-    if runner_project.persisted?
+    if @runner.assign_to(project, current_user)
       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 5cec88660f8..e2a1c9fb929 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -120,9 +120,8 @@ module Ci
         raise ArgumentError, 'Transitioning a group runner to a project runner is not supported'
       end
 
-      runner_project = project.runner_projects.create(runner_id: self.id)
-      self.save!
-      runner_project
+      self.projects << project
+      self.save
     end
 
     def display_name
diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index 5cb96d467c0..d9a42960cb6 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -133,9 +133,7 @@ module API
         runner = get_runner(params[:runner_id])
         authenticate_enable_runner!(runner)
 
-        runner_project = runner.assign_to(user_project)
-
-        if runner_project.persisted?
+        if runner.assign_to(user_project)
           present runner, with: Entities::Runner
         else
           conflict!("Runner was already enabled for this project")
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 08578c27e37..2f254956f92 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -200,7 +200,7 @@ describe Ci::Runner do
       let(:runner) { create(:ci_runner, :instance) }
 
       it 'transitions shared runner to project runner and assigns project' do
-        subject
+        expect(subject).to be_truthy
 
         expect(runner).to be_specific
         expect(runner).to be_project_type

From 782337b3f204102ee82d9f40351f150350354a8c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 12:55:13 +0200
Subject: [PATCH 14/26] Fix traits of runners factories

---
 spec/factories/ci/runners.rb | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb
index e9bbb9f36e8..c698b74c578 100644
--- a/spec/factories/ci/runners.rb
+++ b/spec/factories/ci/runners.rb
@@ -21,11 +21,19 @@ FactoryBot.define do
     trait :group do
       is_shared false
       runner_type :group_type
+
+      after(:build) do |runner, evaluator|
+        runner.groups << build(:group) if runner.groups.empty?
+      end
     end
 
     trait :project do
       is_shared false
       runner_type :project_type
+
+      after(:build) do |runner, evaluator|
+        runner.projects << build(:project) if runner.projects.empty?
+      end
     end
 
     trait :inactive do

From 53ef14c6765c09601e0ef2bd632741a78f84ae32 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 12:56:45 +0200
Subject: [PATCH 15/26] Fix static analysis

---
 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 e2a1c9fb929..5017c983425 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -63,7 +63,7 @@ module Ci
     validate :no_groups, unless: :group_type?
     validate :any_project, if: :project_type?
     validate :exactly_one_group, if: :group_type?
-    validate :is_shared_is_valid
+    validate :validate_is_shared
 
     acts_as_taggable
 
@@ -282,7 +282,7 @@ module Ci
       end
     end
 
-    def is_shared_is_valid
+    def validate_is_shared
       unless is_shared? == instance_type?
         errors.add(:is_shared, 'is not equal to instance_type?')
       end

From cfee3e0c4ef3a8b3bf6aafd325c791d0d89d68df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 13:07:28 +0200
Subject: [PATCH 16/26] Add `Ci::Runner` inverse_of's

---
 app/models/ci/runner.rb           | 4 ++--
 app/models/ci/runner_namespace.rb | 4 ++--
 app/models/ci/runner_project.rb   | 4 ++--
 app/models/namespace.rb           | 2 +-
 app/models/project.rb             | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 5017c983425..3f0476fb7cf 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -12,9 +12,9 @@ module Ci
     FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze
 
     has_many :builds
-    has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+    has_many :runner_projects, inverse_of: :runner, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
     has_many :projects, through: :runner_projects
-    has_many :runner_namespaces
+    has_many :runner_namespaces, inverse_of: :runner
     has_many :groups, through: :runner_namespaces
 
     has_one :last_build, ->() { order('id DESC') }, class_name: 'Ci::Build'
diff --git a/app/models/ci/runner_namespace.rb b/app/models/ci/runner_namespace.rb
index 420e34df091..26ca8bbdeeb 100644
--- a/app/models/ci/runner_namespace.rb
+++ b/app/models/ci/runner_namespace.rb
@@ -2,8 +2,8 @@ module Ci
   class RunnerNamespace < ActiveRecord::Base
     extend Gitlab::Ci::Model
 
-    belongs_to :runner, validate: true
-    belongs_to :namespace, class_name: '::Namespace'
+    belongs_to :runner, inverse_of: :runner_namespaces, validate: true
+    belongs_to :namespace, inverse_of: :runner_namespaces, class_name: '::Namespace'
     belongs_to :group, class_name: '::Group', foreign_key: :namespace_id
   end
 end
diff --git a/app/models/ci/runner_project.rb b/app/models/ci/runner_project.rb
index 505d178ba8e..52437047300 100644
--- a/app/models/ci/runner_project.rb
+++ b/app/models/ci/runner_project.rb
@@ -2,8 +2,8 @@ module Ci
   class RunnerProject < ActiveRecord::Base
     extend Gitlab::Ci::Model
 
-    belongs_to :runner
-    belongs_to :project
+    belongs_to :runner, inverse_of: :runner_projects
+    belongs_to :project, inverse_of: :runner_projects
 
     validates :runner_id, uniqueness: { scope: :project_id }
   end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 3dad4277713..52fe529c016 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -21,7 +21,7 @@ class Namespace < ActiveRecord::Base
   has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :project_statistics
 
-  has_many :runner_namespaces, class_name: 'Ci::RunnerNamespace'
+  has_many :runner_namespaces, inverse_of: :namespace, class_name: 'Ci::RunnerNamespace'
   has_many :runners, through: :runner_namespaces, source: :runner, class_name: 'Ci::Runner'
 
   # This should _not_ be `inverse_of: :namespace`, because that would also set
diff --git a/app/models/project.rb b/app/models/project.rb
index e275ac4dc6f..d2f360e4915 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -236,7 +236,7 @@ class Project < ActiveRecord::Base
   has_many :builds, class_name: 'Ci::Build', inverse_of: :project, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName'
   has_many :build_trace_chunks, class_name: 'Ci::BuildTraceChunk', through: :builds, source: :trace_chunks
-  has_many :runner_projects, class_name: 'Ci::RunnerProject'
+  has_many :runner_projects, class_name: 'Ci::RunnerProject', inverse_of: :project
   has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
   has_many :variables, class_name: 'Ci::Variable'
   has_many :triggers, class_name: 'Ci::Trigger'

From 385f37a724f8c63f551e7236649a3f28058b860b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 13:09:31 +0200
Subject: [PATCH 17/26] Improve runner registration API

---
 lib/api/runner.rb                | 16 +++++++++-------
 spec/requests/api/runner_spec.rb |  8 +++++---
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index 5b7ae89440c..e9886c76870 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -21,24 +21,26 @@ module API
         attributes = attributes_for_keys([:description, :active, :locked, :run_untagged, :tag_list, :maximum_timeout])
           .merge(get_runner_details_from_request)
 
-        runner =
+        attributes =
           if runner_registration_token_valid?
             # Create shared runner. Requires admin access
-            Ci::Runner.create(attributes.merge(is_shared: true, runner_type: :instance_type))
+            attributes.merge(is_shared: true, runner_type: :instance_type)
           elsif project = Project.find_by(runners_token: params[:token])
             # Create a specific runner for the project
-            project.runners.create(attributes.merge(runner_type: :project_type))
+            attributes.merge(is_shared: false, runner_type: :project_type, projects: [project])
           elsif group = Group.find_by(runners_token: params[:token])
             # Create a specific runner for the group
-            group.runners.create(attributes.merge(runner_type: :group_type))
+            attributes.merge(is_shared: false, runner_type: :group_type, groups: [group])
+          else
+            forbidden!
           end
 
-        break forbidden! unless runner
+        runner = Ci::Runner.create(attributes)
 
-        if runner.id
+        if runner.persisted?
           present runner, with: Entities::RunnerRegistrationDetails
         else
-          not_found!
+          render_validation_error!(runner)
         end
       end
 
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index c3c8d95dded..c63d894e3dd 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -115,7 +115,9 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             post api('/runners'), token: registration_token,
                                   run_untagged: false
 
-            expect(response).to have_gitlab_http_status 404
+            expect(response).to have_gitlab_http_status 400
+            expect(json_response['message']).to include(
+              'tags_list' => ['can not be empty when runner is not allowed to pick untagged jobs'])
           end
         end
       end
@@ -720,7 +722,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
               end
 
               context 'when runner specifies lower timeout' do
-                let(:runner) { create(:ci_runner, maximum_timeout: 1000) }
+                let(:runner) { create(:ci_runner, :project, maximum_timeout: 1000, projects: [project]) }
 
                 it 'contains info about timeout overridden by runner' do
                   request_job
@@ -731,7 +733,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
               end
 
               context 'when runner specifies bigger timeout' do
-                let(:runner) { create(:ci_runner, maximum_timeout: 2000) }
+                let(:runner) { create(:ci_runner, :project, maximum_timeout: 2000, projects: [project]) }
 
                 it 'contains info about timeout not overridden by runner' do
                   request_job

From 5805e92299f91a8d849418a03ed0e6cbcbbb5568 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 13:41:04 +0200
Subject: [PATCH 18/26] Improve Runners API validations

---
 lib/api/runners.rb                | 2 +-
 spec/requests/api/runners_spec.rb | 8 ++------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index d9a42960cb6..2b78075ddbf 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -136,7 +136,7 @@ module API
         if runner.assign_to(user_project)
           present runner, with: Entities::Runner
         else
-          conflict!("Runner was already enabled for this project")
+          render_validation_error!(runner)
         end
       end
 
diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb
index 4dfc3b8cfc4..cc326ff6484 100644
--- a/spec/requests/api/runners_spec.rb
+++ b/spec/requests/api/runners_spec.rb
@@ -522,11 +522,7 @@ describe API::Runners do
 
   describe 'POST /projects/:id/runners' do
     context 'authorized user' do
-      let(:project_runner2) do
-        create(:ci_runner).tap do |runner|
-          create(:ci_runner_project, runner: runner, project: project2)
-        end
-      end
+      let(:project_runner2) { create(:ci_runner, :project, projects: [project2]) }
 
       it 'enables specific runner' do
         expect do
@@ -539,7 +535,7 @@ describe API::Runners do
         expect do
           post api("/projects/#{project.id}/runners", user), runner_id: project_runner.id
         end.to change { project.runners.count }.by(0)
-        expect(response).to have_gitlab_http_status(409)
+        expect(response).to have_gitlab_http_status(400)
       end
 
       it 'does not enable locked runner' do

From adc860ae0e1113e01a586958d41f5916c713e5b5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 13:44:37 +0200
Subject: [PATCH 19/26] Ensure that we can remove degenerate runners

---
 spec/models/ci/runner_spec.rb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 2f254956f92..b808eb84b58 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -764,4 +764,20 @@ describe Ci::Runner do
       end
     end
   end
+
+  describe 'project runner without projects is destroyable' do
+    subject { create(:ci_runner, :project) }
+
+    before do
+      subject.runner_projects.delete_all
+    end
+
+    it 'does not have projects' do
+      expect(subject.runner_projects).to be_empty
+    end
+
+    it 'can be destroyed' do
+      expect { subject.destroy }.to change { Ci::Runner.count }.by(-1)
+    end
+  end
 end

From bf74e69eeb3d7881fe46ee34a26ba03a1744e8d0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 14:36:47 +0200
Subject: [PATCH 20/26] Add uniqueness for RunnerNamespace

---
 app/models/ci/runner_namespace.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/models/ci/runner_namespace.rb b/app/models/ci/runner_namespace.rb
index 26ca8bbdeeb..29508fdd326 100644
--- a/app/models/ci/runner_namespace.rb
+++ b/app/models/ci/runner_namespace.rb
@@ -5,5 +5,7 @@ module Ci
     belongs_to :runner, inverse_of: :runner_namespaces, validate: true
     belongs_to :namespace, inverse_of: :runner_namespaces, class_name: '::Namespace'
     belongs_to :group, class_name: '::Group', foreign_key: :namespace_id
+
+    validates :runner_id, uniqueness: { scope: :namespace_id }
   end
 end

From 2ccbe4fd341254c1d3146c81888881ace5f997c4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 15:31:46 +0200
Subject: [PATCH 21/26] Run `Ci::Runner#assign_to` in transaction

---
 app/models/ci/runner.rb | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 3f0476fb7cf..57edd6a4956 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -120,8 +120,15 @@ module Ci
         raise ArgumentError, 'Transitioning a group runner to a project runner is not supported'
       end
 
-      self.projects << project
-      self.save
+      begin
+        transaction do
+          self.projects << project
+          self.save!
+        end
+      rescue ActiveRecord::RecordInvalid => e
+        self.errors.add(:assign_to, e.message)
+        false
+      end
     end
 
     def display_name

From c7f013f07106f7e5cc7c4e47608deab602f67d5d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 15:32:31 +0200
Subject: [PATCH 22/26] Fix `static-analysis` changes

---
 spec/models/ci/runner_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index b808eb84b58..19ceb9ae4f2 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -314,7 +314,7 @@ describe Ci::Runner do
     context 'a different runner' do
       let(:other_project) { create(:project) }
       let(:other_runner) { create(:ci_runner, :project, projects: [other_project], tag_list: tag_list, run_untagged: run_untagged) }
- 
+
       it 'cannot handle builds' do
         expect(other_runner.can_pick?(build)).to be_falsey
       end

From 41b65d3514d6841ee29899ffda212f0c2d393a21 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Mon, 28 May 2018 17:14:15 +0200
Subject: [PATCH 23/26] Fix rubocop error in runner_spec.rb

---
 spec/models/ci/runner_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 19ceb9ae4f2..cd60e67cc01 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -777,7 +777,7 @@ describe Ci::Runner do
     end
 
     it 'can be destroyed' do
-      expect { subject.destroy }.to change { Ci::Runner.count }.by(-1)
+      expect { subject.destroy }.to change { described_class.count }.by(-1)
     end
   end
 end

From 47c11248636ede9b50bc3b6b4ac5de850ed3e0a9 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Mon, 28 May 2018 17:15:11 +0200
Subject: [PATCH 24/26] Fix test description in
 spec/requests/api/runner_spec.rb

---
 spec/requests/api/runner_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index c63d894e3dd..319ac389083 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -111,7 +111,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
         end
 
         context 'when tags are not provided' do
-          it 'returns 404 error' do
+          it 'returns 400 error' do
             post api('/runners'), token: registration_token,
                                   run_untagged: false
 

From c0c5f896b79635343d3651b40bcb875413ceedd9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 28 May 2018 17:58:46 +0200
Subject: [PATCH 25/26] Bring back deleted specs

---
 spec/factories/ci/runners.rb      |  8 ++++++++
 spec/models/ci/runner_spec.rb     |  6 +-----
 spec/requests/api/runners_spec.rb | 33 +++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb
index c698b74c578..6fb621b5e51 100644
--- a/spec/factories/ci/runners.rb
+++ b/spec/factories/ci/runners.rb
@@ -36,6 +36,14 @@ FactoryBot.define do
       end
     end
 
+    trait :without_projects do
+      # we use that to create invalid runner:
+      # the one without projects
+      after(:create) do |runner, evaluator|
+        runner.runner_projects.delete_all
+      end
+    end
+
     trait :inactive do
       active false
     end
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index cd60e67cc01..f8d590e4776 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -766,11 +766,7 @@ describe Ci::Runner do
   end
 
   describe 'project runner without projects is destroyable' do
-    subject { create(:ci_runner, :project) }
-
-    before do
-      subject.runner_projects.delete_all
-    end
+    subject { create(:ci_runner, :project, :without_projects) }
 
     it 'does not have projects' do
       expect(subject.runner_projects).to be_empty
diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb
index cc326ff6484..0c7937feed6 100644
--- a/spec/requests/api/runners_spec.rb
+++ b/spec/requests/api/runners_spec.rb
@@ -128,6 +128,18 @@ describe API::Runners do
       end
 
       context 'when runner is not shared' do
+        context 'when unused runner is present' do
+          let!(:unused_project_runner) { create(:ci_runner, :project, :without_projects) }
+
+          it 'deletes unused runner' do
+            expect do
+              delete api("/runners/#{unused_project_runner.id}", admin)
+
+              expect(response).to have_gitlab_http_status(204)
+            end.to change { Ci::Runner.specific.count }.by(-1)
+          end
+        end
+
         it "returns runner's details" do
           get api("/runners/#{project_runner.id}", admin)
 
@@ -561,6 +573,17 @@ describe API::Runners do
       end
 
       context 'user is admin' do
+        context 'when project runner is used' do
+          let!(:new_project_runner) { create(:ci_runner, :project) }
+
+          it 'enables any specific runner' do
+            expect do
+              post api("/projects/#{project.id}/runners", admin), runner_id: new_project_runner.id
+            end.to change { project.runners.count }.by(+1)
+            expect(response).to have_gitlab_http_status(201)
+          end
+        end
+
         it 'enables a shared runner' do
           expect do
             post api("/projects/#{project.id}/runners", admin), runner_id: shared_runner.id
@@ -578,6 +601,16 @@ describe API::Runners do
       end
     end
 
+    context 'user is not admin' do
+      let!(:new_project_runner) { create(:ci_runner, :project) }
+
+      it 'does not enable runner without access to' do
+        post api("/projects/#{project.id}/runners", user), runner_id: new_project_runner.id
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
     context 'authorized user without permissions' do
       it 'does not enable runner' do
         post api("/projects/#{project.id}/runners", user2)

From 5c6c184f70719c464690455abc02e777b3ba4b7b Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Wed, 30 May 2018 15:43:30 +0200
Subject: [PATCH 26/26] Fix spec/models/ci/runner_spec.rb:775 destroy runner
 with no projects

---
 spec/models/ci/runner_spec.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index f8d590e4776..0f072aa1719 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -773,6 +773,7 @@ describe Ci::Runner do
     end
 
     it 'can be destroyed' do
+      subject
       expect { subject.destroy }.to change { described_class.count }.by(-1)
     end
   end