From 75a35860615d2c8cb85640d8ec0e2c936c952bf9 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Tue, 5 Dec 2023 17:35:46 +0000 Subject: [PATCH] consistent promise behavior for upload artifact --- .../__tests__/upload-artifact.test.ts | 19 ++++----- packages/artifact/src/artifact.ts | 1 + .../artifact/src/internal/shared/errors.ts | 21 ++++++++++ .../src/internal/shared/interfaces.ts | 5 --- .../src/internal/upload/blob-upload.ts | 42 ++++++------------- .../src/internal/upload/upload-artifact.ts | 28 +++++-------- 6 files changed, 53 insertions(+), 63 deletions(-) create mode 100644 packages/artifact/src/internal/shared/errors.ts diff --git a/packages/artifact/__tests__/upload-artifact.test.ts b/packages/artifact/__tests__/upload-artifact.test.ts index 439a96a3..b0dca5c8 100644 --- a/packages/artifact/__tests__/upload-artifact.test.ts +++ b/packages/artifact/__tests__/upload-artifact.test.ts @@ -7,6 +7,7 @@ import {Timestamp, ArtifactServiceClientJSON} from '../src/generated' import * as blobUpload from '../src/internal/upload/blob-upload' import {uploadArtifact} from '../src/internal/upload/upload-artifact' import {noopLogs} from './common' +import {FilesNotFoundError} from '../src/internal/shared/errors' describe('upload-artifact', () => { beforeEach(() => { @@ -59,7 +60,6 @@ describe('upload-artifact', () => { ) jest.spyOn(blobUpload, 'uploadZipToBlobStorage').mockReturnValue( Promise.resolve({ - isSuccess: true, uploadSize: 1234, sha256Hash: 'test-sha256-hash' }) @@ -84,7 +84,7 @@ describe('upload-artifact', () => { '/home/user/files/plz-upload' ) - expect(uploadResp).resolves.toEqual({success: true, size: 1234, id: 1}) + expect(uploadResp).resolves.toEqual({size: 1234, id: 1}) }) it('should throw an error if the root directory is invalid', () => { @@ -107,7 +107,7 @@ describe('upload-artifact', () => { expect(uploadResp).rejects.toThrow('Invalid root directory') }) - it('should return false if there are no files to upload', () => { + it('should reject if there are no files to upload', () => { jest .spyOn(uploadZipSpecification, 'validateRootDirectory') .mockReturnValue() @@ -124,7 +124,7 @@ describe('upload-artifact', () => { ], '/home/user/files/plz-upload' ) - expect(uploadResp).resolves.toEqual({success: false}) + expect(uploadResp).rejects.toThrowError(FilesNotFoundError) }) it('should reject if no backend IDs are found', () => { @@ -217,7 +217,7 @@ describe('upload-artifact', () => { '/home/user/files/plz-upload' ) - expect(uploadResp).resolves.toEqual({success: false}) + expect(uploadResp).rejects.toThrow() }) it('should return false if blob storage upload is unsuccessful', () => { @@ -262,7 +262,7 @@ describe('upload-artifact', () => { ) jest .spyOn(blobUpload, 'uploadZipToBlobStorage') - .mockReturnValue(Promise.resolve({isSuccess: false})) + .mockReturnValue(Promise.reject(new Error('boom'))) // ArtifactHttpClient mocks jest.spyOn(config, 'getRuntimeToken').mockReturnValue('test-token') @@ -280,10 +280,10 @@ describe('upload-artifact', () => { '/home/user/files/plz-upload' ) - expect(uploadResp).resolves.toEqual({success: false}) + expect(uploadResp).rejects.toThrow() }) - it('should return false if finalize artifact fails', () => { + it('should reject if finalize artifact fails', () => { const mockDate = new Date('2020-01-01') jest .spyOn(uploadZipSpecification, 'validateRootDirectory') @@ -325,7 +325,6 @@ describe('upload-artifact', () => { ) jest.spyOn(blobUpload, 'uploadZipToBlobStorage').mockReturnValue( Promise.resolve({ - isSuccess: true, uploadSize: 1234, sha256Hash: 'test-sha256-hash' }) @@ -350,6 +349,6 @@ describe('upload-artifact', () => { '/home/user/files/plz-upload' ) - expect(uploadResp).resolves.toEqual({success: false}) + expect(uploadResp).rejects.toThrow() }) }) diff --git a/packages/artifact/src/artifact.ts b/packages/artifact/src/artifact.ts index 5742b32d..3ba38448 100644 --- a/packages/artifact/src/artifact.ts +++ b/packages/artifact/src/artifact.ts @@ -4,6 +4,7 @@ import {ArtifactClient, Client} from './internal/client' * Exported functionality that we want to expose for any users of @actions/artifact */ export * from './internal/shared/interfaces' +export * from './internal/shared/errors' export {ArtifactClient} export function create(): ArtifactClient { diff --git a/packages/artifact/src/internal/shared/errors.ts b/packages/artifact/src/internal/shared/errors.ts new file mode 100644 index 00000000..37b92d14 --- /dev/null +++ b/packages/artifact/src/internal/shared/errors.ts @@ -0,0 +1,21 @@ +export class FilesNotFoundError extends Error { + files: string[] + + constructor(files: string[] = []) { + let message = 'No files were found to upload' + if (files.length > 0) { + message += `: ${files.join(', ')}` + } + + super(message) + this.files = files + this.name = 'FilesNotFoundError' + } +} + +export class InvalidResponseError extends Error { + constructor(message: string) { + super(message) + this.name = 'InvalidResponseError' + } +} diff --git a/packages/artifact/src/internal/shared/interfaces.ts b/packages/artifact/src/internal/shared/interfaces.ts index 61994b2b..654c5981 100644 --- a/packages/artifact/src/internal/shared/interfaces.ts +++ b/packages/artifact/src/internal/shared/interfaces.ts @@ -4,11 +4,6 @@ * * *****************************************************************************/ export interface UploadArtifactResponse { - /** - * Denotes if an artifact was successfully uploaded - */ - success: boolean - /** * Total size of the artifact in bytes. Not provided if no artifact was uploaded */ diff --git a/packages/artifact/src/internal/upload/blob-upload.ts b/packages/artifact/src/internal/upload/blob-upload.ts index cb7a11b7..59472385 100644 --- a/packages/artifact/src/internal/upload/blob-upload.ts +++ b/packages/artifact/src/internal/upload/blob-upload.ts @@ -7,11 +7,6 @@ import * as crypto from 'crypto' import * as stream from 'stream' export interface BlobUploadResponse { - /** - * If the upload was successful or not - */ - isSuccess: boolean - /** * The total reported upload size in bytes. Empty if the upload failed */ @@ -55,41 +50,28 @@ export async function uploadZipToBlobStorage( zipUploadStream.pipe(uploadStream) // This stream is used for the upload zipUploadStream.pipe(hashStream).setEncoding('hex') // This stream is used to compute a hash of the zip content that gets used. Integrity check - try { - core.info('Beginning upload of artifact content to blob storage') + core.info('Beginning upload of artifact content to blob storage') - await blockBlobClient.uploadStream( - uploadStream, - bufferSize, - maxConcurrency, - options - ) + await blockBlobClient.uploadStream( + uploadStream, + bufferSize, + maxConcurrency, + options + ) - core.info('Finished uploading artifact content to blob storage!') + core.info('Finished uploading artifact content to blob storage!') - hashStream.end() - sha256Hash = hashStream.read() as string - core.info(`SHA256 hash of uploaded artifact zip is ${sha256Hash}`) - } catch (error) { - core.warning( - `Failed to upload artifact zip to blob storage, error: ${error}` - ) - return { - isSuccess: false - } - } + hashStream.end() + sha256Hash = hashStream.read() as string + core.info(`SHA256 hash of uploaded artifact zip is ${sha256Hash}`) if (uploadByteCount === 0) { core.warning( - `No data was uploaded to blob storage. Reported upload byte count is 0` + `No data was uploaded to blob storage. Reported upload byte count is 0.` ) - return { - isSuccess: false - } } return { - isSuccess: true, uploadSize: uploadByteCount, sha256Hash } diff --git a/packages/artifact/src/internal/upload/upload-artifact.ts b/packages/artifact/src/internal/upload/upload-artifact.ts index 6546f98f..55921252 100644 --- a/packages/artifact/src/internal/upload/upload-artifact.ts +++ b/packages/artifact/src/internal/upload/upload-artifact.ts @@ -19,6 +19,7 @@ import { FinalizeArtifactRequest, StringValue } from '../../generated' +import {FilesNotFoundError, InvalidResponseError} from '../shared/errors' export async function uploadArtifact( name: string, @@ -34,10 +35,9 @@ export async function uploadArtifact( rootDirectory ) if (zipSpecification.length === 0) { - core.warning(`No files were found to upload`) - return { - success: false - } + throw new FilesNotFoundError( + zipSpecification.flatMap(s => (s.sourcePath ? [s.sourcePath] : [])) + ) } const zipUploadStream = await createZipUploadStream( @@ -68,10 +68,9 @@ export async function uploadArtifact( const createArtifactResp = await artifactClient.CreateArtifact(createArtifactReq) if (!createArtifactResp.ok) { - core.warning(`Failed to create artifact`) - return { - success: false - } + throw new InvalidResponseError( + 'CreateArtifact: response from backend was not ok' + ) } // Upload zip to blob storage @@ -79,11 +78,6 @@ export async function uploadArtifact( createArtifactResp.signedUploadUrl, zipUploadStream ) - if (uploadResult.isSuccess === false) { - return { - success: false - } - } // finalize the artifact const finalizeArtifactReq: FinalizeArtifactRequest = { @@ -104,10 +98,9 @@ export async function uploadArtifact( const finalizeArtifactResp = await artifactClient.FinalizeArtifact(finalizeArtifactReq) if (!finalizeArtifactResp.ok) { - core.warning(`Failed to finalize artifact`) - return { - success: false - } + throw new InvalidResponseError( + 'FinalizeArtifact: response from backend was not ok' + ) } const artifactId = BigInt(finalizeArtifactResp.artifactId) @@ -116,7 +109,6 @@ export async function uploadArtifact( ) return { - success: true, size: uploadResult.uploadSize, id: Number(artifactId) }