From ad113a3cf803f95950be1a2af12eb3ad6f512ecd Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Fri, 27 Apr 2018 18:30:52 +0100 Subject: [PATCH 1/2] Don't automatically remove artifacts for pages jobs after pages:deploy has run --- app/services/projects/update_pages_service.rb | 11 +--- .../unreleased/45481-sane-pages-artifacts.yml | 6 +++ .../projects/update_pages_service_spec.rb | 51 ++++--------------- 3 files changed, 17 insertions(+), 51 deletions(-) create mode 100644 changelogs/unreleased/45481-sane-pages-artifacts.yml diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index de77f6bf585..bc3eae36574 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -40,7 +40,7 @@ module Projects rescue InvaildStateError => e error(e.message) rescue => e - error(e.message, false) + error(e.message) raise e end @@ -48,17 +48,15 @@ module Projects def success @status.success - delete_artifact! super end - def error(message, allow_delete_artifact = true) + def error(message) register_failure log_error("Projects::UpdatePagesService: #{message}") @status.allow_failure = !latest? @status.description = message @status.drop(:script_failure) - delete_artifact! if allow_delete_artifact super end @@ -162,11 +160,6 @@ module Projects build.artifacts_file.path end - def delete_artifact! - build.reload # Reload stable object to prevent erase artifacts with old state - build.erase_artifacts! unless build.has_expiring_artifacts? - end - def latest_sha project.commit(build.ref).try(:sha).to_s ensure diff --git a/changelogs/unreleased/45481-sane-pages-artifacts.yml b/changelogs/unreleased/45481-sane-pages-artifacts.yml new file mode 100644 index 00000000000..b9c68b70012 --- /dev/null +++ b/changelogs/unreleased/45481-sane-pages-artifacts.yml @@ -0,0 +1,6 @@ +--- +title: Don't automatically remove artifacts for pages jobs after pages:deploy has + run +merge_request: 18628 +author: +type: fixed diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 1b6caeab15d..a418808fd26 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -29,25 +29,10 @@ describe Projects::UpdatePagesService do end describe 'pages artifacts' do - context 'with expiry date' do - before do - build.artifacts_expire_in = "2 days" - build.save! - end + it "doesn't delete artifacts after deploying" do + expect(execute).to eq(:success) - it "doesn't delete artifacts" do - expect(execute).to eq(:success) - - expect(build.reload.artifacts?).to eq(true) - end - end - - context 'without expiry date' do - it "does delete artifacts" do - expect(execute).to eq(:success) - - expect(build.reload.artifacts?).to eq(false) - end + expect(build.reload.artifacts?).to eq(true) end end @@ -100,25 +85,10 @@ describe Projects::UpdatePagesService do end describe 'pages artifacts' do - context 'with expiry date' do - before do - build.artifacts_expire_in = "2 days" - build.save! - end + it "doesn't delete artifacts after deploying" do + expect(execute).to eq(:success) - it "doesn't delete artifacts" do - expect(execute).to eq(:success) - - expect(build.artifacts?).to eq(true) - end - end - - context 'without expiry date' do - it "does delete artifacts" do - expect(execute).to eq(:success) - - expect(build.reload.artifacts?).to eq(false) - end + expect(build.artifacts?).to eq(true) end end @@ -171,13 +141,12 @@ describe Projects::UpdatePagesService do build.reload expect(deploy_status).to be_failed - expect(build.artifacts?).to be_truthy end end context 'when failed to extract zip artifacts' do before do - allow_any_instance_of(described_class) + expect_any_instance_of(described_class) .to receive(:extract_zip_archive!) .and_raise(Projects::UpdatePagesService::FailedToExtractError) end @@ -188,21 +157,19 @@ describe Projects::UpdatePagesService do build.reload expect(deploy_status).to be_failed - expect(build.artifacts?).to be_truthy end end context 'when missing artifacts metadata' do before do - allow(build).to receive(:artifacts_metadata?).and_return(false) + expect(build).to receive(:artifacts_metadata?).and_return(false) end - it 'does not raise an error and remove artifacts as failed job' do + it 'does not raise an error as failed job' do execute build.reload expect(deploy_status).to be_failed - expect(build.artifacts?).to be_falsey end end end From 47ac3347d6557ba3413fed338923273563169586 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Fri, 27 Apr 2018 18:40:31 +0100 Subject: [PATCH 2/2] InvaildStateError -> InvalidStateError --- app/services/projects/update_pages_service.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index bc3eae36574..1d8caec9c6f 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -1,6 +1,6 @@ module Projects class UpdatePagesService < BaseService - InvaildStateError = Class.new(StandardError) + InvalidStateError = Class.new(StandardError) FailedToExtractError = Class.new(StandardError) BLOCK_SIZE = 32.kilobytes @@ -21,8 +21,8 @@ module Projects @status.enqueue! @status.run! - raise InvaildStateError, 'missing pages artifacts' unless build.artifacts? - raise InvaildStateError, 'pages are outdated' unless latest? + raise InvalidStateError, 'missing pages artifacts' unless build.artifacts? + raise InvalidStateError, 'pages are outdated' unless latest? # Create temporary directory in which we will extract the artifacts FileUtils.mkdir_p(tmp_path) @@ -31,13 +31,13 @@ module Projects # Check if we did extract public directory archive_public_path = File.join(archive_path, 'public') - raise InvaildStateError, 'pages miss the public folder' unless Dir.exist?(archive_public_path) - raise InvaildStateError, 'pages are outdated' unless latest? + raise InvalidStateError, 'pages miss the public folder' unless Dir.exist?(archive_public_path) + raise InvalidStateError, 'pages are outdated' unless latest? deploy_page!(archive_public_path) success end - rescue InvaildStateError => e + rescue InvalidStateError => e error(e.message) rescue => e error(e.message) @@ -75,18 +75,18 @@ module Projects if artifacts.ends_with?('.zip') extract_zip_archive!(temp_path) else - raise InvaildStateError, 'unsupported artifacts format' + raise InvalidStateError, 'unsupported artifacts format' end end def extract_zip_archive!(temp_path) - raise InvaildStateError, 'missing artifacts metadata' unless build.artifacts_metadata? + raise InvalidStateError, 'missing artifacts metadata' unless build.artifacts_metadata? # Calculate page size after extract public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true) if public_entry.total_size > max_size - raise InvaildStateError, "artifacts for pages are too large: #{public_entry.total_size}" + raise InvalidStateError, "artifacts for pages are too large: #{public_entry.total_size}" end # Requires UnZip at least 6.00 Info-ZIP.