Merge branch '295187-roll-out-pages_migration_mark_as_not_deployed-feature-flag' into 'master'

Replace pages_migration_mark_as_not_deployed feature flag with env variable [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!55862

app/services/pages/migrate_from_legacy_storage_service.rb

@@ -2,11 +2,12 @@
 
 module Pages
   class MigrateFromLegacyStorageService
-    def initialize(logger, migration_threads:, batch_size:, ignore_invalid_entries:)
+    def initialize(logger, migration_threads:, batch_size:, ignore_invalid_entries:, mark_projects_as_not_deployed:)
       @logger = logger
       @migration_threads = migration_threads
       @batch_size = batch_size
       @ignore_invalid_entries = ignore_invalid_entries
+      @mark_projects_as_not_deployed = mark_projects_as_not_deployed
 
       @migrated = 0
       @errored = 0
@@ -60,7 +61,9 @@ module Pages
     def migrate_project(project)
       result = nil
       time = Benchmark.realtime do
-        result = ::Pages::MigrateLegacyStorageToDeploymentService.new(project, ignore_invalid_entries: @ignore_invalid_entries).execute
+        result = ::Pages::MigrateLegacyStorageToDeploymentService.new(project,
+                                                                      ignore_invalid_entries: @ignore_invalid_entries,
+                                                                      mark_projects_as_not_deployed: @mark_projects_as_not_deployed).execute
       end
 
       if result[:status] == :success

app/services/pages/migrate_legacy_storage_to_deployment_service.rb

@@ -9,9 +9,10 @@ module Pages
 
     attr_reader :project
 
-    def initialize(project, ignore_invalid_entries: false)
+    def initialize(project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false)
       @project = project
       @ignore_invalid_entries = ignore_invalid_entries
+      @mark_projects_as_not_deployed = mark_projects_as_not_deployed
     end
 
     def execute
@@ -36,10 +37,12 @@ module Pages
       archive_path = zip_result[:archive_path]
 
       unless archive_path
+        return error("Archive not created. Missing public directory in #{@project.pages_path}") unless @mark_projects_as_not_deployed
+
         project.set_first_pages_deployment!(nil)
 
         return success(
-          message: "Archive not created. Missing public directory in #{project.pages_path} ? Marked project as not deployed")
+          message: "Archive not created. Missing public directory in #{project.pages_path}? Marked project as not deployed")
       end
 
       deployment = nil

app/services/pages/zip_directory_service.rb

@@ -18,13 +18,7 @@ module Pages
     end
 
     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
+      return success unless resolve_public_dir
 
       output_file = File.join(real_dir, "@migrated.zip") # '@' to avoid any name collision with groups or projects
 

changelogs/unreleased/295187-roll-out-pages_migration_mark_as_not_deployed-feature-flag.yml

+---
+title: Allow users to mark pages projects as not deployed during migration to zip
+  storage
+merge_request: 55862
+author:
+type: added

config/feature_flags/development/pages_migration_mark_as_not_deployed.yml

----
-name: pages_migration_mark_as_not_deployed
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/49473
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/295187
-milestone: '13.8'
-type: development
-group: group::release
-default_enabled: false

lib/tasks/gitlab/pages.rake

@@ -11,7 +11,8 @@ namespace :gitlab do
       result = ::Pages::MigrateFromLegacyStorageService.new(logger,
                                                             migration_threads: migration_threads,
                                                             batch_size: batch_size,
-                                                            ignore_invalid_entries: ignore_invalid_entries).execute
+                                                            ignore_invalid_entries: ignore_invalid_entries,
+                                                            mark_projects_as_not_deployed: mark_projects_as_not_deployed).execute
 
       logger.info("A total of #{result[:migrated] + result[:errored]} projects were processed.")
       logger.info("- The #{result[:migrated]} projects migrated successfully")
@@ -51,5 +52,11 @@ namespace :gitlab do
         ENV.fetch('PAGES_MIGRATION_IGNORE_INVALID_ENTRIES', 'false')
       )
     end
+
+    def mark_projects_as_not_deployed
+      Gitlab::Utils.to_boolean(
+        ENV.fetch('PAGES_MIGRATION_MARK_PROJECTS_AS_NOT_DEPLOYED', 'false')
+      )
+    end
   end
 end

spec/services/pages/migrate_from_legacy_storage_service_spec.rb

@@ -3,7 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Pages::MigrateFromLegacyStorageService do
-  let(:service) { described_class.new(Rails.logger, migration_threads: 3, batch_size: 10, ignore_invalid_entries: false) }
+  let(:batch_size) { 10 }
+  let(:mark_projects_as_not_deployed) { false }
+  let(:service) { described_class.new(Rails.logger, migration_threads: 3, batch_size: batch_size, ignore_invalid_entries: false, mark_projects_as_not_deployed: mark_projects_as_not_deployed) }
 
   it 'does not try to migrate pages if pages are not deployed' do
     expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new)
@@ -11,33 +13,35 @@ RSpec.describe Pages::MigrateFromLegacyStorageService do
     expect(service.execute).to eq(migrated: 0, errored: 0)
   end
 
-  it 'uses multiple threads' do
-    projects = create_list(:project, 20)
-    projects.each do |project|
-      project.mark_pages_as_deployed
+  context 'when there is work for multiple threads' do
+    let(:batch_size) { 2 } # override to force usage of multiple threads
+
+    it 'uses multiple threads' do
+      projects = create_list(:project, 20)
+      projects.each do |project|
+        project.mark_pages_as_deployed
 
-      FileUtils.mkdir_p File.join(project.pages_path, "public")
-      File.open(File.join(project.pages_path, "public/index.html"), "w") do |f|
-        f.write("Hello!")
+        FileUtils.mkdir_p File.join(project.pages_path, "public")
+        File.open(File.join(project.pages_path, "public/index.html"), "w") do |f|
+          f.write("Hello!")
+        end
       end
-    end
 
-    service = described_class.new(Rails.logger, migration_threads: 3, batch_size: 2, ignore_invalid_entries: false)
+      threads = Concurrent::Set.new
 
-    threads = Concurrent::Set.new
+      expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args|
+        threads.add(Thread.current)
 
-    expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args|
-      threads.add(Thread.current)
+        # sleep to be 100% certain that once thread can't consume all the queue
+        # it works without it, but I want to avoid making this test flaky
+        sleep(0.01)
 
-      # sleep to be 100% certain that once thread can't consume all the queue
-      # it works without it, but I want to avoid making this test flaky
-      sleep(0.01)
+        m.call(*args)
+      end
 
-      m.call(*args)
+      expect(service.execute).to eq(migrated: 20, errored: 0)
+      expect(threads.length).to eq(3)
     end
-
-    expect(service.execute).to eq(migrated: 20, errored: 0)
-    expect(threads.length).to eq(3)
   end
 
   context 'when pages are marked as deployed' do
@@ -48,12 +52,24 @@ RSpec.describe Pages::MigrateFromLegacyStorageService do
     end
 
     context 'when pages directory does not exist' do
-      it 'counts project as migrated' do
-        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service|
+      context 'when mark_projects_as_not_deployed is set' do
+        let(:mark_projects_as_not_deployed) { true }
+
+        it 'counts project as migrated' do
+          expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: true) do |service|
+            expect(service).to receive(:execute).and_call_original
+          end
+
+          expect(service.execute).to eq(migrated: 1, errored: 0)
+        end
+      end
+
+      it 'counts project as errored' do
+        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service|
           expect(service).to receive(:execute).and_call_original
         end
 
-        expect(service.execute).to eq(migrated: 1, errored: 0)
+        expect(service.execute).to eq(migrated: 0, errored: 1)
       end
     end
 
@@ -66,7 +82,7 @@ RSpec.describe Pages::MigrateFromLegacyStorageService do
       end
 
       it 'migrates pages projects without deployments' do
-        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service|
+        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service|
           expect(service).to receive(:execute).and_call_original
         end
 

spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb

@@ -11,47 +11,47 @@ 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(:success)
+    expect(described_class.new(project, ignore_invalid_entries: true).execute[:status]).to eq(:error)
   end
 
-  it 'marks pages as not deployed if public directory is absent' do
-    project.mark_pages_as_deployed
+  context 'when mark_projects_as_not_deployed is passed' do
+    let(:service) { described_class.new(project, mark_projects_as_not_deployed: true) }
 
-    expect(project.pages_metadatum.reload.deployed).to eq(true)
+    it 'marks pages as not deployed if public directory is absent and invalid entries are ignored' do
+      project.mark_pages_as_deployed
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
 
-    expect(service.execute).to(
-      eq(status: :success,
-         message: "Archive not created. Missing public directory in #{project.pages_path} ? Marked project as not deployed")
-    )
+      expect(service.execute).to(
+        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)
-  end
-
-  it 'does not mark pages as not deployed if public directory is absent but pages_deployment exists' do
-    deployment = create(:pages_deployment, project: project)
-    project.update_pages_deployment!(deployment)
-    project.mark_pages_as_deployed
+      expect(project.pages_metadatum.reload.deployed).to eq(false)
+    end
 
-    expect(project.pages_metadatum.reload.deployed).to eq(true)
+    it 'does not mark pages as not deployed if public directory is absent but pages_deployment exists' do
+      deployment = create(:pages_deployment, project: project)
+      project.update_pages_deployment!(deployment)
+      project.mark_pages_as_deployed
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
 
-    expect(service.execute).to(
-      eq(status: :success,
-         message: "Archive not created. Missing public directory in #{project.pages_path} ? Marked project as not deployed")
-    )
+      expect(service.execute).to(
+        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)
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
+    end
   end
 
-  it 'does not mark pages as not deployed if public directory is absent but feature is disabled' do
-    stub_feature_flags(pages_migration_mark_as_not_deployed: false)
-
+  it 'does not mark pages as not deployed if public directory is absent but invalid entries are not ignored' do
     project.mark_pages_as_deployed
 
     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}")
+         message: "Archive not created. Missing public directory in #{project.pages_path}")
     )
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)

spec/services/pages/zip_directory_service_spec.rb

@@ -33,15 +33,6 @@ RSpec.describe Pages::ZipDirectoryService do
       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(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
 
   context "when work direcotry doesn't exist" do

spec/tasks/gitlab/pages_rake_spec.rb

@@ -14,7 +14,8 @@ RSpec.describe 'gitlab:pages' do
       expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
                               migration_threads: 3,
                               batch_size: 10,
-                              ignore_invalid_entries: false) do |service|
+                              ignore_invalid_entries: false,
+                              mark_projects_as_not_deployed: false) do |service|
         expect(service).to receive(:execute).and_call_original
       end
 
@@ -27,7 +28,8 @@ RSpec.describe 'gitlab:pages' do
       expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
                               migration_threads: 5,
                               batch_size: 10,
-                              ignore_invalid_entries: false) do |service|
+                              ignore_invalid_entries: false,
+                              mark_projects_as_not_deployed: false) do |service|
         expect(service).to receive(:execute).and_call_original
       end
 
@@ -40,7 +42,8 @@ RSpec.describe 'gitlab:pages' do
       expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
                               migration_threads: 3,
                               batch_size: 100,
-                              ignore_invalid_entries: false) do |service|
+                              ignore_invalid_entries: false,
+                              mark_projects_as_not_deployed: false) do |service|
         expect(service).to receive(:execute).and_call_original
       end
 
@@ -53,7 +56,22 @@ RSpec.describe 'gitlab:pages' do
       expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
                               migration_threads: 3,
                               batch_size: 10,
-                              ignore_invalid_entries: true) do |service|
+                              ignore_invalid_entries: true,
+                              mark_projects_as_not_deployed: false) do |service|
+        expect(service).to receive(:execute).and_call_original
+      end
+
+      subject
+    end
+
+    it 'uses PAGES_MIGRATION_MARK_PROJECTS_AS_NOT_DEPLOYED environment variable' do
+      stub_env('PAGES_MIGRATION_MARK_PROJECTS_AS_NOT_DEPLOYED', 'true')
+
+      expect_next_instance_of(::Pages::MigrateFromLegacyStorageService, anything,
+                              migration_threads: 3,
+                              batch_size: 10,
+                              ignore_invalid_entries: false,
+                              mark_projects_as_not_deployed: true) do |service|
         expect(service).to receive(:execute).and_call_original
       end