consistent promise behavior for upload artifact

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()
   })
 })

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 {

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'
+  }
+}

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
    */

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,7 +50,6 @@ 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')
 
   await blockBlobClient.uploadStream(
@@ -70,26 +64,14 @@ export async function uploadZipToBlobStorage(
   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
-    }
-  }
 
   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
   }

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)
   }