From d3c5f358d11f9b074021398c0c6bc98f6c9bff5f Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Tue, 5 Dec 2023 17:56:18 +0000 Subject: [PATCH] consistent promise behavior for get artifact --- .../artifact/__tests__/get-artifact.test.ts | 34 ++++++++----------- .../src/internal/find/get-artifact.ts | 22 ++++-------- .../artifact/src/internal/shared/errors.ts | 8 +++++ .../src/internal/shared/interfaces.ts | 5 --- 4 files changed, 29 insertions(+), 40 deletions(-) diff --git a/packages/artifact/__tests__/get-artifact.test.ts b/packages/artifact/__tests__/get-artifact.test.ts index 82008a4b..56ed0d34 100644 --- a/packages/artifact/__tests__/get-artifact.test.ts +++ b/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> @@ -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({ - success: false - }) + expect(response).rejects.toThrowError(ArtifactNotFoundError) }) 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({ - success: false - }) + expect(response).rejects.toThrowError(InvalidResponseError) }) }) @@ -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({ - success: false - }) + expect(response).rejects.toThrowError(ArtifactNotFoundError) }) it('should fail if non-200 response', async () => { jest .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts') - .mockRejectedValue(new Error('test error')) + .mockRejectedValue(new Error('boom')) - await expect( - getArtifactInternal(fixtures.artifacts[0].name) - ).rejects.toThrow('test error') + const response = getArtifactInternal(fixtures.artifacts[0].name) + + expect(response).rejects.toThrow() }) }) }) diff --git a/packages/artifact/src/internal/find/get-artifact.ts b/packages/artifact/src/internal/find/get-artifact.ts index 91c85818..45db581f 100644 --- a/packages/artifact/src/internal/find/get-artifact.ts +++ b/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}`) - return { - success: false - } + throw new InvalidResponseError( + `Invalid response from GitHub API: ${getArtifactResp.status} (${getArtifactResp?.headers?.['x-github-request-id']})` + ) } if (getArtifactResp.data.artifacts.length === 0) { - core.warning('no artifacts found') - return { - success: false - } + throw new ArtifactNotFoundError(artifactName) } 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') - return { - success: false - } + throw new ArtifactNotFoundError(artifactName) } 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), diff --git a/packages/artifact/src/internal/shared/errors.ts b/packages/artifact/src/internal/shared/errors.ts index 37b92d14..2ddf79ab 100644 --- a/packages/artifact/src/internal/shared/errors.ts +++ b/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' + } +} diff --git a/packages/artifact/src/internal/shared/interfaces.ts b/packages/artifact/src/internal/shared/interfaces.ts index 654c5981..b72274de 100644 --- a/packages/artifact/src/internal/shared/interfaces.ts +++ b/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 */