Merge branch '323228-ambiguous-error-message-when-missing-public-dir' into 'master'

Handle missing `public` pages directory as successful case

See merge request gitlab-org/gitlab!55829

app/models/project.rb

@@ -1848,7 +1848,7 @@ class Project < ApplicationRecord
     # where().update_all to perform update in the single transaction with check for null
     ProjectPagesMetadatum
       .where(project_id: id, pages_deployment_id: nil)
-      .update_all(pages_deployment_id: deployment.id)
+      .update_all(deployed: deployment.present?, pages_deployment_id: deployment&.id)
   end
 
   def write_repository_config(gl_full_path: full_path)

app/services/pages/migrate_from_legacy_storage_service.rb

@@ -64,7 +64,7 @@ module Pages
       end
 
       if result[:status] == :success
-        @logger.info("project_id: #{project.id} #{project.pages_path} has been migrated in #{time.round(2)} seconds")
+        @logger.info("project_id: #{project.id} #{project.pages_path} has been migrated in #{time.round(2)} seconds: #{result[:message]}")
         @counters_lock.synchronize { @migrated += 1 }
       else
         @logger.error("project_id: #{project.id} #{project.pages_path} failed to be migrated in #{time.round(2)} seconds: #{result[:message]}")

app/services/pages/migrate_legacy_storage_to_deployment_service.rb

@@ -30,16 +30,18 @@ module Pages
       zip_result = ::Pages::ZipDirectoryService.new(project.pages_path, ignore_invalid_entries: @ignore_invalid_entries).execute
 
       if zip_result[:status] == :error
-        if !project.pages_metadatum&.reload&.pages_deployment &&
-           Feature.enabled?(:pages_migration_mark_as_not_deployed, project)
-          project.mark_pages_as_not_deployed
-        end
-
         return error("Can't create zip archive: #{zip_result[:message]}")
       end
 
       archive_path = zip_result[:archive_path]
 
+      unless archive_path
+        project.set_first_pages_deployment!(nil)
+
+        return success(
+          message: "Archive not created. Missing public directory in #{project.pages_path} ? Marked project as not deployed")
+      end
+
       deployment = nil
       File.open(archive_path) do |file|
         deployment = project.pages_deployments.create!(

app/services/pages/zip_directory_service.rb

@@ -19,6 +19,10 @@ module Pages
 
     def execute
       unless resolve_public_dir
+        if Feature.enabled?(:pages_migration_mark_as_not_deployed)
+          return success
+        end
+
         return error("Can not find valid public dir in #{@input_dir}")
       end
 

spec/models/project_spec.rb

@@ -6016,12 +6016,15 @@ RSpec.describe Project, factory_default: :keep do
       project.set_first_pages_deployment!(deployment)
 
       expect(project.pages_metadatum.reload.pages_deployment).to eq(deployment)
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
     end
 
     it "updates the existing metadara record with deployment" do
       expect do
         project.set_first_pages_deployment!(deployment)
       end.to change { project.pages_metadatum.reload.pages_deployment }.from(nil).to(deployment)
+
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
     end
 
     it 'only updates metadata for this project' do
@@ -6030,6 +6033,8 @@ RSpec.describe Project, factory_default: :keep do
       expect do
         project.set_first_pages_deployment!(deployment)
       end.not_to change { other_project.pages_metadatum.reload.pages_deployment }.from(nil)
+
+      expect(other_project.pages_metadatum.reload.deployed).to eq(false)
     end
 
     it 'does nothing if metadata already references some deployment' do
@@ -6040,6 +6045,14 @@ RSpec.describe Project, factory_default: :keep do
         project.set_first_pages_deployment!(deployment)
       end.not_to change { project.pages_metadatum.reload.pages_deployment }.from(existing_deployment)
     end
+
+    it 'marks project as not deployed if deployment is nil' do
+      project.mark_pages_as_deployed
+
+      expect do
+        project.set_first_pages_deployment!(nil)
+      end.to change { project.pages_metadatum.reload.deployed }.from(true).to(false)
+    end
   end
 
   describe '#has_pool_repsitory?' do

spec/services/pages/migrate_from_legacy_storage_service_spec.rb

@@ -48,12 +48,12 @@ RSpec.describe Pages::MigrateFromLegacyStorageService do
     end
 
     context 'when pages directory does not exist' do
-      it 'tries to migrate the project, but does not crash' do
+      it 'counts project as migrated' do
         expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service|
           expect(service).to receive(:execute).and_call_original
         end
 
-        expect(service.execute).to eq(migrated: 0, errored: 1)
+        expect(service.execute).to eq(migrated: 1, errored: 0)
       end
     end
 

spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
       expect(zip_service).to receive(:execute).and_call_original
     end
 
-    expect(described_class.new(project, ignore_invalid_entries: true).execute[:status]).to eq(:error)
+    expect(described_class.new(project, ignore_invalid_entries: true).execute[:status]).to eq(:success)
   end
 
   it 'marks pages as not deployed if public directory is absent' do
@@ -20,8 +20,8 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
     expect(project.pages_metadatum.reload.deployed).to eq(true)
 
     expect(service.execute).to(
-      eq(status: :error,
-         message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}")
+      eq(status: :success,
+         message: "Archive not created. Missing public directory in #{project.pages_path} ? Marked project as not deployed")
     )
 
     expect(project.pages_metadatum.reload.deployed).to eq(false)
@@ -35,8 +35,8 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
     expect(project.pages_metadatum.reload.deployed).to eq(true)
 
     expect(service.execute).to(
-      eq(status: :error,
-         message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}")
+      eq(status: :success,
+         message: "Archive not created. Missing public directory in #{project.pages_path} ? Marked project as not deployed")
     )
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)

spec/services/pages/zip_directory_service_spec.rb

@@ -12,8 +12,10 @@ RSpec.describe Pages::ZipDirectoryService do
 
   let(:ignore_invalid_entries) { false }
 
+  let(:service_directory) { @work_dir }
+
   let(:service) do
-    described_class.new(@work_dir, ignore_invalid_entries: ignore_invalid_entries)
+    described_class.new(service_directory, ignore_invalid_entries: ignore_invalid_entries)
   end
 
   let(:result) do
@@ -25,32 +27,41 @@ RSpec.describe Pages::ZipDirectoryService do
   let(:archive) { result[:archive_path] }
   let(:entries_count) { result[:entries_count] }
 
-  it 'returns error if project pages dir does not exist' do
-    expect(Gitlab::ErrorTracking).not_to receive(:track_exception)
+  shared_examples 'handles invalid public directory' do
+    it 'returns success' do
+      expect(status).to eq(:success)
+      expect(archive).to be_nil
+      expect(entries_count).to be_nil
+    end
+
+    it 'returns error if pages_migration_mark_as_not_deployed is disabled' do
+      stub_feature_flags(pages_migration_mark_as_not_deployed: false)
 
-    expect(
-      described_class.new("/tmp/not/existing/dir").execute
-    ).to eq(status: :error, message: "Can not find valid public dir in /tmp/not/existing/dir")
+      expect(status).to eq(:error)
+      expect(message).to eq("Can not find valid public dir in #{service_directory}")
+      expect(archive).to be_nil
+      expect(entries_count).to be_nil
+    end
   end
 
-  it 'returns nils if there is no public directory and does not leave archive' do
-    expect(status).to eq(:error)
-    expect(message).to eq("Can not find valid public dir in #{@work_dir}")
-    expect(archive).to eq(nil)
-    expect(entries_count).to eq(nil)
+  context "when work direcotry doesn't exist" do
+    let(:service_directory) { "/tmp/not/existing/dir" }
 
-    expect(File.exist?(File.join(@work_dir, '@migrated.zip'))).to eq(false)
+    include_examples 'handles invalid public directory'
   end
 
-  it 'returns nils if public directory is a symlink' do
-    create_dir('target')
-    create_file('./target/index.html', 'hello')
-    create_link("public", "./target")
+  context 'when public directory is absent' do
+    include_examples 'handles invalid public directory'
+  end
+
+  context 'when public directory is a symlink' do
+    before do
+      create_dir('target')
+      create_file('./target/index.html', 'hello')
+      create_link("public", "./target")
+    end
 
-    expect(status).to eq(:error)
-    expect(message).to eq("Can not find valid public dir in #{@work_dir}")
-    expect(archive).to eq(nil)
-    expect(entries_count).to eq(nil)
+    include_examples 'handles invalid public directory'
   end
 
   context 'when there is a public directory' do