consistent promise behavior for get artifact

packages/artifact/__tests__/get-artifact.test.ts

@@ -8,6 +8,10 @@ import * as config from '../src/internal/shared/config'
 import {ArtifactServiceClientJSON, Timestamp} from '../src/generated'
 import * as util from '../src/internal/shared/util'
 import {noopLogs} from './common'
+import {
+  ArtifactNotFoundError,
+  InvalidResponseError
+} from '../src/internal/shared/errors'
 
 type MockedRequest = jest.MockedFunction<RequestInterface<object>>
 
@@ -76,7 +80,6 @@ describe('get-artifact', () => {
       )
 
       expect(response).toEqual({
-        success: true,
         artifact: fixtures.artifacts[0]
       })
     })
@@ -107,7 +110,6 @@ describe('get-artifact', () => {
       )
 
       expect(response).toEqual({
-        success: true,
         artifact: fixtures.artifacts[1]
       })
     })
@@ -124,7 +126,7 @@ describe('get-artifact', () => {
         }
       })
 
-      const response = await getArtifactPublic(
+      const response = getArtifactPublic(
         fixtures.artifacts[0].name,
         fixtures.runId,
         fixtures.owner,
@@ -132,9 +134,7 @@ describe('get-artifact', () => {
         fixtures.token
       )
 
-      expect(response).toEqual({
+      expect(response).rejects.toThrowError(ArtifactNotFoundError)
-        success: false
-      })
     })
 
     it('should fail if non-200 response', async () => {
@@ -147,7 +147,7 @@ describe('get-artifact', () => {
         data: {}
       })
 
-      const response = await getArtifactPublic(
+      const response = getArtifactPublic(
         fixtures.artifacts[0].name,
         fixtures.runId,
         fixtures.owner,
@@ -155,9 +155,7 @@ describe('get-artifact', () => {
         fixtures.token
       )
 
-      expect(response).toEqual({
+      expect(response).rejects.toThrowError(InvalidResponseError)
-        success: false
-      })
     })
   })
 
@@ -192,7 +190,6 @@ describe('get-artifact', () => {
       const response = await getArtifactInternal(fixtures.artifacts[0].name)
 
       expect(response).toEqual({
-        success: true,
         artifact: fixtures.artifacts[0]
       })
     })
@@ -213,7 +210,6 @@ describe('get-artifact', () => {
       const response = await getArtifactInternal(fixtures.artifacts[0].name)
 
       expect(response).toEqual({
-        success: true,
         artifact: fixtures.artifacts[1]
       })
     })
@@ -225,21 +221,19 @@ describe('get-artifact', () => {
           artifacts: []
         })
 
-      const response = await getArtifactInternal(fixtures.artifacts[0].name)
+      const response = getArtifactInternal(fixtures.artifacts[0].name)
 
-      expect(response).toEqual({
+      expect(response).rejects.toThrowError(ArtifactNotFoundError)
-        success: false
-      })
     })
 
     it('should fail if non-200 response', async () => {
       jest
         .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts')
-        .mockRejectedValue(new Error('test error'))
+        .mockRejectedValue(new Error('boom'))
 
-      await expect(
+      const response = getArtifactInternal(fixtures.artifacts[0].name)
-        getArtifactInternal(fixtures.artifacts[0].name)
+
-      ).rejects.toThrow('test error')
+      expect(response).rejects.toThrow()
     })
   })
 })

packages/artifact/src/internal/find/get-artifact.ts

@@ -10,6 +10,7 @@ import {getBackendIdsFromToken} from '../shared/util'
 import {getUserAgentString} from '../shared/user-agent'
 import {internalArtifactTwirpClient} from '../shared/artifact-twirp-client'
 import {ListArtifactsRequest, StringValue, Timestamp} from '../../generated'
+import {ArtifactNotFoundError, InvalidResponseError} from '../shared/errors'
 
 export async function getArtifactPublic(
   artifactName: string,
@@ -41,17 +42,13 @@ export async function getArtifactPublic(
   )
 
   if (getArtifactResp.status !== 200) {
-    core.warning(`non-200 response from GitHub API: ${getArtifactResp.status}`)
+    throw new InvalidResponseError(
-    return {
+      `Invalid response from GitHub API: ${getArtifactResp.status} (${getArtifactResp?.headers?.['x-github-request-id']})`
-      success: false
+    )
-    }
   }
 
   if (getArtifactResp.data.artifacts.length === 0) {
-    core.warning('no artifacts found')
+    throw new ArtifactNotFoundError(artifactName)
-    return {
-      success: false
-    }
   }
 
   let artifact = getArtifactResp.data.artifacts[0]
@@ -63,7 +60,6 @@ export async function getArtifactPublic(
   }
 
   return {
-    success: true,
     artifact: {
       name: artifact.name,
       id: artifact.id,
@@ -90,10 +86,7 @@ export async function getArtifactInternal(
   const res = await artifactClient.ListArtifacts(req)
 
   if (res.artifacts.length === 0) {
-    core.warning('no artifacts found')
+    throw new ArtifactNotFoundError(artifactName)
-    return {
-      success: false
-    }
   }
 
   let artifact = res.artifacts[0]
@@ -103,12 +96,11 @@ export async function getArtifactInternal(
     )[0]
 
     core.debug(
-      `more than one artifact found for a single name, returning newest (id: ${artifact.databaseId})`
+      `More than one artifact found for a single name, returning newest (id: ${artifact.databaseId})`
     )
   }
 
   return {
-    success: true,
     artifact: {
       name: artifact.name,
       id: Number(artifact.databaseId),

packages/artifact/src/internal/shared/errors.ts

@@ -19,3 +19,11 @@ export class InvalidResponseError extends Error {
     this.name = 'InvalidResponseError'
   }
 }
+
+export class ArtifactNotFoundError extends Error {
+  constructor(name: string) {
+    const message = `No artifact found for name: ${name}`
+    super(message)
+    this.name = 'ArtifactNotFoundError'
+  }
+}

packages/artifact/src/internal/shared/interfaces.ts

@@ -53,11 +53,6 @@ export interface UploadArtifactOptions {
  *****************************************************************************/
 
 export interface GetArtifactResponse {
-  /**
-   * If an artifact was found
-   */
-  success: boolean
-
   /**
    * Metadata about the artifact that was found
    */