From ac35f995fea643cbfd583dc9c9ace8da5c2f453a Mon Sep 17 00:00:00 2001 From: Grant Birkinbine Date: Thu, 17 Apr 2025 04:47:03 +0000 Subject: [PATCH 1/8] implement new `artifact-ids` input --- README.md | 32 ++++++++++++++++++++ __tests__/download.test.ts | 2 +- action.yml | 3 ++ dist/index.js | 45 +++++++++++++++++++++++++-- docs/MIGRATION.md | 36 ++++++++++++++++++++++ src/constants.ts | 3 +- src/download-artifact.ts | 62 ++++++++++++++++++++++++++++++++++++-- 7 files changed, 177 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 6ed23d8..aa71e83 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,7 @@ See also [upload-artifact](https://github.com/actions/upload-artifact). - [Outputs](#outputs) - [Examples](#examples) - [Download Single Artifact](#download-single-artifact) + - [Download Artifacts by ID](#download-artifacts-by-id) - [Download All Artifacts](#download-all-artifacts) - [Download multiple (filtered) Artifacts to the same directory](#download-multiple-filtered-artifacts-to-the-same-directory) - [Download Artifacts from other Workflow Runs or Repositories](#download-artifacts-from-other-workflow-runs-or-repositories) @@ -53,6 +54,11 @@ For assistance with breaking changes, see [MIGRATION.md](docs/MIGRATION.md). # Optional. name: + # IDs of the artifacts to download, comma-separated. + # Either inputs `artifact-ids` or `name` can be used, but not both. + # Optional. + artifact-ids: + # Destination path. Supports basic tilde expansion. # Optional. Default is $GITHUB_WORKSPACE path: @@ -117,6 +123,32 @@ steps: run: ls -R your/destination/dir ``` +### Download Artifacts by ID + +The `artifact-ids` input allows downloading artifacts using their unique ID rather than name. This is particularly useful when working with immutable artifacts from `actions/upload-artifact@v4` which assigns a unique ID to each artifact. + +```yaml +steps: +- uses: actions/download-artifact@v4 + with: + artifact-ids: 12345 +- name: Display structure of downloaded files + run: ls -R +``` + +Multiple artifacts can be downloaded by providing a comma-separated list of IDs: + +```yaml +steps: +- uses: actions/download-artifact@v4 + with: + artifact-ids: 12345,67890 + path: path/to/artifacts +- name: Display structure of downloaded files + run: ls -R path/to/artifacts +``` + +This will download multiple artifacts to separate directories (similar to downloading multiple artifacts by name). ### Download All Artifacts diff --git a/__tests__/download.test.ts b/__tests__/download.test.ts index ac79f4e..09b4573 100644 --- a/__tests__/download.test.ts +++ b/__tests__/download.test.ts @@ -112,7 +112,7 @@ describe('download', () => { await run() expect(core.info).toHaveBeenCalledWith( - 'No input name or pattern filtered specified, downloading all artifacts' + 'No input name, artifact-ids or pattern filtered specified, downloading all artifacts' ) expect(core.info).toHaveBeenCalledWith('Total of 2 artifact(s) downloaded') diff --git a/action.yml b/action.yml index 54a3eb6..7fc4fb5 100644 --- a/action.yml +++ b/action.yml @@ -5,6 +5,9 @@ inputs: name: description: 'Name of the artifact to download. If unspecified, all artifacts for the run are downloaded.' required: false + artifact-ids: + description: 'IDs of the artifacts to download, comma-separated. Either inputs `artifact-ids` or `name` can be used, but not both.' + required: false path: description: 'Destination path. Supports basic tilde expansion. Defaults to $GITHUB_WORKSPACE' required: false diff --git a/dist/index.js b/dist/index.js index d0a4ebc..4ce8c85 100644 --- a/dist/index.js +++ b/dist/index.js @@ -118710,6 +118710,7 @@ var Inputs; Inputs["RunID"] = "run-id"; Inputs["Pattern"] = "pattern"; Inputs["MergeMultiple"] = "merge-multiple"; + Inputs["ArtifactIds"] = "artifact-ids"; })(Inputs || (exports.Inputs = Inputs = {})); var Outputs; (function (Outputs) { @@ -118783,7 +118784,10 @@ function run() { repository: core.getInput(constants_1.Inputs.Repository, { required: false }), runID: parseInt(core.getInput(constants_1.Inputs.RunID, { required: false })), pattern: core.getInput(constants_1.Inputs.Pattern, { required: false }), - mergeMultiple: core.getBooleanInput(constants_1.Inputs.MergeMultiple, { required: false }) + mergeMultiple: core.getBooleanInput(constants_1.Inputs.MergeMultiple, { + required: false + }), + artifactIds: core.getInput(constants_1.Inputs.ArtifactIds, { required: false }) }; if (!inputs.path) { inputs.path = process.env['GITHUB_WORKSPACE'] || process.cwd(); @@ -118791,7 +118795,12 @@ function run() { if (inputs.path.startsWith(`~`)) { inputs.path = inputs.path.replace('~', os.homedir()); } + // Check for mutually exclusive inputs + if (inputs.name && inputs.artifactIds) { + throw new Error(`Inputs 'name' and 'artifact-ids' cannot be used together. Please specify only one.`); + } const isSingleArtifactDownload = !!inputs.name; + const isDownloadByIds = !!inputs.artifactIds; const resolvedPath = path.resolve(inputs.path); core.debug(`Resolved path is ${resolvedPath}`); const options = {}; @@ -118808,6 +118817,7 @@ function run() { }; } let artifacts = []; + let artifactIds = []; if (isSingleArtifactDownload) { core.info(`Downloading single artifact`); const { artifact: targetArtifact } = yield artifact_1.default.getArtifact(inputs.name, options); @@ -118817,6 +118827,37 @@ function run() { core.debug(`Found named artifact '${inputs.name}' (ID: ${targetArtifact.id}, Size: ${targetArtifact.size})`); artifacts = [targetArtifact]; } + else if (isDownloadByIds) { + core.info(`Downloading artifacts by ID`); + const artifactIdList = inputs.artifactIds + .split(',') + .map(id => id.trim()) + .filter(id => id !== ''); + if (artifactIdList.length === 0) { + throw new Error(`No valid artifact IDs provided in 'artifact-ids' input`); + } + core.debug(`Parsed artifact IDs: ${JSON.stringify(artifactIdList)}`); + // Parse the artifact IDs + artifactIds = artifactIdList.map(id => { + const numericId = parseInt(id); + if (isNaN(numericId)) { + throw new Error(`Invalid artifact ID: '${id}'. Must be a number.`); + } + return numericId; + }); + // We need to fetch all artifacts to get metadata for the specified IDs + const listArtifactResponse = yield artifact_1.default.listArtifacts(Object.assign({ latest: true }, options)); + artifacts = listArtifactResponse.artifacts.filter(artifact => artifactIds.includes(artifact.id)); + if (artifacts.length === 0) { + throw new Error(`None of the provided artifact IDs were found`); + } + if (artifacts.length < artifactIds.length) { + const foundIds = artifacts.map(a => a.id); + const missingIds = artifactIds.filter(id => !foundIds.includes(id)); + core.warning(`Could not find the following artifact IDs: ${missingIds.join(', ')}`); + } + core.debug(`Found ${artifacts.length} artifacts by ID`); + } else { const listArtifactResponse = yield artifact_1.default.listArtifacts(Object.assign({ latest: true }, options)); artifacts = listArtifactResponse.artifacts; @@ -118828,7 +118869,7 @@ function run() { core.debug(`Filtered from ${listArtifactResponse.artifacts.length} to ${artifacts.length} artifacts`); } else { - core.info('No input name or pattern filtered specified, downloading all artifacts'); + core.info('No input name, artifact-ids or pattern filtered specified, downloading all artifacts'); if (!inputs.mergeMultiple) { core.info('An extra directory with the artifact name will be created for each download'); } diff --git a/docs/MIGRATION.md b/docs/MIGRATION.md index d56279e..fd1781d 100644 --- a/docs/MIGRATION.md +++ b/docs/MIGRATION.md @@ -4,6 +4,7 @@ - [Multiple uploads to the same named Artifact](#multiple-uploads-to-the-same-named-artifact) - [Overwriting an Artifact](#overwriting-an-artifact) - [Merging multiple artifacts](#merging-multiple-artifacts) + - [Working with Immutable Artifacts](#working-with-immutable-artifacts) Several behavioral differences exist between Artifact actions `v3` and below vs `v4`. This document outlines common scenarios in `v3`, and how they would be handled in `v4`. @@ -207,3 +208,38 @@ jobs: ``` Note that this will download all artifacts to a temporary directory and reupload them as a single artifact. For more information on inputs and other use cases for `actions/upload-artifact/merge@v4`, see [the action documentation](https://github.com/actions/upload-artifact/blob/main/merge/README.md). + +## Working with Immutable Artifacts + +In `v4`, artifacts are immutable by default and each artifact gets a unique ID when uploaded. When an artifact with the same name is uploaded again (with or without `overwrite: true`), it gets a new artifact ID. + +To take advantage of this immutability for security purposes (to avoid potential TOCTOU issues where an artifact might be replaced between upload and download), the new `artifact-ids` input allows you to download artifacts by their specific ID rather than by name: + +```yaml +jobs: + upload: + runs-on: ubuntu-latest + steps: + - name: Create a file + run: echo "hello world" > my-file.txt + - name: Upload Artifact + id: upload + uses: actions/upload-artifact@v4 + with: + name: my-artifact + path: my-file.txt + # The upload step outputs the artifact ID + - name: Print Artifact ID + run: echo "Artifact ID is ${{ steps.upload.outputs.artifact-id }}" + download: + needs: upload + runs-on: ubuntu-latest + steps: + - name: Download Artifact by ID + uses: actions/download-artifact@v4 + with: + # Use the artifact ID directly, not the name, to ensure you get exactly the artifact you expect + artifact-ids: ${{ needs.upload.outputs.artifact-id }} +``` + +This approach provides stronger guarantees about which artifact version you're downloading compared to using just the artifact name. diff --git a/src/constants.ts b/src/constants.ts index 17c7d34..b58f4e6 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -5,7 +5,8 @@ export enum Inputs { Repository = 'repository', RunID = 'run-id', Pattern = 'pattern', - MergeMultiple = 'merge-multiple' + MergeMultiple = 'merge-multiple', + ArtifactIds = 'artifact-ids' } export enum Outputs { diff --git a/src/download-artifact.ts b/src/download-artifact.ts index 1beef4d..cbbf099 100644 --- a/src/download-artifact.ts +++ b/src/download-artifact.ts @@ -23,7 +23,10 @@ export async function run(): Promise { repository: core.getInput(Inputs.Repository, {required: false}), runID: parseInt(core.getInput(Inputs.RunID, {required: false})), pattern: core.getInput(Inputs.Pattern, {required: false}), - mergeMultiple: core.getBooleanInput(Inputs.MergeMultiple, {required: false}) + mergeMultiple: core.getBooleanInput(Inputs.MergeMultiple, { + required: false + }), + artifactIds: core.getInput(Inputs.ArtifactIds, {required: false}) } if (!inputs.path) { @@ -34,7 +37,15 @@ export async function run(): Promise { inputs.path = inputs.path.replace('~', os.homedir()) } + // Check for mutually exclusive inputs + if (inputs.name && inputs.artifactIds) { + throw new Error( + `Inputs 'name' and 'artifact-ids' cannot be used together. Please specify only one.` + ) + } + const isSingleArtifactDownload = !!inputs.name + const isDownloadByIds = !!inputs.artifactIds const resolvedPath = path.resolve(inputs.path) core.debug(`Resolved path is ${resolvedPath}`) @@ -56,6 +67,7 @@ export async function run(): Promise { } let artifacts: Artifact[] = [] + let artifactIds: number[] = [] if (isSingleArtifactDownload) { core.info(`Downloading single artifact`) @@ -74,6 +86,52 @@ export async function run(): Promise { ) artifacts = [targetArtifact] + } else if (isDownloadByIds) { + core.info(`Downloading artifacts by ID`) + + const artifactIdList = inputs.artifactIds + .split(',') + .map(id => id.trim()) + .filter(id => id !== '') + + if (artifactIdList.length === 0) { + throw new Error(`No valid artifact IDs provided in 'artifact-ids' input`) + } + + core.debug(`Parsed artifact IDs: ${JSON.stringify(artifactIdList)}`) + + // Parse the artifact IDs + artifactIds = artifactIdList.map(id => { + const numericId = parseInt(id) + if (isNaN(numericId)) { + throw new Error(`Invalid artifact ID: '${id}'. Must be a number.`) + } + return numericId + }) + + // We need to fetch all artifacts to get metadata for the specified IDs + const listArtifactResponse = await artifactClient.listArtifacts({ + latest: true, + ...options + }) + + artifacts = listArtifactResponse.artifacts.filter(artifact => + artifactIds.includes(artifact.id) + ) + + if (artifacts.length === 0) { + throw new Error(`None of the provided artifact IDs were found`) + } + + if (artifacts.length < artifactIds.length) { + const foundIds = artifacts.map(a => a.id) + const missingIds = artifactIds.filter(id => !foundIds.includes(id)) + core.warning( + `Could not find the following artifact IDs: ${missingIds.join(', ')}` + ) + } + + core.debug(`Found ${artifacts.length} artifacts by ID`) } else { const listArtifactResponse = await artifactClient.listArtifacts({ latest: true, @@ -92,7 +150,7 @@ export async function run(): Promise { ) } else { core.info( - 'No input name or pattern filtered specified, downloading all artifacts' + 'No input name, artifact-ids or pattern filtered specified, downloading all artifacts' ) if (!inputs.mergeMultiple) { core.info( From 42aef06f22940dcf3f2d3a655f07d553905547a4 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Thu, 17 Apr 2025 09:55:13 -0700 Subject: [PATCH 2/8] apply https://github.com/actions/download-artifact/pull/401#discussion_r2048225048 suggestion --- src/download-artifact.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/download-artifact.ts b/src/download-artifact.ts index cbbf099..5cc6b17 100644 --- a/src/download-artifact.ts +++ b/src/download-artifact.ts @@ -102,7 +102,7 @@ export async function run(): Promise { // Parse the artifact IDs artifactIds = artifactIdList.map(id => { - const numericId = parseInt(id) + const numericId = parseInt(id, 10) if (isNaN(numericId)) { throw new Error(`Invalid artifact ID: '${id}'. Must be a number.`) } From ec378bcca14b8289ac31be4e58f96edad70bd770 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Thu, 17 Apr 2025 11:48:44 -0700 Subject: [PATCH 3/8] when only one artifact-id is given, use `getArtifact` and check the resulting id returned --- __tests__/download.test.ts | 162 +++++++++++++++++++++++++++++++++++++ src/download-artifact.ts | 45 +++++++++-- 2 files changed, 199 insertions(+), 8 deletions(-) diff --git a/__tests__/download.test.ts b/__tests__/download.test.ts index 09b4573..888c4e7 100644 --- a/__tests__/download.test.ts +++ b/__tests__/download.test.ts @@ -25,6 +25,8 @@ const mockInputs = (overrides?: Partial<{[K in Inputs]?: any}>) => { [Inputs.Repository]: 'owner/some-repository', [Inputs.RunID]: 'some-run-id', [Inputs.Pattern]: 'some-pattern', + [Inputs.MergeMultiple]: false, + [Inputs.ArtifactIds]: '', ...overrides } @@ -221,4 +223,164 @@ describe('download', () => { expect.stringContaining('digest validation failed') ) }) + + test('throws error when both name and artifact-ids are provided', async () => { + mockInputs({ + [Inputs.Name]: 'artifact-name', + [Inputs.ArtifactIds]: '123' + }) + + await expect(run()).rejects.toThrow( + "Inputs 'name' and 'artifact-ids' cannot be used together. Please specify only one." + ) + }) + + test('throws error when artifact-ids is empty', async () => { + mockInputs({ + [Inputs.Name]: '', + [Inputs.ArtifactIds]: ' , ' + }) + + await expect(run()).rejects.toThrow( + "No valid artifact IDs provided in 'artifact-ids' input" + ) + }) + + test('throws error when artifact-id is not a number', async () => { + mockInputs({ + [Inputs.Name]: '', + [Inputs.ArtifactIds]: '123,abc,456' + }) + + await expect(run()).rejects.toThrow( + "Invalid artifact ID: 'abc'. Must be a number." + ) + }) + + test('downloads a single artifact by ID', async () => { + const mockArtifact = { + id: 123, + name: 'artifact-by-id', + size: 1024, + digest: 'def456' + } + + mockInputs({ + [Inputs.Name]: '', + [Inputs.ArtifactIds]: '123' + }) + + jest + .spyOn(artifact, 'getArtifact') + .mockImplementation(() => Promise.resolve({artifact: mockArtifact})) + + await run() + + expect(core.debug).toHaveBeenCalledWith( + 'Only one artifact ID provided. Fetching latest artifact by its name and checking the ID' + ) + expect(artifact.getArtifact).toHaveBeenCalled() + expect(artifact.downloadArtifact).toHaveBeenCalledWith( + mockArtifact.id, + expect.objectContaining({ + expectedHash: mockArtifact.digest + }) + ) + expect(artifact.listArtifacts).not.toHaveBeenCalled() + expect(core.info).toHaveBeenCalledWith('Total of 1 artifact(s) downloaded') + }) + + test('throws error when single artifact ID is not found', async () => { + mockInputs({ + [Inputs.Name]: '', + [Inputs.ArtifactIds]: '999' + }) + + jest.spyOn(artifact, 'getArtifact').mockImplementation(() => { + return Promise.resolve({artifact: null} as any) + }) + + await expect(run()).rejects.toThrow( + "Artifact with ID '999' not found. Please check the ID." + ) + }) + + test('downloads multiple artifacts by IDs', async () => { + const mockArtifacts = [ + {id: 123, name: 'artifact1', size: 1024, digest: 'abc123'}, + {id: 456, name: 'artifact2', size: 2048, digest: 'def456'}, + {id: 789, name: 'artifact3', size: 3072, digest: 'ghi789'} + ] + + mockInputs({ + [Inputs.Name]: '', + [Inputs.ArtifactIds]: '123, 456' + }) + + jest + .spyOn(artifact, 'listArtifacts') + .mockImplementation(() => Promise.resolve({artifacts: mockArtifacts})) + + await run() + + expect(core.info).toHaveBeenCalledWith( + 'Multiple artifact IDs provided. Fetching all artifacts to filter by ID' + ) + expect(artifact.getArtifact).not.toHaveBeenCalled() + expect(artifact.listArtifacts).toHaveBeenCalled() + expect(artifact.downloadArtifact).toHaveBeenCalledTimes(2) + expect(artifact.downloadArtifact).toHaveBeenCalledWith( + 123, + expect.anything() + ) + expect(artifact.downloadArtifact).toHaveBeenCalledWith( + 456, + expect.anything() + ) + }) + + test('warns when some artifact IDs are not found', async () => { + const mockArtifacts = [ + {id: 123, name: 'artifact1', size: 1024, digest: 'abc123'} + ] + + mockInputs({ + [Inputs.Name]: '', + [Inputs.ArtifactIds]: '123, 456, 789' + }) + + jest + .spyOn(artifact, 'listArtifacts') + .mockImplementation(() => Promise.resolve({artifacts: mockArtifacts})) + + await run() + + expect(core.warning).toHaveBeenCalledWith( + 'Could not find the following artifact IDs: 456, 789' + ) + expect(artifact.downloadArtifact).toHaveBeenCalledTimes(1) + expect(artifact.downloadArtifact).toHaveBeenCalledWith( + 123, + expect.anything() + ) + }) + + test('throws error when none of the provided artifact IDs are found', async () => { + const mockArtifacts = [ + {id: 999, name: 'other-artifact', size: 1024, digest: 'xyz999'} + ] + + mockInputs({ + [Inputs.Name]: '', + [Inputs.ArtifactIds]: '123, 456' + }) + + jest + .spyOn(artifact, 'listArtifacts') + .mockImplementation(() => Promise.resolve({artifacts: mockArtifacts})) + + await expect(run()).rejects.toThrow( + 'None of the provided artifact IDs were found' + ) + }) }) diff --git a/src/download-artifact.ts b/src/download-artifact.ts index 5cc6b17..693a17a 100644 --- a/src/download-artifact.ts +++ b/src/download-artifact.ts @@ -109,15 +109,44 @@ export async function run(): Promise { return numericId }) - // We need to fetch all artifacts to get metadata for the specified IDs - const listArtifactResponse = await artifactClient.listArtifacts({ - latest: true, - ...options - }) + // if the length of artifactIds exactly 1 fetch the latest artifact by its name and check the ID + if (artifactIds.length === 1) { + core.debug( + `Only one artifact ID provided. Fetching latest artifact by its name and checking the ID` + ) + const getArtifactResponse = await artifactClient.getArtifact( + inputs.name, + {...options} + ) - artifacts = listArtifactResponse.artifacts.filter(artifact => - artifactIds.includes(artifact.id) - ) + if (!getArtifactResponse || !getArtifactResponse.artifact) { + throw new Error( + `Artifact with ID '${artifactIds[0]}' not found. Please check the ID.` + ) + } + + const artifact = getArtifactResponse.artifact + + core.debug( + `Found artifact by ID '${artifact.name}' (ID: ${artifact.id}, Size: ${artifact.size})` + ) + + artifacts = [artifact] + } else { + core.info( + `Multiple artifact IDs provided. Fetching all artifacts to filter by ID` + ) + + // We need to fetch all artifacts to get metadata for the specified IDs + const listArtifactResponse = await artifactClient.listArtifacts({ + latest: true, + ...options + }) + + artifacts = listArtifactResponse.artifacts.filter(artifact => + artifactIds.includes(artifact.id) + ) + } if (artifacts.length === 0) { throw new Error(`None of the provided artifact IDs were found`) From e463631f66dff8341f3a8188088081c3e4bff512 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Thu, 17 Apr 2025 11:49:32 -0700 Subject: [PATCH 4/8] bundle --- dist/index.js | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/dist/index.js b/dist/index.js index 4ce8c85..3a8adea 100644 --- a/dist/index.js +++ b/dist/index.js @@ -118839,15 +118839,29 @@ function run() { core.debug(`Parsed artifact IDs: ${JSON.stringify(artifactIdList)}`); // Parse the artifact IDs artifactIds = artifactIdList.map(id => { - const numericId = parseInt(id); + const numericId = parseInt(id, 10); if (isNaN(numericId)) { throw new Error(`Invalid artifact ID: '${id}'. Must be a number.`); } return numericId; }); - // We need to fetch all artifacts to get metadata for the specified IDs - const listArtifactResponse = yield artifact_1.default.listArtifacts(Object.assign({ latest: true }, options)); - artifacts = listArtifactResponse.artifacts.filter(artifact => artifactIds.includes(artifact.id)); + // if the length of artifactIds exactly 1 fetch the latest artifact by its name and check the ID + if (artifactIds.length === 1) { + core.debug(`Only one artifact ID provided. Fetching latest artifact by its name and checking the ID`); + const getArtifactResponse = yield artifact_1.default.getArtifact(inputs.name, Object.assign({}, options)); + if (!getArtifactResponse || !getArtifactResponse.artifact) { + throw new Error(`Artifact with ID '${artifactIds[0]}' not found. Please check the ID.`); + } + const artifact = getArtifactResponse.artifact; + core.debug(`Found artifact by ID '${artifact.name}' (ID: ${artifact.id}, Size: ${artifact.size})`); + artifacts = [artifact]; + } + else { + core.info(`Multiple artifact IDs provided. Fetching all artifacts to filter by ID`); + // We need to fetch all artifacts to get metadata for the specified IDs + const listArtifactResponse = yield artifact_1.default.listArtifacts(Object.assign({ latest: true }, options)); + artifacts = listArtifactResponse.artifacts.filter(artifact => artifactIds.includes(artifact.id)); + } if (artifacts.length === 0) { throw new Error(`None of the provided artifact IDs were found`); } @@ -128958,4 +128972,4 @@ module.exports = JSON.parse('[[[0,44],"disallowed_STD3_valid"],[[45,46],"valid"] /******/ module.exports = __webpack_exports__; /******/ /******/ })() -; \ No newline at end of file +; From 171183c7dce98c3cf8a1fc842429d0a38ed21d33 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Thu, 17 Apr 2025 12:18:37 -0700 Subject: [PATCH 5/8] use the same `artifactClient.getArtifact` structure as seen above in `isSingleArtifactDownload` logic --- src/download-artifact.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/download-artifact.ts b/src/download-artifact.ts index 693a17a..5414f10 100644 --- a/src/download-artifact.ts +++ b/src/download-artifact.ts @@ -114,24 +114,22 @@ export async function run(): Promise { core.debug( `Only one artifact ID provided. Fetching latest artifact by its name and checking the ID` ) - const getArtifactResponse = await artifactClient.getArtifact( + const {artifact: targetArtifact} = await artifactClient.getArtifact( inputs.name, - {...options} + options ) - if (!getArtifactResponse || !getArtifactResponse.artifact) { + if (!targetArtifact) { throw new Error( `Artifact with ID '${artifactIds[0]}' not found. Please check the ID.` ) } - const artifact = getArtifactResponse.artifact - core.debug( - `Found artifact by ID '${artifact.name}' (ID: ${artifact.id}, Size: ${artifact.size})` + `Found artifact by ID '${targetArtifact.name}' (ID: ${targetArtifact.id}, Size: ${targetArtifact.size})` ) - artifacts = [artifact] + artifacts = [targetArtifact] } else { core.info( `Multiple artifact IDs provided. Fetching all artifacts to filter by ID` From b83057b90d3e218abf5c7b1906579eb6c598ae85 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Thu, 17 Apr 2025 12:20:46 -0700 Subject: [PATCH 6/8] bundle --- dist/index.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/dist/index.js b/dist/index.js index 3a8adea..61783a4 100644 --- a/dist/index.js +++ b/dist/index.js @@ -118848,13 +118848,12 @@ function run() { // if the length of artifactIds exactly 1 fetch the latest artifact by its name and check the ID if (artifactIds.length === 1) { core.debug(`Only one artifact ID provided. Fetching latest artifact by its name and checking the ID`); - const getArtifactResponse = yield artifact_1.default.getArtifact(inputs.name, Object.assign({}, options)); - if (!getArtifactResponse || !getArtifactResponse.artifact) { + const { artifact: targetArtifact } = yield artifact_1.default.getArtifact(inputs.name, options); + if (!targetArtifact) { throw new Error(`Artifact with ID '${artifactIds[0]}' not found. Please check the ID.`); } - const artifact = getArtifactResponse.artifact; - core.debug(`Found artifact by ID '${artifact.name}' (ID: ${artifact.id}, Size: ${artifact.size})`); - artifacts = [artifact]; + core.debug(`Found artifact by ID '${targetArtifact.name}' (ID: ${targetArtifact.id}, Size: ${targetArtifact.size})`); + artifacts = [targetArtifact]; } else { core.info(`Multiple artifact IDs provided. Fetching all artifacts to filter by ID`); From 54124fbd881f8ce794405a06896c93c49c17463e Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Thu, 17 Apr 2025 12:30:12 -0700 Subject: [PATCH 7/8] revert `getArtifact()` changes - for now we have to list and filter by artifact-ids until a `getArtifactById()` public method exists --- __tests__/download.test.ts | 162 ------------------------------------- dist/index.js | 19 +---- src/download-artifact.ts | 43 ++-------- 3 files changed, 11 insertions(+), 213 deletions(-) diff --git a/__tests__/download.test.ts b/__tests__/download.test.ts index 888c4e7..09b4573 100644 --- a/__tests__/download.test.ts +++ b/__tests__/download.test.ts @@ -25,8 +25,6 @@ const mockInputs = (overrides?: Partial<{[K in Inputs]?: any}>) => { [Inputs.Repository]: 'owner/some-repository', [Inputs.RunID]: 'some-run-id', [Inputs.Pattern]: 'some-pattern', - [Inputs.MergeMultiple]: false, - [Inputs.ArtifactIds]: '', ...overrides } @@ -223,164 +221,4 @@ describe('download', () => { expect.stringContaining('digest validation failed') ) }) - - test('throws error when both name and artifact-ids are provided', async () => { - mockInputs({ - [Inputs.Name]: 'artifact-name', - [Inputs.ArtifactIds]: '123' - }) - - await expect(run()).rejects.toThrow( - "Inputs 'name' and 'artifact-ids' cannot be used together. Please specify only one." - ) - }) - - test('throws error when artifact-ids is empty', async () => { - mockInputs({ - [Inputs.Name]: '', - [Inputs.ArtifactIds]: ' , ' - }) - - await expect(run()).rejects.toThrow( - "No valid artifact IDs provided in 'artifact-ids' input" - ) - }) - - test('throws error when artifact-id is not a number', async () => { - mockInputs({ - [Inputs.Name]: '', - [Inputs.ArtifactIds]: '123,abc,456' - }) - - await expect(run()).rejects.toThrow( - "Invalid artifact ID: 'abc'. Must be a number." - ) - }) - - test('downloads a single artifact by ID', async () => { - const mockArtifact = { - id: 123, - name: 'artifact-by-id', - size: 1024, - digest: 'def456' - } - - mockInputs({ - [Inputs.Name]: '', - [Inputs.ArtifactIds]: '123' - }) - - jest - .spyOn(artifact, 'getArtifact') - .mockImplementation(() => Promise.resolve({artifact: mockArtifact})) - - await run() - - expect(core.debug).toHaveBeenCalledWith( - 'Only one artifact ID provided. Fetching latest artifact by its name and checking the ID' - ) - expect(artifact.getArtifact).toHaveBeenCalled() - expect(artifact.downloadArtifact).toHaveBeenCalledWith( - mockArtifact.id, - expect.objectContaining({ - expectedHash: mockArtifact.digest - }) - ) - expect(artifact.listArtifacts).not.toHaveBeenCalled() - expect(core.info).toHaveBeenCalledWith('Total of 1 artifact(s) downloaded') - }) - - test('throws error when single artifact ID is not found', async () => { - mockInputs({ - [Inputs.Name]: '', - [Inputs.ArtifactIds]: '999' - }) - - jest.spyOn(artifact, 'getArtifact').mockImplementation(() => { - return Promise.resolve({artifact: null} as any) - }) - - await expect(run()).rejects.toThrow( - "Artifact with ID '999' not found. Please check the ID." - ) - }) - - test('downloads multiple artifacts by IDs', async () => { - const mockArtifacts = [ - {id: 123, name: 'artifact1', size: 1024, digest: 'abc123'}, - {id: 456, name: 'artifact2', size: 2048, digest: 'def456'}, - {id: 789, name: 'artifact3', size: 3072, digest: 'ghi789'} - ] - - mockInputs({ - [Inputs.Name]: '', - [Inputs.ArtifactIds]: '123, 456' - }) - - jest - .spyOn(artifact, 'listArtifacts') - .mockImplementation(() => Promise.resolve({artifacts: mockArtifacts})) - - await run() - - expect(core.info).toHaveBeenCalledWith( - 'Multiple artifact IDs provided. Fetching all artifacts to filter by ID' - ) - expect(artifact.getArtifact).not.toHaveBeenCalled() - expect(artifact.listArtifacts).toHaveBeenCalled() - expect(artifact.downloadArtifact).toHaveBeenCalledTimes(2) - expect(artifact.downloadArtifact).toHaveBeenCalledWith( - 123, - expect.anything() - ) - expect(artifact.downloadArtifact).toHaveBeenCalledWith( - 456, - expect.anything() - ) - }) - - test('warns when some artifact IDs are not found', async () => { - const mockArtifacts = [ - {id: 123, name: 'artifact1', size: 1024, digest: 'abc123'} - ] - - mockInputs({ - [Inputs.Name]: '', - [Inputs.ArtifactIds]: '123, 456, 789' - }) - - jest - .spyOn(artifact, 'listArtifacts') - .mockImplementation(() => Promise.resolve({artifacts: mockArtifacts})) - - await run() - - expect(core.warning).toHaveBeenCalledWith( - 'Could not find the following artifact IDs: 456, 789' - ) - expect(artifact.downloadArtifact).toHaveBeenCalledTimes(1) - expect(artifact.downloadArtifact).toHaveBeenCalledWith( - 123, - expect.anything() - ) - }) - - test('throws error when none of the provided artifact IDs are found', async () => { - const mockArtifacts = [ - {id: 999, name: 'other-artifact', size: 1024, digest: 'xyz999'} - ] - - mockInputs({ - [Inputs.Name]: '', - [Inputs.ArtifactIds]: '123, 456' - }) - - jest - .spyOn(artifact, 'listArtifacts') - .mockImplementation(() => Promise.resolve({artifacts: mockArtifacts})) - - await expect(run()).rejects.toThrow( - 'None of the provided artifact IDs were found' - ) - }) }) diff --git a/dist/index.js b/dist/index.js index 61783a4..374211f 100644 --- a/dist/index.js +++ b/dist/index.js @@ -118845,22 +118845,9 @@ function run() { } return numericId; }); - // if the length of artifactIds exactly 1 fetch the latest artifact by its name and check the ID - if (artifactIds.length === 1) { - core.debug(`Only one artifact ID provided. Fetching latest artifact by its name and checking the ID`); - const { artifact: targetArtifact } = yield artifact_1.default.getArtifact(inputs.name, options); - if (!targetArtifact) { - throw new Error(`Artifact with ID '${artifactIds[0]}' not found. Please check the ID.`); - } - core.debug(`Found artifact by ID '${targetArtifact.name}' (ID: ${targetArtifact.id}, Size: ${targetArtifact.size})`); - artifacts = [targetArtifact]; - } - else { - core.info(`Multiple artifact IDs provided. Fetching all artifacts to filter by ID`); - // We need to fetch all artifacts to get metadata for the specified IDs - const listArtifactResponse = yield artifact_1.default.listArtifacts(Object.assign({ latest: true }, options)); - artifacts = listArtifactResponse.artifacts.filter(artifact => artifactIds.includes(artifact.id)); - } + // We need to fetch all artifacts to get metadata for the specified IDs + const listArtifactResponse = yield artifact_1.default.listArtifacts(Object.assign({ latest: true }, options)); + artifacts = listArtifactResponse.artifacts.filter(artifact => artifactIds.includes(artifact.id)); if (artifacts.length === 0) { throw new Error(`None of the provided artifact IDs were found`); } diff --git a/src/download-artifact.ts b/src/download-artifact.ts index 5414f10..5cc6b17 100644 --- a/src/download-artifact.ts +++ b/src/download-artifact.ts @@ -109,42 +109,15 @@ export async function run(): Promise { return numericId }) - // if the length of artifactIds exactly 1 fetch the latest artifact by its name and check the ID - if (artifactIds.length === 1) { - core.debug( - `Only one artifact ID provided. Fetching latest artifact by its name and checking the ID` - ) - const {artifact: targetArtifact} = await artifactClient.getArtifact( - inputs.name, - options - ) + // We need to fetch all artifacts to get metadata for the specified IDs + const listArtifactResponse = await artifactClient.listArtifacts({ + latest: true, + ...options + }) - if (!targetArtifact) { - throw new Error( - `Artifact with ID '${artifactIds[0]}' not found. Please check the ID.` - ) - } - - core.debug( - `Found artifact by ID '${targetArtifact.name}' (ID: ${targetArtifact.id}, Size: ${targetArtifact.size})` - ) - - artifacts = [targetArtifact] - } else { - core.info( - `Multiple artifact IDs provided. Fetching all artifacts to filter by ID` - ) - - // We need to fetch all artifacts to get metadata for the specified IDs - const listArtifactResponse = await artifactClient.listArtifacts({ - latest: true, - ...options - }) - - artifacts = listArtifactResponse.artifacts.filter(artifact => - artifactIds.includes(artifact.id) - ) - } + artifacts = listArtifactResponse.artifacts.filter(artifact => + artifactIds.includes(artifact.id) + ) if (artifacts.length === 0) { throw new Error(`None of the provided artifact IDs were found`) From d219c630f65d8bd14366a9e2f731cbf854f62258 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Thu, 17 Apr 2025 13:14:21 -0700 Subject: [PATCH 8/8] add supporting unit tests for artifact downloads with ids --- __tests__/download.test.ts | 150 +++++++++++++++++++++++++++++++++++++ 1 file changed, 150 insertions(+) diff --git a/__tests__/download.test.ts b/__tests__/download.test.ts index 09b4573..032d857 100644 --- a/__tests__/download.test.ts +++ b/__tests__/download.test.ts @@ -221,4 +221,154 @@ describe('download', () => { expect.stringContaining('digest validation failed') ) }) + + test('downloads a single artifact by ID', async () => { + const mockArtifact = { + id: 456, + name: 'artifact-by-id', + size: 1024, + digest: 'def456' + } + + mockInputs({ + [Inputs.Name]: '', + [Inputs.Pattern]: '', + [Inputs.ArtifactIds]: '456' + }) + + jest.spyOn(artifact, 'listArtifacts').mockImplementation(() => + Promise.resolve({ + artifacts: [mockArtifact] + }) + ) + + await run() + + expect(core.info).toHaveBeenCalledWith('Downloading artifacts by ID') + expect(core.debug).toHaveBeenCalledWith('Parsed artifact IDs: ["456"]') + expect(artifact.downloadArtifact).toHaveBeenCalledTimes(1) + expect(artifact.downloadArtifact).toHaveBeenCalledWith( + 456, + expect.objectContaining({ + expectedHash: mockArtifact.digest + }) + ) + expect(core.info).toHaveBeenCalledWith('Total of 1 artifact(s) downloaded') + }) + + test('downloads multiple artifacts by ID', async () => { + const mockArtifacts = [ + {id: 123, name: 'first-artifact', size: 1024, digest: 'abc123'}, + {id: 456, name: 'second-artifact', size: 2048, digest: 'def456'}, + {id: 789, name: 'third-artifact', size: 3072, digest: 'ghi789'} + ] + + mockInputs({ + [Inputs.Name]: '', + [Inputs.Pattern]: '', + [Inputs.ArtifactIds]: '123, 456, 789' + }) + + jest.spyOn(artifact, 'listArtifacts').mockImplementation(() => + Promise.resolve({ + artifacts: mockArtifacts + }) + ) + + await run() + + expect(core.info).toHaveBeenCalledWith('Downloading artifacts by ID') + expect(core.debug).toHaveBeenCalledWith( + 'Parsed artifact IDs: ["123","456","789"]' + ) + expect(artifact.downloadArtifact).toHaveBeenCalledTimes(3) + mockArtifacts.forEach(mockArtifact => { + expect(artifact.downloadArtifact).toHaveBeenCalledWith( + mockArtifact.id, + expect.objectContaining({ + expectedHash: mockArtifact.digest + }) + ) + }) + expect(core.info).toHaveBeenCalledWith('Total of 3 artifact(s) downloaded') + }) + + test('warns when some artifact IDs are not found', async () => { + const mockArtifacts = [ + {id: 123, name: 'found-artifact', size: 1024, digest: 'abc123'} + ] + + mockInputs({ + [Inputs.Name]: '', + [Inputs.Pattern]: '', + [Inputs.ArtifactIds]: '123, 456, 789' + }) + + jest.spyOn(artifact, 'listArtifacts').mockImplementation(() => + Promise.resolve({ + artifacts: mockArtifacts + }) + ) + + await run() + + expect(core.warning).toHaveBeenCalledWith( + 'Could not find the following artifact IDs: 456, 789' + ) + expect(core.debug).toHaveBeenCalledWith('Found 1 artifacts by ID') + expect(artifact.downloadArtifact).toHaveBeenCalledTimes(1) + }) + + test('throws error when no artifacts with requested IDs are found', async () => { + mockInputs({ + [Inputs.Name]: '', + [Inputs.Pattern]: '', + [Inputs.ArtifactIds]: '123, 456' + }) + + jest.spyOn(artifact, 'listArtifacts').mockImplementation(() => + Promise.resolve({ + artifacts: [] + }) + ) + + await expect(run()).rejects.toThrow( + 'None of the provided artifact IDs were found' + ) + }) + + test('throws error when artifact-ids input is empty', async () => { + mockInputs({ + [Inputs.Name]: '', + [Inputs.Pattern]: '', + [Inputs.ArtifactIds]: ' ' + }) + + await expect(run()).rejects.toThrow( + "No valid artifact IDs provided in 'artifact-ids' input" + ) + }) + + test('throws error when some artifact IDs are not valid numbers', async () => { + mockInputs({ + [Inputs.Name]: '', + [Inputs.Pattern]: '', + [Inputs.ArtifactIds]: '123, abc, 456' + }) + + await expect(run()).rejects.toThrow( + "Invalid artifact ID: 'abc'. Must be a number." + ) + }) + + test('throws error when both name and artifact-ids are provided', async () => { + mockInputs({ + [Inputs.Name]: 'some-artifact', + [Inputs.ArtifactIds]: '123' + }) + + await expect(run()).rejects.toThrow( + "Inputs 'name' and 'artifact-ids' cannot be used together. Please specify only one." + ) + }) })