Merge branch 'update-pages-config-only-when-changed' into 'master'

Update pages config only when changed

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

3 files changed, 57 insertions, 14 deletions

diff --git a/app/services/projects/update_pages_configuration_service.rb b/app/services/projects/update_pages_configuration_service.rb
index abf40b3ad7a..674071ad92a 100644
--- a/app/services/projects/update_pages_configuration_service.rb
+++ b/app/services/projects/update_pages_configuration_service.rb
@@ -2,6 +2,8 @@
 
 module Projects
   class UpdatePagesConfigurationService < BaseService
+    include Gitlab::Utils::StrongMemoize
+
     attr_reader :project
 
     def initialize(project)
@@ -9,15 +11,25 @@ module Projects
     end
 
     def execute
-      update_file(pages_config_file, pages_config.to_json)
+      if file_equals?(pages_config_file, pages_config_json)
+        return success(reload: false)
+      end
+
+      update_file(pages_config_file, pages_config_json)
       reload_daemon
-      success
+      success(reload: true)
     rescue => e
       error(e.message)
     end
 
     private
 
+    def pages_config_json
+      strong_memoize(:pages_config_json) do
+        pages_config.to_json
+      end
+    end
+
     def pages_config
       {
         domains: pages_domains_config,
@@ -67,11 +79,6 @@ module Projects
     end
 
     def update_file(file, data)
-      unless data
-        FileUtils.remove(file, force: true)
-        return
-      end
-
       temp_file = "#{file}.#{SecureRandom.hex(16)}"
       File.open(temp_file, 'w') do |f|
         f.write(data)
@@ -81,5 +88,18 @@ module Projects
       # In case if the updating fails
       FileUtils.remove(temp_file, force: true)
     end
+
+    def file_equals?(file, data)
+      existing_data = read_file(file)
+      data == existing_data.to_s
+    end
+
+    def read_file(file)
+      File.open(file, 'r') do |f|
+        f.read
+      end
+    rescue
+      nil
+    end
   end
 end
diff --git a/changelogs/unreleased/update-pages-config-only-when-changed.yml b/changelogs/unreleased/update-pages-config-only-when-changed.yml
new file mode 100644
index 00000000000..8d9e02df678
--- /dev/null
+++ b/changelogs/unreleased/update-pages-config-only-when-changed.yml
@@ -0,0 +1,5 @@
+---
+title: Do not reload daemon if configuration file of pages does not change
+merge_request:
+author:
+type: performance
diff --git a/spec/services/projects/update_pages_configuration_service_spec.rb b/spec/services/projects/update_pages_configuration_service_spec.rb
index e4d4e6ff3dd..7f5ef3129d7 100644
--- a/spec/services/projects/update_pages_configuration_service_spec.rb
+++ b/spec/services/projects/update_pages_configuration_service_spec.rb
@@ -2,23 +2,41 @@ require 'spec_helper'
 
 describe Projects::UpdatePagesConfigurationService do
   let(:project) { create(:project) }
-  subject { described_class.new(project) }
+  let(:service) { described_class.new(project) }
 
   describe "#update" do
     let(:file) { Tempfile.new('pages-test') }
 
+    subject { service.execute }
+
     after do
       file.close
       file.unlink
     end
 
-    it 'updates the .update file' do
-      # Access this reference to ensure scoping works
-      Projects::Settings # rubocop:disable Lint/Void
-      expect(subject).to receive(:pages_config_file).and_return(file.path)
-      expect(subject).to receive(:reload_daemon).and_call_original
+    before do
+      allow(service).to receive(:pages_config_file).and_return(file.path)
+    end
+
+    context 'when configuration changes' do
+      it 'updates the .update file' do
+        expect(service).to receive(:reload_daemon).and_call_original
+
+        expect(subject).to include(status: :success, reload: true)
+      end
+    end
+
+    context 'when configuration does not change' do
+      before do
+        # we set the configuration
+        service.execute
+      end
+
+      it 'does not update the .update file' do
+        expect(service).not_to receive(:reload_daemon)
 
-      expect(subject.execute).to eq({ status: :success })
+        expect(subject).to include(status: :success, reload: false)
+      end
     end
   end
 end