actions
/
toolkit
mirror of https://github.com/actions/toolkit
1
0
Fork 0
Code

Merge pull request #1593 from actions/robherley/api-consistency 

Consistent error behavior for Artifact methods
pull/1595/head
Rob Herley 2023-12-05 15:22:16 -05:00 committed by GitHub
parent 8ac8bf1d3d 5f152b798e
commit 04945c6048
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 133 additions and 180 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
.github/workflows/artifact-tests.yml vendored
View File

@ -24,10 +24,10 @@ jobs:
steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4
- name: Set Node.js 20.x
uses: actions/setup-node@v3
uses: actions/setup-node@v4
with:
node-version: 20.x
@ -54,8 +54,8 @@ jobs:
echo '${{ env.file1 }}' > artifact-path/first.txt
echo '${{ env.file2 }}' > artifact-path/second.txt
- name: Upload Artifacts using actions/github-script@v6
uses: actions/github-script@v6
- name: Upload Artifacts using actions/github-script@v7
uses: actions/github-script@v7
with:
script: |
const artifact = require('./packages/artifact/lib/artifact')
@ -68,25 +68,20 @@ jobs:
const uploadResult = await artifact.create().uploadArtifact(artifactName, fileContents, './')
console.log(uploadResult)
const success = uploadResult.success
const size = uploadResult.size
const id = uploadResult.id
if (!success) {
throw new Error('Failed to upload artifact')
} else {
console.log(`Successfully uploaded artifact ${id}`)
}
console.log(`Successfully uploaded artifact ${id}`)
verify:
runs-on: ubuntu-latest
needs: [build]
steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4
- name: Set Node.js 20.x
uses: actions/setup-node@v3
uses: actions/setup-node@v4
with:
node-version: 20.x
@ -101,8 +96,8 @@ jobs:
npm run tsc
working-directory: packages/artifact
- name: List artifacts using actions/github-script@v6
uses: actions/github-script@v6
- name: List artifacts using actions/github-script@v7
uses: actions/github-script@v7
with:
script: |
const artifact = require('./packages/artifact/lib/artifact')

4
packages/artifact/__tests__/download-artifact.test.ts
View File

@ -164,7 +164,6 @@ describe('download-artifact', () => {
fixtures.blobStorageUrl
)
expectExtractedArchive(fixtures.workspaceDir)
expect(response.success).toBe(true)
expect(response.downloadPath).toBe(fixtures.workspaceDir)
})
@ -214,7 +213,6 @@ describe('download-artifact', () => {
fixtures.blobStorageUrl
)
expectExtractedArchive(customPath)
expect(response.success).toBe(true)
expect(response.downloadPath).toBe(customPath)
})
@ -344,7 +342,6 @@ describe('download-artifact', () => {
const response = await downloadArtifactInternal(fixtures.artifactID)
expectExtractedArchive(fixtures.workspaceDir)
expect(response.success).toBe(true)
expect(response.downloadPath).toBe(fixtures.workspaceDir)
expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString())
expect(mockListArtifacts).toHaveBeenCalledWith({
@ -396,7 +393,6 @@ describe('download-artifact', () => {
})
expectExtractedArchive(customPath)
expect(response.success).toBe(true)
expect(response.downloadPath).toBe(customPath)
expect(mockHttpClient).toHaveBeenCalledWith(getUserAgentString())
expect(mockListArtifacts).toHaveBeenCalledWith({

34
packages/artifact/__tests__/get-artifact.test.ts
View File

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

19
packages/artifact/__tests__/upload-artifact.test.ts
View File

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

1
packages/artifact/src/artifact.ts
View File

@ -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 {

75
packages/artifact/src/internal/client.ts
View File

@ -17,6 +17,7 @@ import {
} from './download/download-artifact'
import {getArtifactPublic, getArtifactInternal} from './find/get-artifact'
import {listArtifactsPublic, listArtifactsInternal} from './find/list-artifacts'
import {GHESNotSupportedError} from './shared/errors'
export interface ArtifactClient {
/**
@ -52,6 +53,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 the artifact is not found, it will throw.
*
* 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
@ -99,16 +101,11 @@ export class Client implements ArtifactClient {
rootDirectory: string,
options?: UploadArtifactOptions
): Promise<UploadArtifactResponse> {
if (isGhes()) {
warning(
`@actions/artifact v2.0.0+ and upload-artifact@v4+ are not currently supported on GHES.`
)
return {
success: false
}
}
try {
if (isGhes()) {
throw new GHESNotSupportedError()
}
return uploadArtifact(name, files, rootDirectory, options)
} catch (error) {
warning(
@ -118,9 +115,8 @@ Errors can be temporary, so please try again and optionally run the action with
If the error persists, please check whether Actions is operating normally at [https://githubstatus.com](https://www.githubstatus.com).`
)
return {
success: false
}
throw error
}
}
@ -131,16 +127,11 @@ If the error persists, please check whether Actions is operating normally at [ht
artifactId: number,
options?: DownloadArtifactOptions & FindOptions
): Promise<DownloadArtifactResponse> {
if (isGhes()) {
warning(
`@actions/artifact v2.0.0+ and download-artifact@v4+ are not currently supported on GHES.`
)
return {
success: false
}
}
try {
if (isGhes()) {
throw new GHESNotSupportedError()
}
if (options?.findBy) {
const {
findBy: {repositoryOwner, repositoryName, token},
@ -159,16 +150,14 @@ If the error persists, please check whether Actions is operating normally at [ht
return downloadArtifactInternal(artifactId, options)
} catch (error) {
warning(
`Artifact download failed with error: ${error}.
`Download Artifact failed with error: ${error}.
Errors can be temporary, so please try again and optionally run the action with debug mode enabled for more information.
If the error persists, please check whether Actions and API requests are operating normally at [https://githubstatus.com](https://www.githubstatus.com).`
)
return {
success: false
}
throw error
}
}
@ -178,16 +167,11 @@ If the error persists, please check whether Actions and API requests are operati
async listArtifacts(
options?: ListArtifactsOptions & FindOptions
): Promise<ListArtifactsResponse> {
if (isGhes()) {
warning(
`@actions/artifact v2.0.0+ and download-artifact@v4+ are not currently supported on GHES.`
)
return {
artifacts: []
}
}
try {
if (isGhes()) {
throw new GHESNotSupportedError()
}
if (options?.findBy) {
const {
findBy: {workflowRunId, repositoryOwner, repositoryName, token}
@ -212,9 +196,7 @@ Errors can be temporary, so please try again and optionally run the action with
If the error persists, please check whether Actions and API requests are operating normally at [https://githubstatus.com](https://www.githubstatus.com).`
)
return {
artifacts: []
}
throw error
}
}
@ -225,16 +207,11 @@ If the error persists, please check whether Actions and API requests are operati
artifactName: string,
options?: FindOptions
): Promise<GetArtifactResponse> {
if (isGhes()) {
warning(
`@actions/artifact v2.0.0+ and download-artifact@v4+ are not currently supported on GHES.`
)
return {
success: false
}
}
try {
if (isGhes()) {
throw new GHESNotSupportedError()
}
if (options?.findBy) {
const {
findBy: {workflowRunId, repositoryOwner, repositoryName, token}
@ -252,15 +229,13 @@ If the error persists, please check whether Actions and API requests are operati
return getArtifactInternal(artifactName)
} catch (error: unknown) {
warning(
`Fetching Artifact failed with error: ${error}.
`Get Artifact failed with error: ${error}.
Errors can be temporary, so please try again and optionally run the action with debug mode enabled for more information.
If the error persists, please check whether Actions and API requests are operating normally at [https://githubstatus.com](https://www.githubstatus.com).`
)
return {
success: false
}
throw error
}
}
}

8
packages/artifact/src/internal/download/download-artifact.ts
View File

@ -16,6 +16,7 @@ import {
ListArtifactsRequest
} from '../../generated'
import {getBackendIdsFromToken} from '../shared/util'
import {ArtifactNotFoundError} from '../shared/errors'
const scrubQueryParameters = (url: string): string => {
const parsed = new URL(url)
@ -95,7 +96,7 @@ export async function downloadArtifactPublic(
throw new Error(`Unable to download and extract artifact: ${error.message}`)
}
return {success: true, downloadPath}
return {downloadPath}
}
export async function downloadArtifactInternal(
@ -118,10 +119,9 @@ export async function downloadArtifactInternal(
const {artifacts} = await artifactClient.ListArtifacts(listReq)
if (artifacts.length === 0) {
core.warning(
throw new ArtifactNotFoundError(
`No artifacts found for ID: ${artifactId}\nAre you trying to download from a different run? Try specifying a github-token with \`actions:read\` scope.`
)
return {success: false}
}
if (artifacts.length > 1) {
@ -148,7 +148,7 @@ export async function downloadArtifactInternal(
throw new Error(`Unable to download and extract artifact: ${error.message}`)
}
return {success: true, downloadPath}
return {downloadPath}
}
async function resolveOrCreateDirectory(

26
packages/artifact/src/internal/find/get-artifact.ts
View File

@ -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,15 @@ 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(
`Artifact not found for name: ${artifactName}`
)
}
let artifact = getArtifactResp.data.artifacts[0]
@ -63,7 +62,6 @@ export async function getArtifactPublic(
}
return {
success: true,
artifact: {
name: artifact.name,
id: artifact.id,
@ -90,10 +88,9 @@ 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(
`Artifact not found for name: ${artifactName}`
)
}
let artifact = res.artifacts[0]
@ -103,12 +100,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),

37
packages/artifact/src/internal/shared/errors.ts Normal file
View File

@ -0,0 +1,37 @@
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'
}
}
export class ArtifactNotFoundError extends Error {
constructor(message = 'Artifact not found') {
super(message)
this.name = 'ArtifactNotFoundError'
}
}
export class GHESNotSupportedError extends Error {
constructor(
message = '@actions/artifact v2.0.0+ and download-artifact@v4+ are not currently supported on GHES.'
) {
super(message)
this.name = 'GHESNotSupportedError'
}
}

16
packages/artifact/src/internal/shared/interfaces.ts
View File

@ -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
*/
@ -58,15 +53,10 @@ export interface UploadArtifactOptions {
*****************************************************************************/
export interface GetArtifactResponse {
/**
* If an artifact was found
*/
success: boolean
/**
* Metadata about the artifact that was found
*/
artifact?: Artifact
artifact: Artifact
}
/*****************************************************************************
@ -96,10 +86,6 @@ export interface ListArtifactsResponse {
* *
*****************************************************************************/
export interface DownloadArtifactResponse {
/**
* If the artifact download was successful
*/
success: boolean
/**
* The path where the artifact was downloaded to
*/

42
packages/artifact/src/internal/upload/blob-upload.ts
View File

@ -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,41 +50,28 @@ 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')
core.info('Beginning upload of artifact content to blob storage')
await blockBlobClient.uploadStream(
uploadStream,
bufferSize,
maxConcurrency,
options
)
await blockBlobClient.uploadStream(
uploadStream,
bufferSize,
maxConcurrency,
options
)
core.info('Finished uploading artifact content to blob storage!')
core.info('Finished uploading artifact content to blob storage!')
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
}
}
hashStream.end()
sha256Hash = hashStream.read() as string
core.info(`SHA256 hash of uploaded artifact zip is ${sha256Hash}`)
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
}

28
packages/artifact/src/internal/upload/upload-artifact.ts
View File

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