Merge branch 'optimise-pages-service-calling' into 'master'

Optimise pages service calling

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

app/models/ci/build.rb

@@ -144,6 +144,7 @@ module Ci
       after_transition any => [:success] do |build|
         build.run_after_commit do
           BuildSuccessWorker.perform_async(id)
+          PagesWorker.perform_async(:deploy, id) if build.pages_generator?
         end
       end
@@ -183,6 +184,11 @@ module Ci
       pipeline.manual_actions.where.not(name: name)
     end
+    def pages_generator?
+      Gitlab.config.pages.enabled &&
+        self.name == 'pages'
+    end
+
     def playable?
       action? && (manual? || retryable?)
     end
@@ -402,8 +408,6 @@ module Ci
       build_data = Gitlab::DataBuilder::Build.build(self)
       project.execute_hooks(build_data.dup, :job_hooks)
       project.execute_services(build_data.dup, :job_hooks)
-      PagesService.new(build_data).execute
-      project.running_or_pending_build_count(force: true)
     end
     def browsable_artifacts?

app/models/project.rb

@@ -1649,12 +1649,6 @@ class Project < ActiveRecord::Base
     import_state.update_column(:jid, nil)
   end
-  def running_or_pending_build_count(force: false)
-    Rails.cache.fetch(['projects', id, 'running_or_pending_build_count'], force: force) do
-      builds.running_or_pending.count(:all)
-    end
-  end
-
   # Lazy loading of the `pipeline_status` attribute
   def pipeline_status
     @pipeline_status ||= Gitlab::Cache::Ci::ProjectPipelineStatus.load_for_project(self)

app/services/pages_service.rb

-class PagesService
-  attr_reader :data
-
-  def initialize(data)
-    @data = data
-  end
-
-  def execute
-    return unless Settings.pages.enabled
-    return unless data[:build_name] == 'pages'
-    return unless data[:build_status] == 'success'
-
-    PagesWorker.perform_async(:deploy, data[:build_id])
-  end
-end

changelogs/unreleased/optimise-pages-service-calling.yml

+---
+title: Optimise PagesWorker usage
+merge_request:
+author:
+type: performance

changelogs/unreleased/remove-unused-query-in-hooks.yml

+---
+title: Remove unused running_or_pending_build_count
+merge_request:
+author:
+type: performance

spec/features/projects/jobs/user_browses_job_spec.rb

@@ -32,8 +32,6 @@ describe 'User browses a job', :js do
     page.within('.erased') do
       expect(page).to have_content('Job has been erased')
     end
-
-    expect(build.project.running_or_pending_build_count).to eq(build.project.builds.running_or_pending.count(:all))
   end
   context 'with a failed job' do

spec/models/ci/build_spec.rb

@@ -2506,4 +2506,76 @@ describe Ci::Build do
       end
     end
   end
+
+  describe 'pages deployments' do
+    set(:build) { create(:ci_build, project: project, user: user) }
+
+    context 'when job is "pages"' do
+      before do
+        build.name = 'pages'
+      end
+
+      context 'when pages are enabled' do
+        before do
+          allow(Gitlab.config.pages).to receive_messages(enabled: true)
+        end
+
+        it 'is marked as pages generator' do
+          expect(build).to be_pages_generator
+        end
+
+        context 'job succeeds' do
+          it "calls pages worker" do
+            expect(PagesWorker).to receive(:perform_async).with(:deploy, build.id)
+
+            build.success!
+          end
+        end
+
+        context 'job fails' do
+          it "does not call pages worker" do
+            expect(PagesWorker).not_to receive(:perform_async)
+
+            build.drop!
+          end
+        end
+      end
+
+      context 'when pages are disabled' do
+        before do
+          allow(Gitlab.config.pages).to receive_messages(enabled: false)
+        end
+  
+        it 'is not marked as pages generator' do
+          expect(build).not_to be_pages_generator
+        end
+
+        context 'job succeeds' do
+          it "does not call pages worker" do
+            expect(PagesWorker).not_to receive(:perform_async)
+
+            build.success!
+          end
+        end
+      end
+    end
+
+    context 'when job is not "pages"' do
+      before do
+        build.name = 'other-job'
+      end
+
+      it 'is not marked as pages generator' do
+        expect(build).not_to be_pages_generator
+      end
+
+      context 'job succeeds' do
+        it "does not call pages worker" do
+          expect(PagesWorker).not_to receive(:perform_async)
+
+          build.success
+        end
+      end
+    end
+  end
 end

spec/services/pages_service_spec.rb

-require 'spec_helper'
-
-describe PagesService do
-  let(:build) { create(:ci_build) }
-  let(:data) { Gitlab::DataBuilder::Build.build(build) }
-  let(:service) { described_class.new(data) }
-
-  before do
-    allow(Gitlab.config.pages).to receive(:enabled).and_return(true)
-  end
-
-  context 'execute asynchronously for pages job' do
-    before do
-      build.name = 'pages'
-    end
-
-    context 'on success' do
-      before do
-        build.success
-      end
-
-      it 'executes worker' do
-        expect(PagesWorker).to receive(:perform_async)
-        service.execute
-      end
-    end
-
-    %w(pending running failed canceled).each do |status|
-      context "on #{status}" do
-        before do
-          build.status = status
-        end
-
-        it 'does not execute worker' do
-          expect(PagesWorker).not_to receive(:perform_async)
-          service.execute
-        end
-      end
-    end
-  end
-
-  context 'for other jobs' do
-    before do
-      build.name = 'other job'
-      build.success
-    end
-
-    it 'does not execute worker' do
-      expect(PagesWorker).not_to receive(:perform_async)
-      service.execute
-    end
-  end
-end