Merge branch 'remove-is-shared-from-ci-runners' into 'master'

Remove the use of `is_shared` of `Ci::Runner`

See merge request gitlab-org/gitlab-ce!20172

app/controllers/projects/settings/ci_cd_controller.rb

@@ -74,7 +74,7 @@ module Projects
           .ordered
           .page(params[:page]).per(20)
 
-        @shared_runners = ::Ci::Runner.shared.active
+        @shared_runners = ::Ci::Runner.instance_type.active
 
         @shared_runners_count = @shared_runners.count(:all)
 

app/helpers/ci_status_helper.rb

@@ -122,7 +122,7 @@ module CiStatusHelper
 
   def no_runners_for_project?(project)
     project.runners.blank? &&
-      Ci::Runner.shared.blank?
+      Ci::Runner.instance_type.blank?
   end
 
   def render_status_with_link(type, status, path = nil, tooltip_placement: 'left', cssclass: '', container: 'body')

app/models/ci/runner.rb

@@ -2,6 +2,7 @@ module Ci
   class Runner < ActiveRecord::Base
     extend Gitlab::Ci::Model
     include Gitlab::SQL::Pattern
+    include IgnorableColumn
     include RedisCacheable
     include ChronicDurationAttribute
 
@@ -11,6 +12,8 @@ module Ci
     AVAILABLE_SCOPES = %w[specific shared active paused online].freeze
     FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze
 
+    ignore_column :is_shared
+
     has_many :builds
     has_many :runner_projects, inverse_of: :runner, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
     has_many :projects, through: :runner_projects
@@ -21,13 +24,16 @@ module Ci
 
     before_validation :set_default_values
 
-    scope :specific, -> { where(is_shared: false) }
-    scope :shared, -> { where(is_shared: true) }
     scope :active, -> { where(active: true) }
     scope :paused, -> { where(active: false) }
     scope :online, -> { where('contacted_at > ?', contact_time_deadline) }
     scope :ordered, -> { order(id: :desc) }
 
+    # BACKWARD COMPATIBILITY: There are needed to maintain compatibility with `AVAILABLE_SCOPES` used by `lib/api/runners.rb`
+    scope :deprecated_shared, -> { instance_type }
+    # this should get replaced with `project_type.or(group_type)` once using Rails5
+    scope :deprecated_specific, -> { where(runner_type: [runner_types[:project_type], runner_types[:group_type]]) }
+
     scope :belonging_to_project, -> (project_id) {
       joins(:runner_projects).where(ci_runner_projects: { project_id: project_id })
     }
@@ -39,9 +45,9 @@ module Ci
       joins(:groups).where(namespaces: { id: hierarchy_groups })
     }
 
-    scope :owned_or_shared, -> (project_id) do
+    scope :owned_or_instance_wide, -> (project_id) do
       union = Gitlab::SQL::Union.new(
-        [belonging_to_project(project_id), belonging_to_parent_group_of_project(project_id), shared],
+        [belonging_to_project(project_id), belonging_to_parent_group_of_project(project_id), instance_type],
         remove_duplicates: false
       )
       from("(#{union.to_sql}) ci_runners")
@@ -63,7 +69,6 @@ module Ci
     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
 
@@ -113,8 +118,7 @@ module Ci
     end
 
     def assign_to(project, current_user = nil)
-      if shared?
-        self.is_shared = false if shared?
+      if instance_type?
         self.runner_type = :project_type
       elsif group_type?
         raise ArgumentError, 'Transitioning a group runner to a project runner is not supported'
@@ -137,10 +141,6 @@ module Ci
       description
     end
 
-    def shared?
-      is_shared
-    end
-
     def online?
       contacted_at && contacted_at > self.class.contact_time_deadline
     end
@@ -159,10 +159,6 @@ module Ci
       runner_projects.count == 1
     end
 
-    def specific?
-      !shared?
-    end
-
     def assigned_to_group?
       runner_namespaces.any?
     end
@@ -260,7 +256,7 @@ module Ci
     end
 
     def assignable_for?(project_id)
-      self.class.owned_or_shared(project_id).where(id: self.id).any?
+      self.class.owned_or_instance_wide(project_id).where(id: self.id).any?
     end
 
     def no_projects
@@ -287,12 +283,6 @@ module Ci
       end
     end
 
-    def validate_is_shared
-      unless is_shared? == instance_type?
-        errors.add(:is_shared, 'is not equal to instance_type?')
-      end
-    end
-
     def accepting_tags?(build)
       (run_untagged? || build.has_tags?) && (build.tag_list - tag_list).empty?
     end

app/models/project.rb

@@ -1422,7 +1422,7 @@ class Project < ActiveRecord::Base
   end
 
   def shared_runners
-    @shared_runners ||= shared_runners_available? ? Ci::Runner.shared : Ci::Runner.none
+    @shared_runners ||= shared_runners_available? ? Ci::Runner.instance_type : Ci::Runner.none
   end
 
   def group_runners

app/models/user.rb

@@ -1032,7 +1032,7 @@ class User < ActiveRecord::Base
 
       union = Gitlab::SQL::Union.new([project_runner_ids, group_runner_ids])
 
-      Ci::Runner.specific.where("ci_runners.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
+      Ci::Runner.where("ci_runners.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
     end
   end
 

app/serializers/runner_entity.rb

@@ -4,7 +4,7 @@ class RunnerEntity < Grape::Entity
   expose :id, :description
 
   expose :edit_path,
-    if: -> (*) { can?(request.current_user, :admin_build, project) && runner.specific? } do |runner|
+    if: -> (*) { can?(request.current_user, :admin_build, project) && runner.project_type? } do |runner|
     edit_project_runner_path(project, runner)
   end
 

app/services/ci/register_job_service.rb

@@ -15,7 +15,7 @@ module Ci
 
     def execute
       builds =
-        if runner.shared?
+        if runner.instance_type?
           builds_for_shared_runner
         elsif runner.group_type?
           builds_for_group_runner
@@ -99,7 +99,7 @@ module Ci
     end
 
     def running_builds_for_shared_runners
-      Ci::Build.running.where(runner: Ci::Runner.shared)
+      Ci::Build.running.where(runner: Ci::Runner.instance_type)
         .group(:project_id).select(:project_id, 'count(*) AS running_builds')
     end
 
@@ -115,7 +115,7 @@ module Ci
     end
 
     def register_success(job)
-      labels = { shared_runner: runner.shared?,
+      labels = { shared_runner: runner.instance_type?,
                  jobs_running_for_project: jobs_running_for_project(job) }
 
       job_queue_duration_seconds.observe(labels, Time.now - job.queued_at) unless job.queued_at.nil?
@@ -123,10 +123,10 @@ module Ci
     end
 
     def jobs_running_for_project(job)
-      return '+Inf' unless runner.shared?
+      return '+Inf' unless runner.instance_type?
 
       # excluding currently started job
-      running_jobs_count = job.project.builds.running.where(runner: Ci::Runner.shared)
+      running_jobs_count = job.project.builds.running.where(runner: Ci::Runner.instance_type)
                               .limit(JOBS_RUNNING_FOR_PROJECT_MAX_BUCKET + 1).count - 1
       running_jobs_count < JOBS_RUNNING_FOR_PROJECT_MAX_BUCKET ? running_jobs_count : "#{JOBS_RUNNING_FOR_PROJECT_MAX_BUCKET}+"
     end

app/views/admin/runners/_runner.html.haml

@@ -1,6 +1,6 @@
 %tr{ id: dom_id(runner) }
   %td
-    - if runner.shared?
+    - if runner.instance_type?
       %span.badge.badge-success shared
     - elsif runner.group_type?
       %span.badge.badge-success group
@@ -21,7 +21,7 @@
   %td
     = runner.ip_address
   %td
-    - if runner.shared? || runner.group_type?
+    - if runner.instance_type? || runner.group_type?
       n/a
     - else
       = runner.projects.count(:all)

app/views/admin/runners/show.html.haml

@@ -2,7 +2,7 @@
   %h3.project-title
     Runner ##{@runner.id}
     .float-right
-      - if @runner.shared?
+      - if @runner.instance_type?
         %span.runner-state.runner-state-shared
           Shared
       - else
@@ -13,7 +13,7 @@
 - breadcrumb_title "##{@runner.id}"
 - @no_container = true
 
-- if @runner.shared?
+- if @runner.instance_type?
   .bs-callout.bs-callout-success
     %h4 This Runner will process jobs from ALL UNASSIGNED projects
     %p

app/views/projects/runners/_runner.html.haml

@@ -26,7 +26,7 @@
         - else
           - runner_project = @project.runner_projects.find_by(runner_id: runner)
           = link_to _('Disable for this project'), project_runner_project_path(@project, runner_project), data: { confirm: _("Are you sure?") }, method: :delete, class: 'btn btn-danger btn-sm'
-      - elsif !(runner.is_shared? || runner.group_type?) # We can simplify this to `runner.project_type?` when migrating #runner_type is complete
+      - elsif runner.project_type?
         = form_for [@project.namespace.becomes(Namespace), @project, @project.runner_projects.new] do |f|
           = f.hidden_field :runner_id, value: runner.id
           = f.submit _('Enable for this project'), class: 'btn btn-sm'

app/views/shared/runners/show.html.haml

@@ -3,7 +3,7 @@
 %h3.page-title
   Runner ##{@runner.id}
   .float-right
-    - if @runner.shared?
+    - if @runner.instance_type?
       %span.runner-state.runner-state-shared
         Shared
     - elsif @runner.group_type?

changelogs/unreleased/remove-is-shared-from-ci-runners.yml

@@ -0,0 +1,5 @@
+---
+title: Remove the use of `is_shared` of `Ci::Runner`
+merge_request:
+author:
+type: other

lib/api/entities.rb

@@ -1010,7 +1010,7 @@ module API
       expose :description
       expose :ip_address
       expose :active
-      expose :is_shared
+      expose :instance_type?, as: :is_shared
       expose :name
       expose :online?, as: :online
       expose :status
@@ -1024,7 +1024,7 @@ module API
       expose :access_level
       expose :version, :revision, :platform, :architecture
       expose :contacted_at
-      expose :token, if: lambda { |runner, options| options[:current_user].admin? || !runner.is_shared? }
+      expose :token, if: lambda { |runner, options| options[:current_user].admin? || !runner.instance_type? }
       expose :projects, with: Entities::BasicProjectDetails do |runner, options|
         if options[:current_user].admin?
           runner.projects

lib/api/runner.rb

@@ -24,13 +24,13 @@ module API
         attributes =
           if runner_registration_token_valid?
             # Create shared runner. Requires admin access
-            attributes.merge(is_shared: true, runner_type: :instance_type)
+            attributes.merge(runner_type: :instance_type)
           elsif project = Project.find_by(runners_token: params[:token])
             # Create a specific runner for the project
-            attributes.merge(is_shared: false, runner_type: :project_type, projects: [project])
+            attributes.merge(runner_type: :project_type, projects: [project])
           elsif group = Group.find_by(runners_token: params[:token])
             # Create a specific runner for the group
-            attributes.merge(is_shared: false, runner_type: :group_type, groups: [group])
+            attributes.merge(runner_type: :group_type, groups: [group])
           else
             forbidden!
           end

lib/api/runners.rb

@@ -119,7 +119,7 @@ module API
         use :pagination
       end
       get ':id/runners' do
-        runners = filter_runners(Ci::Runner.owned_or_shared(user_project.id), params[:scope])
+        runners = filter_runners(Ci::Runner.owned_or_instance_wide(user_project.id), params[:scope])
         present paginate(runners), with: Entities::Runner
       end
 
@@ -170,6 +170,11 @@ module API
           render_api_error!('Scope contains invalid value', 400)
         end
 
+        # Support deprecated scopes
+        if runners.respond_to?("deprecated_#{scope}")
+          scope = "deprecated_#{scope}"
+        end
+
         runners.public_send(scope) # rubocop:disable GitlabSecurity/PublicSend
       end
 
@@ -180,7 +185,7 @@ module API
       end
 
       def authenticate_show_runner!(runner)
-        return if runner.is_shared || current_user.admin?
+        return if runner.instance_type? || current_user.admin?
 
         forbidden!("No access granted") unless can?(current_user, :read_runner, runner)
       end

lib/gitlab/data_builder/pipeline.rb

@@ -55,7 +55,7 @@ module Gitlab
           id: runner.id,
           description: runner.description,
           active: runner.active?,
-          is_shared: runner.is_shared?
+          is_shared: runner.instance_type?
         }
       end
     end

spec/factories/ci/runners.rb

@@ -6,7 +6,6 @@ FactoryBot.define do
     active    true
     access_level :not_protected
 
-    is_shared true
     runner_type :instance_type
 
     trait :online do
@@ -14,12 +13,10 @@ FactoryBot.define do
     end
 
     trait :instance do
-      is_shared true
       runner_type :instance_type
     end
 
     trait :group do
-      is_shared false
       runner_type :group_type
 
       after(:build) do |runner, evaluator|
@@ -28,7 +25,6 @@ FactoryBot.define do
     end
 
     trait :project do
-      is_shared false
       runner_type :project_type
 
       after(:build) do |runner, evaluator|

spec/models/ci/runner_spec.rb

@@ -105,7 +105,7 @@ describe Ci::Runner do
     end
   end
 
-  describe '.shared' do
+  describe '.instance_type' do
     let(:group) { create(:group) }
     let(:project) { create(:project) }
     let!(:group_runner) { create(:ci_runner, :group, groups: [group]) }
@@ -113,7 +113,7 @@ describe Ci::Runner do
     let!(:shared_runner) { create(:ci_runner, :instance) }
 
     it 'returns only shared runners' do
-      expect(described_class.shared).to contain_exactly(shared_runner)
+      expect(described_class.instance_type).to contain_exactly(shared_runner)
     end
   end
 
@@ -155,7 +155,7 @@ describe Ci::Runner do
     end
   end
 
-  describe '.owned_or_shared' do
+  describe '.owned_or_instance_wide' do
     it 'returns a globally shared, a project specific and a group specific runner' do
       # group specific
       group = create(:group)
@@ -168,7 +168,7 @@ describe Ci::Runner do
       # globally shared
       shared_runner = create(:ci_runner, :instance)
 
-      expect(described_class.owned_or_shared(project.id)).to contain_exactly(
+      expect(described_class.owned_or_instance_wide(project.id)).to contain_exactly(
         group_runner, project_runner, shared_runner
       )
     end
@@ -202,7 +202,6 @@ describe Ci::Runner do
       it 'transitions shared runner to project runner and assigns project' do
         expect(subject).to be_truthy
 
-        expect(runner).to be_specific
         expect(runner).to be_project_type
         expect(runner.projects).to eq([project])
         expect(runner.only_for?(project)).to be_truthy

spec/requests/api/runners_spec.rb

@@ -89,6 +89,17 @@ describe API::Runners do
         end
       end
 
+      it 'filters runners by scope' do
+        get api('/runners/all?scope=shared', admin)
+
+        shared = json_response.all? { |r| r['is_shared'] }
+        expect(response).to have_gitlab_http_status(200)
+        expect(response).to include_pagination_headers
+        expect(json_response).to be_an Array
+        expect(json_response[0]).to have_key('ip_address')
+        expect(shared).to be_truthy
+      end
+
       it 'filters runners by scope' do
         get api('/runners/all?scope=specific', admin)
 
@@ -136,7 +147,7 @@ describe API::Runners 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.to change { Ci::Runner.project_type.count }.by(-1)
           end
         end
 
@@ -300,7 +311,7 @@ describe API::Runners do
             delete api("/runners/#{shared_runner.id}", admin)
 
             expect(response).to have_gitlab_http_status(204)
-          end.to change { Ci::Runner.shared.count }.by(-1)
+          end.to change { Ci::Runner.instance_type.count }.by(-1)
         end
 
         it_behaves_like '412 response' do
@@ -314,7 +325,7 @@ describe API::Runners do
             delete api("/runners/#{project_runner.id}", admin)
 
             expect(response).to have_http_status(204)
-          end.to change { Ci::Runner.specific.count }.by(-1)
+          end.to change { Ci::Runner.project_type.count }.by(-1)
         end
       end
 
@@ -349,7 +360,7 @@ describe API::Runners do
             delete api("/runners/#{project_runner.id}", user)
 
             expect(response).to have_http_status(204)
-          end.to change { Ci::Runner.specific.count }.by(-1)
+          end.to change { Ci::Runner.project_type.count }.by(-1)
         end
 
         it_behaves_like '412 response' do
@@ -584,12 +595,12 @@ describe API::Runners do
           end
         end
 
-        it 'enables a shared runner' do
+        it 'enables a instance type runner' do
           expect do
             post api("/projects/#{project.id}/runners", admin), runner_id: shared_runner.id
           end.to change { project.runners.count }.by(1)
 
-          expect(shared_runner.reload).not_to be_shared
+          expect(shared_runner.reload).not_to be_instance_type
           expect(response).to have_gitlab_http_status(201)
         end
       end