Merge branch '297228-introduce-pages_serve_from_disk-ff' into 'master'

Introduce feature flag to disable serving from pages legacy storage

See merge request gitlab-org/gitlab!51465

app/models/pages/lookup_path.rb

@@ -4,6 +4,8 @@ module Pages
   class LookupPath
     include Gitlab::Utils::StrongMemoize
 
+    LegacyStorageDisabledError = Class.new(::StandardError)
+
     def initialize(project, trim_prefix: nil, domain: nil)
       @project = project
       @domain = domain
@@ -24,7 +26,7 @@ module Pages
     end
 
     def source
-      zip_source || file_source
+      zip_source || legacy_source
     end
 
     def prefix
@@ -64,11 +66,17 @@ module Pages
       }
     end
 
-    def file_source
+    def legacy_source
+      raise LegacyStorageDisabledError unless Feature.enabled?(:pages_serve_from_legacy_storage, default_enabled: true)
+
       {
         type: 'file',
         path: File.join(project.full_path, 'public/')
       }
+    rescue LegacyStorageDisabledError => e
+      Gitlab::ErrorTracking.track_exception(e)
+
+      nil
     end
   end
 end

app/models/pages/virtual_domain.rb

@@ -17,9 +17,16 @@ module Pages
     end
 
     def lookup_paths
-      projects.map do |project|
+      paths = projects.map do |project|
         project.pages_lookup_path(trim_prefix: trim_prefix, domain: domain)
-      end.sort_by(&:prefix).reverse
+      end
+
+      # TODO: remove in https://gitlab.com/gitlab-org/gitlab/-/issues/297524
+      # source can only be nil if pages_serve_from_legacy_storage FF is disabled
+      # we can remove this filtering once we remove legacy storage
+      paths = paths.select(&:source)
+
+      paths.sort_by(&:prefix).reverse
     end
 
     private

config/feature_flags/development/pages_serve_from_legacy_storage.yml

+---
+name: pages_serve_from_legacy_storage
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/issues/297228
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/297524
+milestone: '13.8'
+type: development
+group: group::release
+default_enabled: true

spec/models/pages/lookup_path_spec.rb

@@ -56,6 +56,15 @@ RSpec.describe Pages::LookupPath do
 
     include_examples 'uses disk storage'
 
+    it 'return nil when legacy storage is disabled and there is no deployment' do
+      stub_feature_flags(pages_serve_from_legacy_storage: false)
+      expect(Gitlab::ErrorTracking).to receive(:track_exception)
+                                         .with(described_class::LegacyStorageDisabledError)
+                                         .and_call_original
+
+      expect(source).to eq(nil)
+    end
+
     context 'when there is pages deployment' do
       let(:deployment) { create(:pages_deployment, project: project) }
 

spec/models/pages/virtual_domain_spec.rb

@@ -26,31 +26,34 @@ RSpec.describe Pages::VirtualDomain do
 
   describe '#lookup_paths' do
     let(:project_a) { instance_double(Project) }
-    let(:project_z) { instance_double(Project) }
-    let(:pages_lookup_path_a) { instance_double(Pages::LookupPath, prefix: 'aaa') }
-    let(:pages_lookup_path_z) { instance_double(Pages::LookupPath, prefix: 'zzz') }
+    let(:project_b) { instance_double(Project) }
+    let(:project_c) { instance_double(Project) }
+    let(:pages_lookup_path_a) { instance_double(Pages::LookupPath, prefix: 'aaa', source: { type: 'zip', path: 'https://example.com' }) }
+    let(:pages_lookup_path_b) { instance_double(Pages::LookupPath, prefix: 'bbb', source: { type: 'zip', path: 'https://example.com' }) }
+    let(:pages_lookup_path_without_source) { instance_double(Pages::LookupPath, prefix: 'ccc', source: nil) }
 
     context 'when there is pages domain provided' do
       let(:domain) { instance_double(PagesDomain) }
 
-      subject(:virtual_domain) { described_class.new([project_a, project_z], domain: domain) }
+      subject(:virtual_domain) { described_class.new([project_a, project_b, project_c], domain: domain) }
 
       it 'returns collection of projects pages lookup paths sorted by prefix in reverse' do
         expect(project_a).to receive(:pages_lookup_path).with(domain: domain, trim_prefix: nil).and_return(pages_lookup_path_a)
-        expect(project_z).to receive(:pages_lookup_path).with(domain: domain, trim_prefix: nil).and_return(pages_lookup_path_z)
+        expect(project_b).to receive(:pages_lookup_path).with(domain: domain, trim_prefix: nil).and_return(pages_lookup_path_b)
+        expect(project_c).to receive(:pages_lookup_path).with(domain: domain, trim_prefix: nil).and_return(pages_lookup_path_without_source)
 
-        expect(virtual_domain.lookup_paths).to eq([pages_lookup_path_z, pages_lookup_path_a])
+        expect(virtual_domain.lookup_paths).to eq([pages_lookup_path_b, pages_lookup_path_a])
       end
     end
 
     context 'when there is trim_prefix provided' do
-      subject(:virtual_domain) { described_class.new([project_a, project_z], trim_prefix: 'group/') }
+      subject(:virtual_domain) { described_class.new([project_a, project_b], trim_prefix: 'group/') }
 
       it 'returns collection of projects pages lookup paths sorted by prefix in reverse' do
         expect(project_a).to receive(:pages_lookup_path).with(trim_prefix: 'group/', domain: nil).and_return(pages_lookup_path_a)
-        expect(project_z).to receive(:pages_lookup_path).with(trim_prefix: 'group/', domain: nil).and_return(pages_lookup_path_z)
+        expect(project_b).to receive(:pages_lookup_path).with(trim_prefix: 'group/', domain: nil).and_return(pages_lookup_path_b)
 
-        expect(virtual_domain.lookup_paths).to eq([pages_lookup_path_z, pages_lookup_path_a])
+        expect(virtual_domain.lookup_paths).to eq([pages_lookup_path_b, pages_lookup_path_a])
       end
     end
   end