From 1a7d9346682af5c496d0a974180eb4054ccae8af Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 30 Mar 2018 16:50:23 +0900 Subject: [PATCH 1/3] Fix wrong error handling in update page service --- app/services/projects/update_pages_service.rb | 16 +++++---- .../projects/update_pages_service_spec.rb | 35 +++++++++++++++++-- 2 files changed, 42 insertions(+), 9 deletions(-) diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 5bf8208e035..9f7243b9ae1 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -37,9 +37,10 @@ module Projects deploy_page!(archive_public_path) success end - rescue InvaildStateError, FailedToExtractError => e - register_failure + rescue InvaildStateError => e error(e.message) + rescue => e + error(e.message, false) end private @@ -50,12 +51,13 @@ module Projects super end - def error(message, http_status = nil) + def error(message, delete_artifact = true) + register_failure log_error("Projects::UpdatePagesService: #{message}") @status.allow_failure = !latest? @status.description = message @status.drop(:script_failure) - delete_artifact! + delete_artifact! if delete_artifact super end @@ -76,7 +78,7 @@ module Projects elsif artifacts.ends_with?('.zip') extract_zip_archive!(temp_path) else - raise FailedToExtractError, 'unsupported artifacts format' + raise InvaildStateError, 'unsupported artifacts format' end end @@ -91,13 +93,13 @@ module Projects end 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 public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true) 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 # Requires UnZip at least 6.00 Info-ZIP. diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 934106627a9..0c8c05168ce 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -169,10 +169,41 @@ describe Projects::UpdatePagesService do end it 'raises an error' do - expect { execute }.to raise_error(SocketError) + execute 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 + execute + + 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 'raises an error' do + execute + + build.reload + expect(deploy_status).to be_failed + expect(build.artifacts?).to be_falsey end end end From b4d9d4db84933a689ed308bd11faf4442e2bbb5f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 30 Mar 2018 16:55:33 +0900 Subject: [PATCH 2/3] Fix bad naming --- app/services/projects/update_pages_service.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 9f7243b9ae1..0a2ac7315ec 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -51,13 +51,13 @@ module Projects super end - def error(message, delete_artifact = true) + def error(message, allow_delete_artifact = true) register_failure log_error("Projects::UpdatePagesService: #{message}") @status.allow_failure = !latest? @status.description = message @status.drop(:script_failure) - delete_artifact! if delete_artifact + delete_artifact! if allow_delete_artifact super end From a26ee804d01912d49d982dfbde8e5fe1c198e04f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 30 Mar 2018 17:20:47 +0900 Subject: [PATCH 3/3] missing the public folder is InvaildStateError --- app/services/projects/update_pages_service.rb | 3 ++- spec/services/projects/update_pages_service_spec.rb | 13 ++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 0a2ac7315ec..3172df28db6 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -31,7 +31,7 @@ module Projects # Check if we did extract public directory 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? deploy_page!(archive_public_path) @@ -41,6 +41,7 @@ module Projects error(e.message) rescue => e error(e.message, false) + raise e end private diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index 0c8c05168ce..dd31a677dfe 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -87,7 +87,8 @@ describe Projects::UpdatePagesService do it 'fails for empty file fails' do 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 @@ -159,7 +160,8 @@ describe Projects::UpdatePagesService do it 'fails for empty file fails' do build.job_artifacts_archive.update_attributes(file: empty_file) - expect(execute).not_to eq(:success) + expect { execute } + .to raise_error(Projects::UpdatePagesService::FailedToExtractError) end context 'when timeout happens by DNS error' do @@ -169,7 +171,7 @@ describe Projects::UpdatePagesService do end it 'raises an error' do - execute + expect { execute }.to raise_error(SocketError) build.reload expect(deploy_status).to be_failed @@ -185,7 +187,8 @@ describe Projects::UpdatePagesService do end it 'raises an error' do - execute + expect { execute } + .to raise_error(Projects::UpdatePagesService::FailedToExtractError) build.reload expect(deploy_status).to be_failed @@ -198,7 +201,7 @@ describe Projects::UpdatePagesService do allow(build).to receive(:artifacts_metadata?).and_return(false) end - it 'raises an error' do + it 'does not raise an error and remove artifacts as failed job' do execute build.reload