Introduce a new index for Ci::Pipeline39923-automatically-disable-auto-devops-for-project

Project_id+config_source+status index was introduced to 'ci_pipeline' in
order to make auto devops queries more performant.

Another minor backend comments were addressed to.

6 files changed, 32 insertions, 33 deletions

diff --git a/app/services/projects/auto_devops/disable_service.rb b/app/services/projects/auto_devops/disable_service.rb
index e243d4f5979..0c9dd8aaf0e 100644
--- a/app/services/projects/auto_devops/disable_service.rb
+++ b/app/services/projects/auto_devops/disable_service.rb
@@ -22,19 +22,8 @@ module Projects
       end
 
       def disable_auto_devops
-        if project.auto_devops.present?
-          project.auto_devops.update_attribute(:enabled, false)
-        else
-          create_disabled_auto_devops
-        end
-      end
-
-      def create_disabled_auto_devops
-        ProjectAutoDevops.new.tap do |auto_devops|
-          auto_devops.project = project
-          auto_devops.enabled = false
-          auto_devops.save!
-        end
+        project.auto_devops_attributes = { enabled: false }
+        project.save!
       end
 
       def auto_devops_pipelines
diff --git a/db/migrate/20180901171833_add_project_config_source_status_index_to_pipeline.rb b/db/migrate/20180901171833_add_project_config_source_status_index_to_pipeline.rb
new file mode 100644
index 00000000000..9d24071aea9
--- /dev/null
+++ b/db/migrate/20180901171833_add_project_config_source_status_index_to_pipeline.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class AddProjectConfigSourceStatusIndexToPipeline < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :ci_pipelines, [:project_id, :config_source, :status]
+  end
+
+  def down
+    remove_concurrent_index :ci_pipelines, [:project_id, :config_source, :status]
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 02e545bec7d..1d5c20c24e6 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180826111825) do
+ActiveRecord::Schema.define(version: 20180901171833) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -471,6 +471,7 @@ ActiveRecord::Schema.define(version: 20180826111825) do
 
   add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree
   add_index "ci_pipelines", ["pipeline_schedule_id"], name: "index_ci_pipelines_on_pipeline_schedule_id", using: :btree
+  add_index "ci_pipelines", ["project_id", "config_source", "status"], name: "index_ci_pipelines_on_project_id_and_config_source_and_status", using: :btree
   add_index "ci_pipelines", ["project_id", "iid"], name: "index_ci_pipelines_on_project_id_and_iid", unique: true, where: "(iid IS NOT NULL)", using: :btree
   add_index "ci_pipelines", ["project_id", "ref", "status", "id"], name: "index_ci_pipelines_on_project_id_and_ref_and_status_and_id", using: :btree
   add_index "ci_pipelines", ["project_id", "sha"], name: "index_ci_pipelines_on_project_id_and_sha", using: :btree
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 081c92b5ba3..68a361fa882 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -1969,7 +1969,7 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'Auto DevOps' do
+  context 'Auto DevOps notifications' do
     describe '#autodevops_disabled' do
       let(:owner) { create(:user) }
       let(:namespace) { create(:namespace, owner: owner) }
diff --git a/spec/services/projects/auto_devops/disable_service_spec.rb b/spec/services/projects/auto_devops/disable_service_spec.rb
index 2764f55cb67..3992b4d3afa 100644
--- a/spec/services/projects/auto_devops/disable_service_spec.rb
+++ b/spec/services/projects/auto_devops/disable_service_spec.rb
@@ -22,7 +22,7 @@ describe Projects::AutoDevops::DisableService, '#execute' do
 
     context 'when Auto DevOps explicitly enabled on project' do
       before do
-        auto_devops.update_attribute(:enabled, true)
+        auto_devops.update!(enabled: true)
       end
 
       it { is_expected.to be_falsy }
@@ -30,7 +30,7 @@ describe Projects::AutoDevops::DisableService, '#execute' do
 
     context 'when Auto DevOps explicitly disabled on project' do
       before do
-        auto_devops.update_attribute(:enabled, false)
+        auto_devops.update!(enabled: false)
       end
 
       it { is_expected.to be_falsy }
@@ -38,7 +38,7 @@ describe Projects::AutoDevops::DisableService, '#execute' do
 
     context 'when Auto DevOps is implicitly enabled' do
       before do
-        auto_devops.update_attribute(:enabled, nil)
+        auto_devops.update!(enabled: nil)
       end
 
       context 'when is the first pipeline failure' do
@@ -46,8 +46,6 @@ describe Projects::AutoDevops::DisableService, '#execute' do
           create(:ci_pipeline, :failed, :auto_devops_source, project: project)
         end
 
-        it { is_expected.to be_truthy }
-
         it 'should disable Auto Devops for project' do
           subject
 
@@ -55,17 +53,15 @@ describe Projects::AutoDevops::DisableService, '#execute' do
         end
       end
 
-      context 'when there is not the first pipeline failure' do
+      context 'when it is not the first pipeline failure' do
         before do
-          2.times { create(:ci_pipeline, :failed, :auto_devops_source, project: project) }
+          create_list(:ci_pipeline, 2, :failed, :auto_devops_source, project: project)
         end
 
-        it { is_expected.to be_falsy }
-
         it 'should not disable Auto DevOps for project' do
           subject
 
-          expect(auto_devops.enabled).to be_nil
+          expect(auto_devops.reload.enabled).to be_nil
         end
       end
 
@@ -74,12 +70,10 @@ describe Projects::AutoDevops::DisableService, '#execute' do
           create(:ci_pipeline, :success, :auto_devops_source, project: project)
         end
 
-        it { is_expected.to be_falsy }
-
         it 'should not disable Auto DevOps for project' do
           subject
 
-          expect(auto_devops.enabled).to be_nil
+          expect(auto_devops.reload.enabled).to be_nil
         end
       end
     end
@@ -91,12 +85,11 @@ describe Projects::AutoDevops::DisableService, '#execute' do
         create(:ci_pipeline, :failed, :auto_devops_source, project: project)
       end
 
-      it { is_expected.to be_truthy }
-
       it 'should disable Auto Devops for project' do
         subject
+        auto_devops = project.reload.auto_devops
 
-        expect(project.reload.auto_devops.enabled).to eq(false)
+        expect(auto_devops.enabled).to eq(false)
       end
 
       it 'should create a ProjectAutoDevops record' do
diff --git a/spec/workers/auto_devops/disable_worker_spec.rb b/spec/workers/auto_devops/disable_worker_spec.rb
index d41992aabab..800a07e41a5 100644
--- a/spec/workers/auto_devops/disable_worker_spec.rb
+++ b/spec/workers/auto_devops/disable_worker_spec.rb
@@ -17,8 +17,7 @@ describe AutoDevops::DisableWorker, '#perform' do
   it 'disables auto devops for project' do
     subject.perform(pipeline.id)
 
-    expect(auto_devops.reload.enabled).not_to be_nil
-    expect(auto_devops.reload.enabled?).to be_falsy
+    expect(auto_devops.reload.enabled).to eq(false)
   end
 
   context 'when project owner is a user' do