From c1f9d37323afac4dced1431052013461238d8493 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Sat, 2 Dec 2023 21:18:22 -0500 Subject: [PATCH 1/8] updates to get/list artifacts --- .../src/generated/results/api/v1/artifact.ts | 15 ++++++- .../src/internal/find/get-artifact.ts | 42 +++++++++++-------- .../src/internal/find/list-artifacts.ts | 15 +++++-- .../src/internal/shared/interfaces.ts | 5 +++ 4 files changed, 55 insertions(+), 22 deletions(-) diff --git a/packages/artifact/src/generated/results/api/v1/artifact.ts b/packages/artifact/src/generated/results/api/v1/artifact.ts index adabae5e..acce3037 100644 --- a/packages/artifact/src/generated/results/api/v1/artifact.ts +++ b/packages/artifact/src/generated/results/api/v1/artifact.ts @@ -163,6 +163,12 @@ export interface ListArtifactsResponse_MonolithArtifact { * @generated from protobuf field: int64 size = 5; */ size: string; + /** + * When the artifact was created in the monolith + * + * @generated from protobuf field: google.protobuf.Timestamp created_at = 6; + */ + createdAt?: Timestamp; } /** * @generated from protobuf message github.actions.results.api.v1.GetSignedArtifactURLRequest @@ -571,7 +577,8 @@ class ListArtifactsResponse_MonolithArtifact$Type extends MessageType Timestamp } ]); } create(value?: PartialMessage): ListArtifactsResponse_MonolithArtifact { @@ -601,6 +608,9 @@ class ListArtifactsResponse_MonolithArtifact$Type extends MessageType 1) { - core.warning( - 'more than one artifact found for a single name, returning first' + artifact = getArtifactResp.data.artifacts.reduce((prev, current) => { + new Date(prev.created_at) > new Date(current.created_at) ? prev : current + }) + + core.debug( + `more than one artifact found for a single name, returning newest (id: ${artifact.id})` ) } return { success: true, artifact: { - name: getArtifactResp.data.artifacts[0].name, - id: getArtifactResp.data.artifacts[0].id, - size: getArtifactResp.data.artifacts[0].size_in_bytes + name: artifact.name, + id: artifact.id, + size: artifact.size_in_bytes, + createdAt: artifact.created_at ? new Date(artifact.created_at) : undefined } } } @@ -93,26 +99,28 @@ export async function getArtifactInternal( } } + let artifact = res.artifacts[0] if (res.artifacts.length > 1) { - core.warning( - 'more than one artifact found for a single name, returning first' + artifact = res.artifacts.reduce((prev, current) => { + const prevDate = Timestamp.toDate(prev.createdAt || Timestamp.now()) + const currentDate = Timestamp.toDate(current.createdAt || Timestamp.now()) + return prevDate > currentDate ? prev : current + }) + + core.debug( + `more than one artifact found for a single name, returning newest (id: ${artifact.databaseId})` ) } - // In the case of reruns, we may have artifacts with the same name scoped under the same workflow run. - // Let's prefer the artifact closest scoped to this run. - // If it doesn't exist (e.g. partial rerun) we'll use the first match. - const artifact = - res.artifacts.find( - artifact => artifact.workflowRunBackendId === workflowRunBackendId - ) || res.artifacts[0] - return { success: true, artifact: { name: artifact.name, id: Number(artifact.databaseId), - size: Number(artifact.size) + size: Number(artifact.size), + createdAt: artifact.createdAt + ? Timestamp.toDate(artifact.createdAt) + : undefined } } } diff --git a/packages/artifact/src/internal/find/list-artifacts.ts b/packages/artifact/src/internal/find/list-artifacts.ts index c25a58bd..a951fed1 100644 --- a/packages/artifact/src/internal/find/list-artifacts.ts +++ b/packages/artifact/src/internal/find/list-artifacts.ts @@ -9,7 +9,7 @@ import {retry} from '@octokit/plugin-retry' import {OctokitOptions} from '@octokit/core/dist-types/types' import {internalArtifactTwirpClient} from '../shared/artifact-twirp-client' import {getBackendIdsFromToken} from '../shared/util' -import {ListArtifactsRequest} from 'src/generated' +import {ListArtifactsRequest, Timestamp} from 'src/generated' // Limiting to 1000 for perf reasons const maximumArtifactCount = 1000 @@ -65,7 +65,8 @@ export async function listArtifactsPublic( artifacts.push({ name: artifact.name, id: artifact.id, - size: artifact.size_in_bytes + size: artifact.size_in_bytes, + createdAt: artifact.created_at ? new Date(artifact.created_at) : undefined }) } @@ -91,7 +92,10 @@ export async function listArtifactsPublic( artifacts.push({ name: artifact.name, id: artifact.id, - size: artifact.size_in_bytes + size: artifact.size_in_bytes, + createdAt: artifact.created_at + ? new Date(artifact.created_at) + : undefined }) } } @@ -118,7 +122,10 @@ export async function listArtifactsInternal(): Promise { const artifacts = res.artifacts.map(artifact => ({ name: artifact.name, id: Number(artifact.databaseId), - size: Number(artifact.size) + size: Number(artifact.size), + createdAt: artifact.createdAt + ? Timestamp.toDate(artifact.createdAt) + : undefined })) info(`Found ${artifacts.length} artifact(s)`) diff --git a/packages/artifact/src/internal/shared/interfaces.ts b/packages/artifact/src/internal/shared/interfaces.ts index bd764412..0b0e3768 100644 --- a/packages/artifact/src/internal/shared/interfaces.ts +++ b/packages/artifact/src/internal/shared/interfaces.ts @@ -124,6 +124,11 @@ export interface Artifact { * The size of the artifact in bytes */ size: number + + /** + * The time when the artifact was created + */ + createdAt?: Date } // FindOptions are for fetching Artifact(s) out of the scope of the current run. From fa7657714a70dc1072a1245a72ab012d96262f98 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Sat, 2 Dec 2023 21:34:07 -0500 Subject: [PATCH 2/8] fix import --- packages/artifact/src/internal/find/list-artifacts.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/find/list-artifacts.ts b/packages/artifact/src/internal/find/list-artifacts.ts index a951fed1..e88c676e 100644 --- a/packages/artifact/src/internal/find/list-artifacts.ts +++ b/packages/artifact/src/internal/find/list-artifacts.ts @@ -9,7 +9,7 @@ import {retry} from '@octokit/plugin-retry' import {OctokitOptions} from '@octokit/core/dist-types/types' import {internalArtifactTwirpClient} from '../shared/artifact-twirp-client' import {getBackendIdsFromToken} from '../shared/util' -import {ListArtifactsRequest, Timestamp} from 'src/generated' +import {ListArtifactsRequest, Timestamp} from '../../generated' // Limiting to 1000 for perf reasons const maximumArtifactCount = 1000 From c94ca49c9c6442b56a6fe850a06624b8838eafce Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Sun, 3 Dec 2023 05:01:20 +0000 Subject: [PATCH 3/8] ability to filter artifacts by latest --- packages/artifact/src/internal/client.ts | 14 ++++-- .../src/internal/find/list-artifacts.ts | 50 +++++++++++++++++-- .../src/internal/shared/interfaces.ts | 9 ++++ 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/packages/artifact/src/internal/client.ts b/packages/artifact/src/internal/client.ts index 20563abc..4811104c 100644 --- a/packages/artifact/src/internal/client.ts +++ b/packages/artifact/src/internal/client.ts @@ -5,6 +5,7 @@ import { UploadResponse, DownloadArtifactOptions, GetArtifactResponse, + ListArtifactsOptions, ListArtifactsResponse, DownloadArtifactResponse, FindOptions @@ -44,7 +45,9 @@ export interface ArtifactClient { * @param options Extra options that allow for the customization of the list behavior * @returns ListArtifactResponse object */ - listArtifacts(options?: FindOptions): Promise + listArtifacts( + options?: ListArtifactsOptions & FindOptions + ): Promise /** * Finds an artifact by name. @@ -171,7 +174,9 @@ If the error persists, please check whether Actions and API requests are operati /** * List Artifacts */ - async listArtifacts(options?: FindOptions): Promise { + async listArtifacts( + options?: ListArtifactsOptions & FindOptions + ): Promise { if (isGhes()) { warning( `@actions/artifact v2.0.0+ and download-artifact@v4+ are not currently supported on GHES.` @@ -191,11 +196,12 @@ If the error persists, please check whether Actions and API requests are operati workflowRunId, repositoryOwner, repositoryName, - token + token, + options?.latest ) } - return listArtifactsInternal() + return listArtifactsInternal(options?.latest) } catch (error: unknown) { warning( `Listing Artifacts failed with error: ${error}. diff --git a/packages/artifact/src/internal/find/list-artifacts.ts b/packages/artifact/src/internal/find/list-artifacts.ts index e88c676e..a61ca583 100644 --- a/packages/artifact/src/internal/find/list-artifacts.ts +++ b/packages/artifact/src/internal/find/list-artifacts.ts @@ -20,13 +20,14 @@ export async function listArtifactsPublic( workflowRunId: number, repositoryOwner: string, repositoryName: string, - token: string + token: string, + latest = false ): Promise { info( `Fetching artifact list for workflow run ${workflowRunId} in repository ${repositoryOwner}/${repositoryName}` ) - const artifacts: Artifact[] = [] + let artifacts: Artifact[] = [] const [retryOpts, requestOpts] = getRetryOptions(defaultGitHubOptions) const opts: OctokitOptions = { @@ -100,6 +101,10 @@ export async function listArtifactsPublic( } } + if (latest) { + artifacts = filterLatest(artifacts) + } + info(`Found ${artifacts.length} artifact(s)`) return { @@ -107,7 +112,9 @@ export async function listArtifactsPublic( } } -export async function listArtifactsInternal(): Promise { +export async function listArtifactsInternal( + latest = false +): Promise { const artifactClient = internalArtifactTwirpClient() const {workflowRunBackendId, workflowJobRunBackendId} = @@ -119,7 +126,7 @@ export async function listArtifactsInternal(): Promise { } const res = await artifactClient.ListArtifacts(req) - const artifacts = res.artifacts.map(artifact => ({ + let artifacts: Artifact[] = res.artifacts.map(artifact => ({ name: artifact.name, id: Number(artifact.databaseId), size: Number(artifact.size), @@ -128,9 +135,44 @@ export async function listArtifactsInternal(): Promise { : undefined })) + if (latest) { + artifacts = filterLatest(artifacts) + } + info(`Found ${artifacts.length} artifact(s)`) return { artifacts } } + +/** + * Filters a list of artifacts to only include the latest artifact for each name + * @param artifacts The artifacts to filter + * @returns The filtered list of artifacts + */ +function filterLatest(artifacts: Artifact[]): Artifact[] { + artifacts.sort((a, b) => { + if (!a.createdAt && !b.createdAt) { + return 0 + } + if (!a.createdAt) { + return -1 + } + if (!b.createdAt) { + return 1 + } + return b.createdAt.getTime() - a.createdAt.getTime() + }) + + const latestArtifacts: Artifact[] = [] + const seenArtifactNames = new Set() + for (const artifact of artifacts) { + if (!seenArtifactNames.has(artifact.name)) { + latestArtifacts.push(artifact) + seenArtifactNames.add(artifact.name) + } + } + + return latestArtifacts +} diff --git a/packages/artifact/src/internal/shared/interfaces.ts b/packages/artifact/src/internal/shared/interfaces.ts index 0b0e3768..647e265f 100644 --- a/packages/artifact/src/internal/shared/interfaces.ts +++ b/packages/artifact/src/internal/shared/interfaces.ts @@ -74,6 +74,15 @@ export interface GetArtifactResponse { * ListArtifact * * * *****************************************************************************/ + +export interface ListArtifactsOptions { + /** + * Filter the workflow run's artifacts to the latest by name + * In the case of reruns, this can be useful to avoid duplicates + */ + latest?: boolean +} + export interface ListArtifactsResponse { /** * A list of artifacts that were found From c11a7cdeac6204d06b80cdb3d763af4361cd5791 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Sun, 3 Dec 2023 06:24:49 +0000 Subject: [PATCH 4/8] wip --- .../artifact/__tests__/get-artifact.test.ts | 205 ++++++++++++++++++ packages/artifact/src/internal/client.ts | 14 +- .../src/internal/find/get-artifact.ts | 15 +- .../src/internal/find/list-artifacts.ts | 15 +- .../src/internal/shared/interfaces.ts | 4 +- 5 files changed, 220 insertions(+), 33 deletions(-) create mode 100644 packages/artifact/__tests__/get-artifact.test.ts diff --git a/packages/artifact/__tests__/get-artifact.test.ts b/packages/artifact/__tests__/get-artifact.test.ts new file mode 100644 index 00000000..aa2b5331 --- /dev/null +++ b/packages/artifact/__tests__/get-artifact.test.ts @@ -0,0 +1,205 @@ +import * as github from '@actions/github' +import type {RequestInterface} from '@octokit/types' +import { + getArtifactInternal, + getArtifactPublic +} from '../src/internal/find/get-artifact' +import * as config from '../src/internal/shared/config' +import {ArtifactServiceClientJSON} from '../src/generated' +import * as util from '../src/internal/shared/util' +import {noopLogs} from './common' + +type MockedRequest = jest.MockedFunction> + +jest.mock('@actions/github', () => ({ + getOctokit: jest.fn().mockReturnValue({ + request: jest.fn() + }) +})) + +const fixtures = { + repo: 'toolkit', + owner: 'actions', + token: 'ghp_1234567890', + runId: 123, + backendIds: { + workflowRunBackendId: 'c4d7c21f-ba3f-4ddc-a8c8-6f2f626f8422', + workflowJobRunBackendId: '760803a1-f890-4d25-9a6e-a3fc01a0c7cf' + }, + artifacts: [ + { + id: 1, + name: 'my-artifact', + size: 456, + createdAt: new Date('2023-12-01') + }, + { + id: 2, + name: 'my-artifact', + size: 456, + createdAt: new Date('2023-12-02') + } + ] +} + +describe('get-artifact', () => { + beforeAll(() => { + noopLogs() + }) + + describe('public', () => { + it('should return the artifact if it is found', async () => { + const mockRequest = github.getOctokit(fixtures.token) + .request as MockedRequest + mockRequest.mockResolvedValueOnce({ + status: 200, + headers: {}, + url: '', + data: { + artifacts: [ + { + name: fixtures.artifacts[0].name, + id: fixtures.artifacts[0].id, + size_in_bytes: fixtures.artifacts[0].size, + created_at: fixtures.artifacts[0].createdAt.toISOString() + } + ] + } + }) + + const response = await getArtifactPublic( + fixtures.artifacts[0].name, + fixtures.runId, + fixtures.owner, + fixtures.repo, + fixtures.token + ) + + expect(response).toEqual({ + success: true, + artifact: fixtures.artifacts[0] + }) + }) + + it('should return the latest artifact if multiple are found', async () => { + const mockRequest = github.getOctokit(fixtures.token) + .request as MockedRequest + mockRequest.mockResolvedValueOnce({ + status: 200, + headers: {}, + url: '', + data: { + artifacts: fixtures.artifacts.map(artifact => ({ + name: artifact.name, + id: artifact.id, + size_in_bytes: artifact.size, + created_at: artifact.createdAt.toISOString() + })) + } + }) + + const response = await getArtifactPublic( + fixtures.artifacts[0].name, + fixtures.runId, + fixtures.owner, + fixtures.repo, + fixtures.token + ) + + expect(response).toEqual({ + success: true, + artifact: fixtures.artifacts[1] + }) + }) + + it('should fail if no artifacts are found', async () => { + const mockRequest = github.getOctokit(fixtures.token) + .request as MockedRequest + mockRequest.mockResolvedValueOnce({ + status: 200, + headers: {}, + url: '', + data: { + artifacts: [] + } + }) + + const response = await getArtifactPublic( + fixtures.artifacts[0].name, + fixtures.runId, + fixtures.owner, + fixtures.repo, + fixtures.token + ) + + expect(response).toEqual({ + success: false + }) + }) + + it('should fail if non-200 response', async () => { + const mockRequest = github.getOctokit(fixtures.token) + .request as MockedRequest + mockRequest.mockResolvedValueOnce({ + status: 404, + headers: {}, + url: '', + data: {} + }) + + const response = await getArtifactPublic( + fixtures.artifacts[0].name, + fixtures.runId, + fixtures.owner, + fixtures.repo, + fixtures.token + ) + + expect(response).toEqual({ + success: false + }) + }) + }) + + describe('internal', () => { + beforeEach(() => { + jest.spyOn(config, 'getRuntimeToken').mockReturnValue('test-token') + + jest + .spyOn(util, 'getBackendIdsFromToken') + .mockReturnValue(fixtures.backendIds) + + jest + .spyOn(config, 'getResultsServiceUrl') + .mockReturnValue('https://results.local') + }) + + it('should return the artifact if it is found', async () => { + jest + .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts') + .mockResolvedValue({ + artifacts: [ + { + ...fixtures.backendIds, + databaseId: fixtures.artifacts[0].id.toString(), + name: fixtures.artifacts[0].name, + size: fixtures.artifacts[0].size.toString() + } + ] + }) + + const response = await getArtifactInternal(fixtures.artifacts[0].name) + + expect(response).toEqual({ + success: true, + artifact: fixtures.artifacts[0] + }) + }) + + it('should return the latest artifact if multiple are found', async () => {}) + + it('should fail if no artifacts are found', async () => {}) + + it('should fail if non-200 response', async () => {}) + }) +}) diff --git a/packages/artifact/src/internal/client.ts b/packages/artifact/src/internal/client.ts index 4811104c..97559652 100644 --- a/packages/artifact/src/internal/client.ts +++ b/packages/artifact/src/internal/client.ts @@ -1,8 +1,8 @@ import {warning} from '@actions/core' import {isGhes} from './shared/config' import { - UploadOptions, - UploadResponse, + UploadArtifactOptions, + UploadArtifactResponse, DownloadArtifactOptions, GetArtifactResponse, ListArtifactsOptions, @@ -26,14 +26,14 @@ export interface ArtifactClient { * @param files A list of absolute or relative paths that denote what files should be uploaded * @param rootDirectory An absolute or relative file path that denotes the root parent directory of the files being uploaded * @param options Extra options for customizing the upload behavior - * @returns single UploadResponse object + * @returns single UploadArtifactResponse object */ uploadArtifact( name: string, files: string[], rootDirectory: string, - options?: UploadOptions - ): Promise + options?: UploadArtifactOptions + ): Promise /** * Lists all artifacts that are part of the current workflow run. @@ -96,8 +96,8 @@ export class Client implements ArtifactClient { name: string, files: string[], rootDirectory: string, - options?: UploadOptions - ): Promise { + options?: UploadArtifactOptions + ): Promise { if (isGhes()) { warning( `@actions/artifact v2.0.0+ and upload-artifact@v4+ are not currently supported on GHES.` diff --git a/packages/artifact/src/internal/find/get-artifact.ts b/packages/artifact/src/internal/find/get-artifact.ts index 2bc1dc97..91c85818 100644 --- a/packages/artifact/src/internal/find/get-artifact.ts +++ b/packages/artifact/src/internal/find/get-artifact.ts @@ -56,12 +56,9 @@ export async function getArtifactPublic( let artifact = getArtifactResp.data.artifacts[0] if (getArtifactResp.data.artifacts.length > 1) { - artifact = getArtifactResp.data.artifacts.reduce((prev, current) => { - new Date(prev.created_at) > new Date(current.created_at) ? prev : current - }) - + artifact = getArtifactResp.data.artifacts.sort((a, b) => b.id - a.id)[0] core.debug( - `more than one artifact found for a single name, returning newest (id: ${artifact.id})` + `More than one artifact found for a single name, returning newest (id: ${artifact.id})` ) } @@ -101,11 +98,9 @@ export async function getArtifactInternal( let artifact = res.artifacts[0] if (res.artifacts.length > 1) { - artifact = res.artifacts.reduce((prev, current) => { - const prevDate = Timestamp.toDate(prev.createdAt || Timestamp.now()) - const currentDate = Timestamp.toDate(current.createdAt || Timestamp.now()) - return prevDate > currentDate ? prev : current - }) + artifact = res.artifacts.sort( + (a, b) => Number(b.databaseId) - Number(a.databaseId) + )[0] core.debug( `more than one artifact found for a single name, returning newest (id: ${artifact.databaseId})` diff --git a/packages/artifact/src/internal/find/list-artifacts.ts b/packages/artifact/src/internal/find/list-artifacts.ts index a61ca583..cd83320c 100644 --- a/packages/artifact/src/internal/find/list-artifacts.ts +++ b/packages/artifact/src/internal/find/list-artifacts.ts @@ -152,19 +152,7 @@ export async function listArtifactsInternal( * @returns The filtered list of artifacts */ function filterLatest(artifacts: Artifact[]): Artifact[] { - artifacts.sort((a, b) => { - if (!a.createdAt && !b.createdAt) { - return 0 - } - if (!a.createdAt) { - return -1 - } - if (!b.createdAt) { - return 1 - } - return b.createdAt.getTime() - a.createdAt.getTime() - }) - + artifacts.sort((a, b) => b.id - a.id) const latestArtifacts: Artifact[] = [] const seenArtifactNames = new Set() for (const artifact of artifacts) { @@ -173,6 +161,5 @@ function filterLatest(artifacts: Artifact[]): Artifact[] { seenArtifactNames.add(artifact.name) } } - return latestArtifacts } diff --git a/packages/artifact/src/internal/shared/interfaces.ts b/packages/artifact/src/internal/shared/interfaces.ts index 647e265f..61994b2b 100644 --- a/packages/artifact/src/internal/shared/interfaces.ts +++ b/packages/artifact/src/internal/shared/interfaces.ts @@ -3,7 +3,7 @@ * UploadArtifact * * * *****************************************************************************/ -export interface UploadResponse { +export interface UploadArtifactResponse { /** * Denotes if an artifact was successfully uploaded */ @@ -21,7 +21,7 @@ export interface UploadResponse { id?: number } -export interface UploadOptions { +export interface UploadArtifactOptions { /** * Duration after which artifact will expire in days. * From 86ce0b159a27ce90292da2201a7ce62f8698a842 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Sun, 3 Dec 2023 19:43:37 +0000 Subject: [PATCH 5/8] get artifact tests --- .../artifact/__tests__/get-artifact.test.ts | 50 +++++++++++++++++-- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/packages/artifact/__tests__/get-artifact.test.ts b/packages/artifact/__tests__/get-artifact.test.ts index aa2b5331..82008a4b 100644 --- a/packages/artifact/__tests__/get-artifact.test.ts +++ b/packages/artifact/__tests__/get-artifact.test.ts @@ -5,7 +5,7 @@ import { getArtifactPublic } from '../src/internal/find/get-artifact' import * as config from '../src/internal/shared/config' -import {ArtifactServiceClientJSON} from '../src/generated' +import {ArtifactServiceClientJSON, Timestamp} from '../src/generated' import * as util from '../src/internal/shared/util' import {noopLogs} from './common' @@ -183,7 +183,8 @@ describe('get-artifact', () => { ...fixtures.backendIds, databaseId: fixtures.artifacts[0].id.toString(), name: fixtures.artifacts[0].name, - size: fixtures.artifacts[0].size.toString() + size: fixtures.artifacts[0].size.toString(), + createdAt: Timestamp.fromDate(fixtures.artifacts[0].createdAt) } ] }) @@ -196,10 +197,49 @@ describe('get-artifact', () => { }) }) - it('should return the latest artifact if multiple are found', async () => {}) + it('should return the latest artifact if multiple are found', async () => { + jest + .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts') + .mockResolvedValue({ + artifacts: fixtures.artifacts.map(artifact => ({ + ...fixtures.backendIds, + databaseId: artifact.id.toString(), + name: artifact.name, + size: artifact.size.toString(), + createdAt: Timestamp.fromDate(artifact.createdAt) + })) + }) - it('should fail if no artifacts are found', async () => {}) + const response = await getArtifactInternal(fixtures.artifacts[0].name) - it('should fail if non-200 response', async () => {}) + expect(response).toEqual({ + success: true, + artifact: fixtures.artifacts[1] + }) + }) + + it('should fail if no artifacts are found', async () => { + jest + .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts') + .mockResolvedValue({ + artifacts: [] + }) + + const response = await getArtifactInternal(fixtures.artifacts[0].name) + + expect(response).toEqual({ + success: false + }) + }) + + it('should fail if non-200 response', async () => { + jest + .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts') + .mockRejectedValue(new Error('test error')) + + await expect( + getArtifactInternal(fixtures.artifacts[0].name) + ).rejects.toThrow('test error') + }) }) }) From ef454f0991074c7772587d36e9c3585bf3d4d030 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Sun, 3 Dec 2023 20:48:33 +0000 Subject: [PATCH 6/8] add tests for list-artifacts --- .../artifact/__tests__/list-artifacts.test.ts | 242 ++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 packages/artifact/__tests__/list-artifacts.test.ts diff --git a/packages/artifact/__tests__/list-artifacts.test.ts b/packages/artifact/__tests__/list-artifacts.test.ts new file mode 100644 index 00000000..7c8699e7 --- /dev/null +++ b/packages/artifact/__tests__/list-artifacts.test.ts @@ -0,0 +1,242 @@ +import * as github from '@actions/github' +import type {RestEndpointMethods} from '@octokit/plugin-rest-endpoint-methods/dist-types/generated/method-types' +import type {RestEndpointMethodTypes} from '@octokit/plugin-rest-endpoint-methods/dist-types/generated/parameters-and-response-types' +import { + listArtifactsInternal, + listArtifactsPublic +} from '../src/internal/find/list-artifacts' +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 {Artifact} from '../src/internal/shared/interfaces' + +type MockedListWorkflowRunArtifacts = jest.MockedFunction< + RestEndpointMethods['actions']['listWorkflowRunArtifacts'] +> + +jest.mock('@actions/github', () => ({ + getOctokit: jest.fn().mockReturnValue({ + rest: { + actions: { + listWorkflowRunArtifacts: jest.fn() + } + } + }) +})) + +const artifactsToListResponse = ( + artifacts: Artifact[] +): RestEndpointMethodTypes['actions']['listWorkflowRunArtifacts']['response']['data'] => { + return { + total_count: artifacts.length, + artifacts: artifacts.map(artifact => ({ + name: artifact.name, + id: artifact.id, + size_in_bytes: artifact.size, + created_at: artifact.createdAt?.toISOString() || '', + run_id: fixtures.runId, + // unused fields for tests + url: '', + archive_download_url: '', + expired: false, + expires_at: '', + node_id: '', + run_url: '', + type: '', + updated_at: '' + })) + } +} + +const fixtures = { + repo: 'toolkit', + owner: 'actions', + token: 'ghp_1234567890', + runId: 123, + backendIds: { + workflowRunBackendId: 'c4d7c21f-ba3f-4ddc-a8c8-6f2f626f8422', + workflowJobRunBackendId: '760803a1-f890-4d25-9a6e-a3fc01a0c7cf' + }, + artifacts: [ + { + id: 1, + name: 'my-artifact', + size: 456, + createdAt: new Date('2023-12-01') + }, + { + id: 2, + name: 'my-artifact', + size: 456, + createdAt: new Date('2023-12-02') + } + ] +} + +describe('list-artifact', () => { + beforeAll(() => { + noopLogs() + }) + + describe('public', () => { + it('should return a list of artifacts', async () => { + const mockListArtifacts = github.getOctokit(fixtures.token).rest.actions + .listWorkflowRunArtifacts as MockedListWorkflowRunArtifacts + + mockListArtifacts.mockResolvedValueOnce({ + status: 200, + headers: {}, + url: '', + data: artifactsToListResponse(fixtures.artifacts) + }) + + const response = await listArtifactsPublic( + fixtures.runId, + fixtures.owner, + fixtures.repo, + fixtures.token, + false + ) + + expect(response).toEqual({ + artifacts: fixtures.artifacts + }) + }) + + it('should return the latest artifact when latest is specified', async () => { + const mockListArtifacts = github.getOctokit(fixtures.token).rest.actions + .listWorkflowRunArtifacts as MockedListWorkflowRunArtifacts + + mockListArtifacts.mockResolvedValueOnce({ + status: 200, + headers: {}, + url: '', + data: artifactsToListResponse(fixtures.artifacts) + }) + + const response = await listArtifactsPublic( + fixtures.runId, + fixtures.owner, + fixtures.repo, + fixtures.token, + true + ) + + expect(response).toEqual({ + artifacts: [fixtures.artifacts[1]] + }) + }) + + it('can return empty artifacts', async () => { + const mockListArtifacts = github.getOctokit(fixtures.token).rest.actions + .listWorkflowRunArtifacts as MockedListWorkflowRunArtifacts + + mockListArtifacts.mockResolvedValueOnce({ + status: 200, + headers: {}, + url: '', + data: { + total_count: 0, + artifacts: [] + } + }) + + const response = await listArtifactsPublic( + fixtures.runId, + fixtures.owner, + fixtures.repo, + fixtures.token, + true + ) + + expect(response).toEqual({ + artifacts: [] + }) + }) + + it('should fail if non-200 response', async () => { + const mockListArtifacts = github.getOctokit(fixtures.token).rest.actions + .listWorkflowRunArtifacts as MockedListWorkflowRunArtifacts + + mockListArtifacts.mockRejectedValue(new Error('boom')) + + await expect( + listArtifactsPublic( + fixtures.runId, + fixtures.owner, + fixtures.repo, + fixtures.token, + false + ) + ).rejects.toThrow('boom') + }) + }) + + describe('internal', () => { + beforeEach(() => { + jest.spyOn(config, 'getRuntimeToken').mockReturnValue('test-token') + jest + .spyOn(util, 'getBackendIdsFromToken') + .mockReturnValue(fixtures.backendIds) + jest + .spyOn(config, 'getResultsServiceUrl') + .mockReturnValue('https://results.local') + }) + + it('should return a list of artifacts', async () => { + jest + .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts') + .mockResolvedValue({ + artifacts: fixtures.artifacts.map(artifact => ({ + ...fixtures.backendIds, + databaseId: artifact.id.toString(), + name: artifact.name, + size: artifact.size.toString(), + createdAt: Timestamp.fromDate(artifact.createdAt) + })) + }) + const response = await listArtifactsInternal(false) + expect(response).toEqual({ + artifacts: fixtures.artifacts + }) + }) + + it('should return the latest artifact when latest is specified', async () => { + jest + .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts') + .mockResolvedValue({ + artifacts: fixtures.artifacts.map(artifact => ({ + ...fixtures.backendIds, + databaseId: artifact.id.toString(), + name: artifact.name, + size: artifact.size.toString(), + createdAt: Timestamp.fromDate(artifact.createdAt) + })) + }) + const response = await listArtifactsInternal(true) + expect(response).toEqual({ + artifacts: [fixtures.artifacts[1]] + }) + }) + + it('can return empty artifacts', async () => { + jest + .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts') + .mockResolvedValue({ + artifacts: [] + }) + const response = await listArtifactsInternal(false) + expect(response).toEqual({ + artifacts: [] + }) + }) + + it('should fail if non-200 response', async () => { + jest + .spyOn(ArtifactServiceClientJSON.prototype, 'ListArtifacts') + .mockRejectedValue(new Error('boom')) + await expect(listArtifactsInternal(false)).rejects.toThrow('boom') + }) + }) +}) From 790e6f71945483a87e9d56e50ff7f24686304886 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Sun, 3 Dec 2023 20:52:36 +0000 Subject: [PATCH 7/8] more docs --- packages/artifact/src/internal/client.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/artifact/src/internal/client.ts b/packages/artifact/src/internal/client.ts index 97559652..45ba0bfe 100644 --- a/packages/artifact/src/internal/client.ts +++ b/packages/artifact/src/internal/client.ts @@ -51,6 +51,7 @@ export interface ArtifactClient { /** * Finds an artifact by name. + * If there are multiple artifacts with the same name in the same workflow run, this will return the latest. * * If options.findBy is specified, this will use the public List Artifacts API with a name filter which can get artifacts from other runs. * https://docs.github.com/en/rest/actions/artifacts?apiVersion=2022-11-28#list-workflow-run-artifacts From 141b3509e4095f289a05a12bdf8a02ba4146f8b2 Mon Sep 17 00:00:00 2001 From: Rob Herley Date: Sun, 3 Dec 2023 21:13:55 +0000 Subject: [PATCH 8/8] update import --- packages/artifact/src/internal/upload/upload-artifact.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/artifact/src/internal/upload/upload-artifact.ts b/packages/artifact/src/internal/upload/upload-artifact.ts index 011dc4c8..6546f98f 100644 --- a/packages/artifact/src/internal/upload/upload-artifact.ts +++ b/packages/artifact/src/internal/upload/upload-artifact.ts @@ -1,5 +1,8 @@ import * as core from '@actions/core' -import {UploadOptions, UploadResponse} from '../shared/interfaces' +import { + UploadArtifactOptions, + UploadArtifactResponse +} from '../shared/interfaces' import {getExpiration} from './retention' import {validateArtifactName} from './path-and-artifact-name-validation' import {internalArtifactTwirpClient} from '../shared/artifact-twirp-client' @@ -21,8 +24,8 @@ export async function uploadArtifact( name: string, files: string[], rootDirectory: string, - options?: UploadOptions | undefined -): Promise { + options?: UploadArtifactOptions | undefined +): Promise { validateArtifactName(name) validateRootDirectory(rootDirectory)