From e46cec3ac1a241783c787fc7661ef70d608c269e Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin <v.shushlin@gmail.com> Date: Fri, 5 Mar 2021 16:31:01 +0300 Subject: [PATCH] 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 | 2 +- .../migrate_from_legacy_storage_service.rb | 2 +- ...te_legacy_storage_to_deployment_service.rb | 12 +++-- app/services/pages/zip_directory_service.rb | 4 ++ spec/models/project_spec.rb | 13 +++++ ...igrate_from_legacy_storage_service_spec.rb | 4 +- ...gacy_storage_to_deployment_service_spec.rb | 10 ++-- .../pages/zip_directory_service_spec.rb | 51 +++++++++++-------- 8 files changed, 64 insertions(+), 34 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index ef92dda443a..f942dea3410 100644 --- a/app/models/project.rb +++ b/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) diff --git a/app/services/pages/migrate_from_legacy_storage_service.rb b/app/services/pages/migrate_from_legacy_storage_service.rb index 9b36b3f11b4..37e701ce5ba 100644 --- a/app/services/pages/migrate_from_legacy_storage_service.rb +++ b/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]}") diff --git a/app/services/pages/migrate_legacy_storage_to_deployment_service.rb b/app/services/pages/migrate_legacy_storage_to_deployment_service.rb index 63410b9fe4a..3bffed4caf6 100644 --- a/app/services/pages/migrate_legacy_storage_to_deployment_service.rb +++ b/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!( diff --git a/app/services/pages/zip_directory_service.rb b/app/services/pages/zip_directory_service.rb index ae08d40ee37..2f4995899a1 100644 --- a/app/services/pages/zip_directory_service.rb +++ b/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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b4010ba6e31..11f4d6241da 100644 --- a/spec/models/project_spec.rb +++ b/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 diff --git a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb index 4ec57044912..b3328d540e7 100644 --- a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb +++ b/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 diff --git a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb b/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb index d95303c3e85..be9dd69ffd3 100644 --- a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb +++ b/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) diff --git a/spec/services/pages/zip_directory_service_spec.rb b/spec/services/pages/zip_directory_service_spec.rb index 9de68dd62bb..a34583413d2 100644 --- a/spec/services/pages/zip_directory_service_spec.rb +++ b/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 -- 2.30.9