Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@actions/artifact package updates #408

Merged
merged 4 commits into from
Apr 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/artifact/RELEASES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
65 changes: 59 additions & 6 deletions packages/artifact/__tests__/download-specification.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ function getPartialContainerEntry(): ContainerEntry {
lastModifiedBy: '82f0bf89-6e55-4e5a-b8b6-f75eb992578c',
itemLocation: 'ADD_INFORMATION',
contentLocation: 'ADD_INFORMATION',
contentId: ''
contentId: '',
fileLength: 100
}
}

Expand Down Expand Up @@ -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')
Expand All @@ -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)
Expand All @@ -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')
Expand All @@ -115,7 +121,8 @@ const artifactContainerEntries: ContainerEntry[] = [
directoryEntry3,
directoryEntry4,
fileEntry4,
fileEntry5,
missingLengthFileEntry,
emptyLengthFileEntry,
rootDirectoryEntry2,
extraFileEntry
]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
})
})
14 changes: 14 additions & 0 deletions packages/artifact/__tests__/upload.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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 = {
Expand Down
54 changes: 48 additions & 6 deletions packages/artifact/__tests__/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand All @@ -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
Expand All @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For asynchronously checking if files exist, I used this for guidance

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with fs.existsSync?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fs.existsSync is perfectly fine however I'm trying to use async/promises wherever possible. In general any sync operation will block the event loop until the operation is complete so asyncification is usually a good thing.

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)
})
})
2 changes: 1 addition & 1 deletion packages/artifact/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/artifact/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@actions/artifact",
"version": "0.2.0",
"version": "0.3.0",
"preview": true,
"description": "Actions artifact lib",
"keywords": [
Expand Down
12 changes: 11 additions & 1 deletion packages/artifact/src/internal/artifact-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -228,6 +235,9 @@ export class DefaultArtifactClient implements ArtifactClient {
await createDirectoriesForArtifact(
downloadSpecification.directoryStructure
)
await createEmptyFilesForArtifact(
downloadSpecification.emptyFilesToCreate
)
await downloadHttpClient.downloadSingleArtifact(
downloadSpecification.filesToDownload
)
Expand Down
19 changes: 14 additions & 5 deletions packages/artifact/src/internal/download-specification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[]
}
Expand All @@ -33,13 +36,15 @@ 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<string>()

const specifications: DownloadSpecification = {
rootDownloadLocation: includeRootDirectory
? path.join(downloadPath, artifactName)
: downloadPath,
directoryStructure: [],
emptyFilesToCreate: [],
filesToDownload: []
}

Expand All @@ -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
})
}
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions packages/artifact/src/internal/upload-http-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
isRetryableStatusCode,
isSuccessStatusCode,
isThrottledStatusCode,
isForbiddenStatusCode,
displayHttpDiagnostics,
getExponentialRetryTimeInMilliseconds,
tryGetRetryAfterValueTimeInMilliseconds
Expand Down Expand Up @@ -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(
Expand Down
Loading