From b009a0084c67877ba6a808c4c8a81c568598d624 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Mon, 4 Jun 2018 21:49:39 +0200
Subject: [PATCH] Remove PagesService and instead make it explicit that we call
 PagesWorker

---
 app/models/ci/build.rb                        |  7 +-
 app/services/pages_service.rb                 | 15 ----
 .../optimise-pages-service-calling.yml        |  5 ++
 spec/models/ci/build_spec.rb                  | 72 +++++++++++++++++++
 spec/services/pages_service_spec.rb           | 53 --------------
 5 files changed, 83 insertions(+), 69 deletions(-)
 delete mode 100644 app/services/pages_service.rb
 create mode 100644 changelogs/unreleased/optimise-pages-service-calling.yml
 delete mode 100644 spec/services/pages_service_spec.rb

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 7cf4dda178a..746464d0e21 100644
--- a/app/models/ci/build.rb
+++ b/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,7 +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
     end
 
     def browsable_artifacts?
diff --git a/app/services/pages_service.rb b/app/services/pages_service.rb
deleted file mode 100644
index 446eeb34d3b..00000000000
--- a/app/services/pages_service.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-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
diff --git a/changelogs/unreleased/optimise-pages-service-calling.yml b/changelogs/unreleased/optimise-pages-service-calling.yml
new file mode 100644
index 00000000000..e017e6b01f1
--- /dev/null
+++ b/changelogs/unreleased/optimise-pages-service-calling.yml
@@ -0,0 +1,5 @@
+---
+title: Optimise PagesWorker usage
+merge_request:
+author:
+type: performance
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 66c9708b4cf..e5982a44e89 100644
--- a/spec/models/ci/build_spec.rb
+++ b/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
diff --git a/spec/services/pages_service_spec.rb b/spec/services/pages_service_spec.rb
deleted file mode 100644
index f8db6900a0a..00000000000
--- a/spec/services/pages_service_spec.rb
+++ /dev/null
@@ -1,53 +0,0 @@
-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