Add instance column to services table

This is needed to support instance-level integrations alongside service
templates.

We want to make sure that we always have the correct state in the
database. This commit is adding the following validations:
- Validates presence of project_id if not instance
- Validates absence of project_id if instance
- Validates only one instance-level service per type
- Validates service is instance or template

app/models/service.rb

@@ -32,9 +32,12 @@ class Service < ApplicationRecord
   belongs_to :project, inverse_of: :services
   has_one :service_hook
 
-  validates :project_id, presence: true, unless: -> { template? }
+  validates :project_id, presence: true, unless: -> { template? || instance? }
+  validates :project_id, absence: true, if: -> { instance? }
   validates :type, presence: true
   validates :template, uniqueness: { scope: :type }, if: -> { template? }
+  validates :instance, uniqueness: { scope: :type }, if: -> { instance? }
+  validate :validate_is_instance_or_template
 
   scope :visible, -> { where.not(type: 'GitlabIssueTrackerService') }
   scope :issue_trackers, -> { where(category: 'issue_tracker') }
@@ -326,6 +329,10 @@ class Service < ApplicationRecord
 
   private
 
+  def validate_is_instance_or_template
+    errors.add(:template, 'The service should be a service template or instance-level integration') if template? && instance?
+  end
+
   def cache_project_has_external_issue_tracker
     if project && !project.destroyed?
       project.cache_has_external_issue_tracker

changelogs/unreleased/204801-add-instance-to-services.yml

+---
+title: Add instance column to services table
+merge_request: 25714
+author:
+type: other

db/migrate/20200310132654_add_instance_to_services.rb

+# frozen_string_literal: true
+
+class AddInstanceToServices < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:services, :instance, :boolean, default: false)
+  end
+
+  def down
+    remove_column(:services, :instance)
+  end
+end

db/migrate/20200310135823_add_index_to_service_unique_instance_per_type.rb

+# frozen_string_literal: true
+
+class AddIndexToServiceUniqueInstancePerType < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:services, [:type, :instance], unique: true, where: 'instance IS TRUE')
+  end
+
+  def down
+    remove_concurrent_index(:services, [:type, :instance])
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_03_09_195710) do
+ActiveRecord::Schema.define(version: 2020_03_10_135823) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -3939,8 +3939,10 @@ ActiveRecord::Schema.define(version: 2020_03_09_195710) do
     t.string "description", limit: 500
     t.boolean "comment_on_event_enabled", default: true, null: false
     t.boolean "template", default: false
+    t.boolean "instance", default: false, null: false
     t.index ["project_id"], name: "index_services_on_project_id"
     t.index ["template"], name: "index_services_on_template"
+    t.index ["type", "instance"], name: "index_services_on_type_and_instance", unique: true, where: "(instance IS TRUE)"
     t.index ["type", "template"], name: "index_services_on_type_and_template", unique: true, where: "(template IS TRUE)"
     t.index ["type"], name: "index_services_on_type"
   end

lib/gitlab/import_export/project/import_export.yml

@@ -248,6 +248,7 @@ excluded_attributes:
     - :token_encrypted
   services:
     - :template
+    - :instance
   error_tracking_setting:
     - :encrypted_token
     - :encrypted_token_iv

spec/factories/services.rb

@@ -4,6 +4,11 @@ FactoryBot.define do
   factory :service do
     project
     type { 'Service' }
+
+    trait :instance do
+      project { nil }
+      instance { true }
+    end
   end
 
   factory :custom_issue_tracker_service, class: 'CustomIssueTrackerService' do

spec/fixtures/lib/gitlab/import_export/light/project.json

@@ -111,6 +111,28 @@
       "active": false,
       "properties": {},
       "template": true,
+      "instance": false,
+      "push_events": true,
+      "issues_events": true,
+      "merge_requests_events": true,
+      "tag_push_events": true,
+      "note_events": true,
+      "job_events": true,
+      "type": "TeamcityService",
+      "category": "ci",
+      "default": false,
+      "wiki_page_events": true
+    },
+    {
+      "id": 101,
+      "title": "JetBrains TeamCity CI",
+      "project_id": 5,
+      "created_at": "2016-06-14T15:01:51.315Z",
+      "updated_at": "2016-06-14T15:01:51.315Z",
+      "active": false,
+      "properties": {},
+      "template": false,
+      "instance": true,
       "push_events": true,
       "issues_events": true,
       "merge_requests_events": true,

spec/lib/gitlab/import_export/project/tree_restorer_spec.rb

@@ -703,6 +703,12 @@ describe Gitlab::ImportExport::Project::TreeRestorer do
         expect(project.services.where(template: true).count).to eq(0)
       end
 
+      it 'does not import any instance services' do
+        expect(restored_project_json).to eq(true)
+
+        expect(project.services.where(instance: true).count).to eq(0)
+      end
+
       it 'imports labels' do
         create(:group_label, name: 'Another label', group: project.group)
 

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -459,6 +459,7 @@ Service:
 - active
 - properties
 - template
+- instance
 - push_events
 - issues_events
 - commit_events

spec/models/service_spec.rb

@@ -18,6 +18,20 @@ describe Service do
       expect(build(:service, project_id: nil, template: false)).to be_invalid
     end
 
+    it 'validates presence of project_id if not instance', :aggregate_failures do
+      expect(build(:service, project_id: nil, instance: true)).to be_valid
+      expect(build(:service, project_id: nil, instance: false)).to be_invalid
+    end
+
+    it 'validates absence of project_id if instance', :aggregate_failures do
+      expect(build(:service, project_id: nil, instance: true)).to be_valid
+      expect(build(:service, instance: true)).to be_invalid
+    end
+
+    it 'validates service is template or instance' do
+      expect(build(:service, project_id: nil, template: true, instance: true)).to be_invalid
+    end
+
     context 'with an existing service template' do
       before do
         create(:service, type: 'Service', template: true)
@@ -27,6 +41,16 @@ describe Service do
         expect(build(:service, type: 'Service', template: true)).to be_invalid
       end
     end
+
+    context 'with an existing instance service' do
+      before do
+        create(:service, :instance)
+      end
+
+      it 'validates only one service instance per type' do
+        expect(build(:service, :instance)).to be_invalid
+      end
+    end
   end
 
   describe 'Scopes' do