From c010a271d94e467c33b73e73d9b337b6d6f8a416 Mon Sep 17 00:00:00 2001 From: Konrad Pabjan Date: Thu, 9 Apr 2020 17:14:12 +0200 Subject: [PATCH] @actions/artifact package updates (#408) * Clear error message when storage quota has been hit * Improved download of empty files * Extra info to RELEASES.md * PR Feedback --- packages/artifact/RELEASES.md | 9 +++ .../__tests__/download-specification.test.ts | 65 +++++++++++++++++-- packages/artifact/__tests__/upload.test.ts | 14 ++++ packages/artifact/__tests__/util.test.ts | 54 +++++++++++++-- packages/artifact/package-lock.json | 2 +- packages/artifact/package.json | 2 +- .../artifact/src/internal/artifact-client.ts | 12 +++- .../src/internal/download-specification.ts | 19 ++++-- .../src/internal/upload-http-client.ts | 7 ++ packages/artifact/src/internal/utils.ts | 16 +++++ 10 files changed, 180 insertions(+), 20 deletions(-) diff --git a/packages/artifact/RELEASES.md b/packages/artifact/RELEASES.md index 35097b35..577cd43a 100644 --- a/packages/artifact/RELEASES.md +++ b/packages/artifact/RELEASES.md @@ -10,3 +10,12 @@ - GZip file compression to speed up downloads - Improved logging and output - Extra documentation + +### 0.3.0 + +- Fixes to gzip decompression when downloading artifacts +- Support handling 429 response codes +- Improved download experience when dealing with empty files +- Exponential backoff when retryable status codes are encountered +- Clearer error message if storage quota has been reached +- Improved logging and output during artifact download diff --git a/packages/artifact/__tests__/download-specification.test.ts b/packages/artifact/__tests__/download-specification.test.ts index 50ca4648..bc40879c 100644 --- a/packages/artifact/__tests__/download-specification.test.ts +++ b/packages/artifact/__tests__/download-specification.test.ts @@ -21,7 +21,8 @@ function getPartialContainerEntry(): ContainerEntry { lastModifiedBy: '82f0bf89-6e55-4e5a-b8b6-f75eb992578c', itemLocation: 'ADD_INFORMATION', contentLocation: 'ADD_INFORMATION', - contentId: '' + contentId: '', + fileLength: 100 } } @@ -72,13 +73,13 @@ function createContentLocation(relativePath: string): string { /dir3 /dir4 file4.txt - file5.txt - + file5.txt (no length property) + file6.txt (empty file) /my-artifact-extra /file1.txt */ -// main artfact +// main artifact const file1Path = path.join(artifact1Name, 'file1.txt') const file2Path = path.join(artifact1Name, 'file2.txt') const dir1Path = path.join(artifact1Name, 'dir1') @@ -88,6 +89,7 @@ const dir3Path = path.join(dir2Path, 'dir3') const dir4Path = path.join(dir3Path, 'dir4') const file4Path = path.join(dir4Path, 'file4.txt') const file5Path = path.join(dir4Path, 'file5.txt') +const file6Path = path.join(dir4Path, 'file6.txt') const rootDirectoryEntry = createDirectoryEntry(artifact1Name) const directoryEntry1 = createDirectoryEntry(dir1Path) @@ -98,7 +100,11 @@ const fileEntry1 = createFileEntry(file1Path) const fileEntry2 = createFileEntry(file2Path) const fileEntry3 = createFileEntry(file3Path) const fileEntry4 = createFileEntry(file4Path) -const fileEntry5 = createFileEntry(file5Path) + +const missingLengthFileEntry = createFileEntry(file5Path) +missingLengthFileEntry.fileLength = undefined // one file does not have a fileLength +const emptyLengthFileEntry = createFileEntry(file6Path) +emptyLengthFileEntry.fileLength = 0 // empty file path // extra artifact const artifact2File1Path = path.join(artifact2Name, 'file1.txt') @@ -115,7 +121,8 @@ const artifactContainerEntries: ContainerEntry[] = [ directoryEntry3, directoryEntry4, fileEntry4, - fileEntry5, + missingLengthFileEntry, + emptyLengthFileEntry, rootDirectoryEntry2, extraFileEntry ] @@ -170,6 +177,14 @@ describe('Search', () => { 'dir4', 'file5.txt' ) + const item6ExpectedTargetPath = path.join( + testDownloadPath, + 'dir1', + 'dir2', + 'dir3', + 'dir4', + 'file6.txt' + ) const targetLocations = specification.filesToDownload.map( item => item.targetPath @@ -214,6 +229,9 @@ describe('Search', () => { expect(specification.directoryStructure).toContain( path.join(testDownloadPath, 'dir1', 'dir2', 'dir3', 'dir4') ) + + expect(specification.emptyFilesToCreate.length).toEqual(1) + expect(specification.emptyFilesToCreate).toContain(item6ExpectedTargetPath) }) it('Download Specification - Relative Path with no root directory', () => { @@ -252,6 +270,14 @@ describe('Search', () => { 'dir4', 'file5.txt' ) + const item6ExpectedTargetPath = path.join( + testDownloadPath, + 'dir1', + 'dir2', + 'dir3', + 'dir4', + 'file6.txt' + ) const targetLocations = specification.filesToDownload.map( item => item.targetPath @@ -296,6 +322,9 @@ describe('Search', () => { expect(specification.directoryStructure).toContain( path.join(testDownloadPath, 'dir1', 'dir2', 'dir3', 'dir4') ) + + expect(specification.emptyFilesToCreate.length).toEqual(1) + expect(specification.emptyFilesToCreate).toContain(item6ExpectedTargetPath) }) it('Download Specification - Absolute Path with root directory', () => { @@ -352,6 +381,15 @@ describe('Search', () => { 'dir4', 'file5.txt' ) + const item6ExpectedTargetPath = path.join( + testDownloadPath, + artifact1Name, + 'dir1', + 'dir2', + 'dir3', + 'dir4', + 'file6.txt' + ) const targetLocations = specification.filesToDownload.map( item => item.targetPath @@ -398,6 +436,9 @@ describe('Search', () => { expect(specification.directoryStructure).toContain( path.join(testDownloadPath, dir4Path) ) + + expect(specification.emptyFilesToCreate.length).toEqual(1) + expect(specification.emptyFilesToCreate).toContain(item6ExpectedTargetPath) }) it('Download Specification - Relative Path with root directory', () => { @@ -449,6 +490,15 @@ describe('Search', () => { 'dir4', 'file5.txt' ) + const item6ExpectedTargetPath = path.join( + testDownloadPath, + artifact1Name, + 'dir1', + 'dir2', + 'dir3', + 'dir4', + 'file6.txt' + ) const targetLocations = specification.filesToDownload.map( item => item.targetPath @@ -495,5 +545,8 @@ describe('Search', () => { expect(specification.directoryStructure).toContain( path.join(testDownloadPath, dir4Path) ) + + expect(specification.emptyFilesToCreate.length).toEqual(1) + expect(specification.emptyFilesToCreate).toContain(item6ExpectedTargetPath) }) }) diff --git a/packages/artifact/__tests__/upload.test.ts b/packages/artifact/__tests__/upload.test.ts index be1b3bf2..fd5ca362 100644 --- a/packages/artifact/__tests__/upload.test.ts +++ b/packages/artifact/__tests__/upload.test.ts @@ -106,6 +106,18 @@ describe('Upload Tests', () => { ) }) + it('Create Artifact - Storage Quota Error', async () => { + const artifactName = 'storage-quota-hit' + const uploadHttpClient = new UploadHttpClient() + expect( + uploadHttpClient.createArtifactInFileContainer(artifactName) + ).rejects.toEqual( + new Error( + 'Artifact storage quota has been hit. Unable to upload any new artifacts' + ) + ) + }) + /** * Artifact Upload Tests */ @@ -367,6 +379,8 @@ describe('Upload Tests', () => { if (inputData.Name === 'invalid-artifact-name') { mockMessage.statusCode = 400 + } else if (inputData.Name === 'storage-quota-hit') { + mockMessage.statusCode = 403 } else { mockMessage.statusCode = 201 const response: ArtifactResponse = { diff --git a/packages/artifact/__tests__/util.test.ts b/packages/artifact/__tests__/util.test.ts index 1ca963eb..889777ee 100644 --- a/packages/artifact/__tests__/util.test.ts +++ b/packages/artifact/__tests__/util.test.ts @@ -190,14 +190,14 @@ describe('Utils', () => { true ) expect(utils.isRetryableStatusCode(HttpCodes.GatewayTimeout)).toEqual(true) - expect(utils.isRetryableStatusCode(429)).toEqual(true) + expect(utils.isRetryableStatusCode(HttpCodes.TooManyRequests)).toEqual(true) expect(utils.isRetryableStatusCode(HttpCodes.OK)).toEqual(false) expect(utils.isRetryableStatusCode(HttpCodes.NotFound)).toEqual(false) expect(utils.isRetryableStatusCode(HttpCodes.Forbidden)).toEqual(false) }) it('Test Throttled Status Code', () => { - expect(utils.isThrottledStatusCode(429)).toEqual(true) + expect(utils.isThrottledStatusCode(HttpCodes.TooManyRequests)).toEqual(true) expect(utils.isThrottledStatusCode(HttpCodes.InternalServerError)).toEqual( false ) @@ -207,6 +207,17 @@ describe('Utils', () => { ) }) + it('Test Forbidden Status Code', () => { + expect(utils.isForbiddenStatusCode(HttpCodes.Forbidden)).toEqual(true) + expect(utils.isForbiddenStatusCode(HttpCodes.InternalServerError)).toEqual( + false + ) + expect(utils.isForbiddenStatusCode(HttpCodes.TooManyRequests)).toEqual( + false + ) + expect(utils.isForbiddenStatusCode(HttpCodes.OK)).toEqual(false) + }) + it('Test Creating Artifact Directories', async () => { const root = path.join(__dirname, '_temp', 'artifact-download') // remove directory before starting @@ -216,12 +227,43 @@ describe('Utils', () => { const directory2 = path.join(directory1, 'folder1') // Initially should not exist - expect(fs.existsSync(directory1)).toEqual(false) - expect(fs.existsSync(directory2)).toEqual(false) + await expect(fs.promises.access(directory1)).rejects.not.toBeUndefined() + await expect(fs.promises.access(directory2)).rejects.not.toBeUndefined() const directoryStructure = [directory1, directory2] await utils.createDirectoriesForArtifact(directoryStructure) // directories should now be created - expect(fs.existsSync(directory1)).toEqual(true) - expect(fs.existsSync(directory2)).toEqual(true) + await expect(fs.promises.access(directory1)).resolves.toEqual(undefined) + await expect(fs.promises.access(directory2)).resolves.toEqual(undefined) + }) + + it('Test Creating Empty Files', async () => { + const root = path.join(__dirname, '_temp', 'empty-files') + await io.rmRF(root) + + const emptyFile1 = path.join(root, 'emptyFile1') + const directoryToCreate = path.join(root, 'folder1') + const emptyFile2 = path.join(directoryToCreate, 'emptyFile2') + + // empty files should only be created after the directory structure is fully setup + // ensure they are first created by using the createDirectoriesForArtifact method + const directoryStructure = [root, directoryToCreate] + await utils.createDirectoriesForArtifact(directoryStructure) + await expect(fs.promises.access(root)).resolves.toEqual(undefined) + await expect(fs.promises.access(directoryToCreate)).resolves.toEqual( + undefined + ) + + await expect(fs.promises.access(emptyFile1)).rejects.not.toBeUndefined() + await expect(fs.promises.access(emptyFile2)).rejects.not.toBeUndefined() + + const emptyFilesToCreate = [emptyFile1, emptyFile2] + await utils.createEmptyFilesForArtifact(emptyFilesToCreate) + + await expect(fs.promises.access(emptyFile1)).resolves.toEqual(undefined) + const size1 = (await fs.promises.stat(emptyFile1)).size + expect(size1).toEqual(0) + await expect(fs.promises.access(emptyFile2)).resolves.toEqual(undefined) + const size2 = (await fs.promises.stat(emptyFile2)).size + expect(size2).toEqual(0) }) }) diff --git a/packages/artifact/package-lock.json b/packages/artifact/package-lock.json index 9953a518..43d68b43 100644 --- a/packages/artifact/package-lock.json +++ b/packages/artifact/package-lock.json @@ -1,6 +1,6 @@ { "name": "@actions/artifact", - "version": "0.2.0", + "version": "0.3.0", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/packages/artifact/package.json b/packages/artifact/package.json index a940a88c..b070e220 100644 --- a/packages/artifact/package.json +++ b/packages/artifact/package.json @@ -1,6 +1,6 @@ { "name": "@actions/artifact", - "version": "0.2.0", + "version": "0.3.0", "preview": true, "description": "Actions artifact lib", "keywords": [ diff --git a/packages/artifact/src/internal/artifact-client.ts b/packages/artifact/src/internal/artifact-client.ts index afbf9e8a..746a2e8a 100644 --- a/packages/artifact/src/internal/artifact-client.ts +++ b/packages/artifact/src/internal/artifact-client.ts @@ -8,7 +8,11 @@ import {UploadResponse} from './upload-response' import {UploadOptions} from './upload-options' import {DownloadOptions} from './download-options' import {DownloadResponse} from './download-response' -import {checkArtifactName, createDirectoriesForArtifact} from './utils' +import { + checkArtifactName, + createDirectoriesForArtifact, + createEmptyFilesForArtifact +} from './utils' import {DownloadHttpClient} from './download-http-client' import {getDownloadSpecification} from './download-specification' import {getWorkSpaceDirectory} from './config-variables' @@ -174,6 +178,9 @@ export class DefaultArtifactClient implements ArtifactClient { downloadSpecification.directoryStructure ) core.info('Directory structure has been setup for the artifact') + await createEmptyFilesForArtifact( + downloadSpecification.emptyFilesToCreate + ) await downloadHttpClient.downloadSingleArtifact( downloadSpecification.filesToDownload ) @@ -228,6 +235,9 @@ export class DefaultArtifactClient implements ArtifactClient { await createDirectoriesForArtifact( downloadSpecification.directoryStructure ) + await createEmptyFilesForArtifact( + downloadSpecification.emptyFilesToCreate + ) await downloadHttpClient.downloadSingleArtifact( downloadSpecification.filesToDownload ) diff --git a/packages/artifact/src/internal/download-specification.ts b/packages/artifact/src/internal/download-specification.ts index b7ab0c42..0e285ec7 100644 --- a/packages/artifact/src/internal/download-specification.ts +++ b/packages/artifact/src/internal/download-specification.ts @@ -8,6 +8,9 @@ export interface DownloadSpecification { // directories that need to be created for all the items in the artifact directoryStructure: string[] + // empty files that are part of the artifact that don't require any downloading + emptyFilesToCreate: string[] + // individual files that need to be downloaded as part of the artifact filesToDownload: DownloadItem[] } @@ -33,6 +36,7 @@ export function getDownloadSpecification( downloadPath: string, includeRootDirectory: boolean ): DownloadSpecification { + // use a set for the directory paths so that there are no duplicates const directories = new Set() const specifications: DownloadSpecification = { @@ -40,6 +44,7 @@ export function getDownloadSpecification( ? path.join(downloadPath, artifactName) : downloadPath, directoryStructure: [], + emptyFilesToCreate: [], filesToDownload: [] } @@ -64,11 +69,15 @@ export function getDownloadSpecification( if (entry.itemType === 'file') { // Get the directories that we need to create from the filePath for each individual file directories.add(path.dirname(filePath)) - - specifications.filesToDownload.push({ - sourceLocation: entry.contentLocation, - targetPath: filePath - }) + if (entry.fileLength === 0) { + // An empty file was uploaded, create the empty files locally so that no extra http calls are made + specifications.emptyFilesToCreate.push(filePath) + } else { + specifications.filesToDownload.push({ + sourceLocation: entry.contentLocation, + targetPath: filePath + }) + } } } } diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 4f7676c5..cae5b526 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -15,6 +15,7 @@ import { isRetryableStatusCode, isSuccessStatusCode, isThrottledStatusCode, + isForbiddenStatusCode, displayHttpDiagnostics, getExponentialRetryTimeInMilliseconds, tryGetRetryAfterValueTimeInMilliseconds @@ -68,6 +69,12 @@ export class UploadHttpClient { if (isSuccessStatusCode(rawResponse.message.statusCode) && body) { return JSON.parse(body) + } else if (isForbiddenStatusCode(rawResponse.message.statusCode)) { + // if a 403 is returned when trying to create a file container, the customer has exceeded + // their storage quota so no new artifact containers can be created + throw new Error( + `Artifact storage quota has been hit. Unable to upload any new artifacts` + ) } else { displayHttpDiagnostics(rawResponse) throw new Error( diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index 95e14c00..41f4179d 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -58,6 +58,14 @@ export function isSuccessStatusCode(statusCode?: number): boolean { return statusCode >= 200 && statusCode < 300 } +export function isForbiddenStatusCode(statusCode?: number): boolean { + if (!statusCode) { + return false + } + + return statusCode === HttpCodes.Forbidden +} + export function isRetryableStatusCode(statusCode?: number): boolean { if (!statusCode) { return false @@ -296,3 +304,11 @@ export async function createDirectoriesForArtifact( }) } } + +export async function createEmptyFilesForArtifact( + emptyFilesToCreate: string[] +): Promise { + for (const filePath of emptyFilesToCreate) { + await (await fs.open(filePath, 'w')).close() + } +}