Treat projects without public directory as successfully migrated

Currently even if mark projects as not deployed we report an error
to users which isn't correct.

This commit changes the error message to be successfull

app/models/project.rb

@@ -1845,7 +1845,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

@@ -5950,12 +5950,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
@@ -5964,6 +5967,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
@@ -5974,6 +5979,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