Merge branch '46010-add-more-validations-for-runners-and-runner-type' into 'master'

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'

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'

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'
@@ -56,10 +56,15 @@ module Ci
     end
 
     validate :tag_constraints
-    validate :either_projects_or_group
     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 :validate_is_shared
+
     acts_as_taggable
 
     after_destroy :cleanup_runner_queue
@@ -115,8 +120,15 @@ 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)
+      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
@@ -253,13 +265,33 @@ module Ci
       self.class.owned_or_shared(project_id).where(id: self.id).any?
     end
 
-    def either_projects_or_group
-      if groups.many?
-        errors.add(:runner, 'can only be assigned to one group')
+    def no_projects
+      if projects.any?
+        errors.add(:runner, 'cannot have projects assigned')
+      end
+    end
+
+    def no_groups
+      if groups.any?
+        errors.add(:runner, 'cannot have groups assigned')
+      end
+    end
+
+    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
 
-      if assigned_to_group? && assigned_to_project?
-        errors.add(:runner, 'can only be assigned either to projects or to a group')
+    def validate_is_shared
+      unless is_shared? == instance_type?
+        errors.add(:is_shared, 'is not equal to instance_type?')
       end
     end
 

app/models/ci/runner_namespace.rb

@@ -2,8 +2,10 @@ module Ci
   class RunnerNamespace < ActiveRecord::Base
     extend Gitlab::Ci::Model
 
-    belongs_to :runner
-    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
+
+    validates :runner_id, uniqueness: { scope: :namespace_id }
   end
 end

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

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
 

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

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'

app/services/ci/register_job_service.rb

@@ -89,7 +89,10 @@ 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

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
 

lib/api/runners.rb

@@ -133,12 +133,10 @@ 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")
+          render_validation_error!(runner)
         end
       end
 

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, 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

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

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)

spec/factories/ci/runner_projects.rb

 FactoryBot.define do
   factory :ci_runner_project, class: Ci::RunnerProject do
-    runner factory: :ci_runner
+    runner factory: [:ci_runner, :project]
     project
   end
 end

spec/factories/ci/runners.rb

@@ -3,22 +3,45 @@ FactoryBot.define do
     sequence(:description) { |n| "My runner#{n}" }
 
     platform  "darwin"
-    is_shared false
     active    true
     access_level :not_protected
-    runner_type :project_type
+
+    is_shared true
+    runner_type :instance_type
 
     trait :online do
       contacted_at Time.now
     end
 
-    trait :shared do
+    trait :instance do
       is_shared true
       runner_type :instance_type
     end
 
-    trait :specific 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 :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

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
@@ -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
 

spec/features/runners_spec.rb

@@ -28,12 +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) }
-
-      background do
-        project.runners << specific_runner
-      end
+    context 'when a project_type runner is activated on the project' do
+      given!(:specific_runner) { create(:ci_runner, :project, projects: [project]) }
 
       scenario 'user sees the specific runner' do
         visit project_runners_path(project)
@@ -114,7 +110,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,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, :specific) }
+      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
@@ -220,8 +215,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 +229,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 +258,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 +272,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)

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 }
 

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

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

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
@@ -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 }
 

spec/models/user_spec.rb

@@ -1858,13 +1858,10 @@ 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!(: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
@@ -1872,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
 
@@ -1914,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
 
@@ -1931,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)
@@ -1941,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)
@@ -1955,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)
@@ -1968,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)

spec/requests/api/runner_spec.rb

@@ -111,11 +111,13 @@ 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
 
-            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
@@ -262,16 +264,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 +377,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
@@ -724,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
@@ -735,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

spec/requests/api/runners_spec.rb

@@ -11,23 +11,10 @@ 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!(: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!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group], runner_type: :group_type) }
+  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!(:group_runner) { create(:ci_runner, :group, description: 'Group runner', groups: [group]) }
 
   before do
     # Set project access for users
@@ -141,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)
 
@@ -310,14 +309,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)
@@ -543,11 +534,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
@@ -560,7 +547,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
@@ -586,11 +573,15 @@ 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)
+        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
@@ -603,14 +594,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)
 
@@ -618,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)

spec/serializers/runner_entity_spec.rb

 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

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!(:group_runner) { create(:ci_runner, groups: [group], runner_type: :group_type) }
+    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
@@ -181,24 +177,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
@@ -292,7 +288,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) }
@@ -324,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) }

spec/services/ci/update_build_queue_service_spec.rb

@@ -6,19 +6,18 @@ 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, 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
     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
@@ -26,7 +25,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 +55,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