Read about our upcoming Code of Conduct on this issue

Commit e0137d53 authored by Kamil Trzciński's avatar Kamil Trzciński
Browse files

Merge branch '45481-sane-pages-artifacts' into 'master'

Don't automatically remove artifacts for pages jobs after pages:deploy has run

Closes #45481

See merge request gitlab-org/gitlab-ce!18628
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 @@ def execute
@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,16 +31,16 @@ def execute
# 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, false)
error(e.message)
raise e
end
......@@ -48,17 +48,15 @@ def execute
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
......@@ -77,18 +75,18 @@ def extract_archive!(temp_path)
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.
......@@ -162,11 +160,6 @@ def artifacts
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
......
---
title: Don't automatically remove artifacts for pages jobs after pages:deploy has
run
merge_request: 18628
author:
type: fixed
......@@ -29,25 +29,10 @@
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" 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)
it "doesn't delete artifacts after deploying" 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 @@
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" 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)
it "doesn't delete artifacts after deploying" 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 @@
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 @@
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
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment