Commit 77a6afd5 authored by Kamil Trzciński's avatar Kamil Trzciński

Merge branch 'fix/sm/fix-wrong-error-handling-in-update-page-service' into 'master'

Fix wrong error handling in update page service

Closes #44817

See merge request gitlab-org/gitlab-ce!18098
parents be1523c1 a26ee804
...@@ -31,15 +31,17 @@ module Projects ...@@ -31,15 +31,17 @@ module Projects
# Check if we did extract public directory # Check if we did extract public directory
archive_public_path = File.join(archive_path, 'public') archive_public_path = File.join(archive_path, 'public')
raise FailedToExtractError, 'pages miss the public folder' unless Dir.exist?(archive_public_path) raise InvaildStateError, 'pages miss the public folder' unless Dir.exist?(archive_public_path)
raise InvaildStateError, 'pages are outdated' unless latest? raise InvaildStateError, 'pages are outdated' unless latest?
deploy_page!(archive_public_path) deploy_page!(archive_public_path)
success success
end end
rescue InvaildStateError, FailedToExtractError => e rescue InvaildStateError => e
register_failure
error(e.message) error(e.message)
rescue => e
error(e.message, false)
raise e
end end
private private
...@@ -50,12 +52,13 @@ module Projects ...@@ -50,12 +52,13 @@ module Projects
super super
end end
def error(message, http_status = nil) def error(message, allow_delete_artifact = true)
register_failure
log_error("Projects::UpdatePagesService: #{message}") log_error("Projects::UpdatePagesService: #{message}")
@status.allow_failure = !latest? @status.allow_failure = !latest?
@status.description = message @status.description = message
@status.drop(:script_failure) @status.drop(:script_failure)
delete_artifact! delete_artifact! if allow_delete_artifact
super super
end end
...@@ -76,7 +79,7 @@ module Projects ...@@ -76,7 +79,7 @@ module Projects
elsif artifacts.ends_with?('.zip') elsif artifacts.ends_with?('.zip')
extract_zip_archive!(temp_path) extract_zip_archive!(temp_path)
else else
raise FailedToExtractError, 'unsupported artifacts format' raise InvaildStateError, 'unsupported artifacts format'
end end
end end
...@@ -91,13 +94,13 @@ module Projects ...@@ -91,13 +94,13 @@ module Projects
end end
def extract_zip_archive!(temp_path) def extract_zip_archive!(temp_path)
raise FailedToExtractError, 'missing artifacts metadata' unless build.artifacts_metadata? raise InvaildStateError, 'missing artifacts metadata' unless build.artifacts_metadata?
# Calculate page size after extract # Calculate page size after extract
public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true) public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true)
if public_entry.total_size > max_size if public_entry.total_size > max_size
raise FailedToExtractError, "artifacts for pages are too large: #{public_entry.total_size}" raise InvaildStateError, "artifacts for pages are too large: #{public_entry.total_size}"
end end
# Requires UnZip at least 6.00 Info-ZIP. # Requires UnZip at least 6.00 Info-ZIP.
......
...@@ -87,7 +87,8 @@ describe Projects::UpdatePagesService do ...@@ -87,7 +87,8 @@ describe Projects::UpdatePagesService do
it 'fails for empty file fails' do it 'fails for empty file fails' do
build.update_attributes(legacy_artifacts_file: empty_file) build.update_attributes(legacy_artifacts_file: empty_file)
expect(execute).not_to eq(:success) expect { execute }
.to raise_error(Projects::UpdatePagesService::FailedToExtractError)
end end
end end
end end
...@@ -159,7 +160,8 @@ describe Projects::UpdatePagesService do ...@@ -159,7 +160,8 @@ describe Projects::UpdatePagesService do
it 'fails for empty file fails' do it 'fails for empty file fails' do
build.job_artifacts_archive.update_attributes(file: empty_file) build.job_artifacts_archive.update_attributes(file: empty_file)
expect(execute).not_to eq(:success) expect { execute }
.to raise_error(Projects::UpdatePagesService::FailedToExtractError)
end end
context 'when timeout happens by DNS error' do context 'when timeout happens by DNS error' do
...@@ -172,7 +174,39 @@ describe Projects::UpdatePagesService do ...@@ -172,7 +174,39 @@ describe Projects::UpdatePagesService do
expect { execute }.to raise_error(SocketError) expect { execute }.to raise_error(SocketError)
build.reload build.reload
expect(build.artifacts?).to eq(true) 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)
.to receive(:extract_zip_archive!)
.and_raise(Projects::UpdatePagesService::FailedToExtractError)
end
it 'raises an error' do
expect { execute }
.to raise_error(Projects::UpdatePagesService::FailedToExtractError)
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)
end
it 'does not raise an error and remove artifacts as failed job' do
execute
build.reload
expect(deploy_status).to be_failed
expect(build.artifacts?).to be_falsey
end end
end 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