From 7628612d834276e5f5936466873958b187008d5e Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Tue, 25 Aug 2020 12:13:34 -0400 Subject: [PATCH 1/8] Add an option to specify retention days for artifacts --- packages/artifact/README.md | 4 ++++ .../artifact/src/internal/artifact-client.ts | 3 ++- packages/artifact/src/internal/contracts.ts | 1 + .../src/internal/upload-http-client.ts | 9 ++++++++- .../artifact/src/internal/upload-options.ts | 18 ++++++++++++++++++ 5 files changed, 33 insertions(+), 2 deletions(-) diff --git a/packages/artifact/README.md b/packages/artifact/README.md index ac67b5cd98..79f033d368 100644 --- a/packages/artifact/README.md +++ b/packages/artifact/README.md @@ -39,6 +39,10 @@ Method Name: `uploadArtifact` - If set to `false`, and an error is encountered, all other uploads will stop and any files that were queued will not be attempted to be uploaded. The partial artifact available will only include files up until the failure. - If set to `true` and an error is encountered, the failed file will be skipped and ignored and all other queued files will be attempted to be uploaded. There will be an artifact available for download at the end with everything excluding the file that failed to upload - Optional, defaults to `true` if not specified +- `retentionDays` + - Durantion after which artifact will expire in days + - Minimum value: 1 + - Maximum value: 90 unless changed by settings #### Example using Absolute File Paths diff --git a/packages/artifact/src/internal/artifact-client.ts b/packages/artifact/src/internal/artifact-client.ts index 746a2e8a05..bdf8849358 100644 --- a/packages/artifact/src/internal/artifact-client.ts +++ b/packages/artifact/src/internal/artifact-client.ts @@ -94,7 +94,8 @@ export class DefaultArtifactClient implements ArtifactClient { } else { // Create an entry for the artifact in the file container const response = await uploadHttpClient.createArtifactInFileContainer( - name + name, + options ) if (!response.fileContainerResourceUrl) { core.debug(response.toString()) diff --git a/packages/artifact/src/internal/contracts.ts b/packages/artifact/src/internal/contracts.ts index c0518dffde..277813ca44 100644 --- a/packages/artifact/src/internal/contracts.ts +++ b/packages/artifact/src/internal/contracts.ts @@ -11,6 +11,7 @@ export interface ArtifactResponse { export interface CreateArtifactParameters { Type: string Name: string + RetentionDays?: number } export interface PatchArtifactSize { diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 66f1dc304b..5cd078b902 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -55,12 +55,19 @@ export class UploadHttpClient { * @returns The response from the Artifact Service if the file container was successfully created */ async createArtifactInFileContainer( - artifactName: string + artifactName: string, + options?: UploadOptions | undefined ): Promise { const parameters: CreateArtifactParameters = { Type: 'actions_storage', Name: artifactName } + + // calculate retention period + if (options && options.retentionDays) { + parameters.RetentionDays = options.retentionDays + } + const data: string = JSON.stringify(parameters, null, 2) const artifactUrl = getArtifactUrl() diff --git a/packages/artifact/src/internal/upload-options.ts b/packages/artifact/src/internal/upload-options.ts index 63d4febe8f..7baca5f789 100644 --- a/packages/artifact/src/internal/upload-options.ts +++ b/packages/artifact/src/internal/upload-options.ts @@ -15,4 +15,22 @@ export interface UploadOptions { * */ continueOnError?: boolean + + /** + * Durantion after which artifact will expire in days. + * + * By default artifact expires after 90 days: + * https://docs.github.com/en/actions/configuring-and-managing-workflows/persisting-workflow-data-using-artifacts#downloading-and-deleting-artifacts-after-a-workflow-run-is-complete + * + * Use this option to override the default expiry. + * + * Min value: 1 + * Max value: 90 unless changed by repository setting + * + * If this is set to a greater value than the retention settings allowed, the retention on artifacts + * will be reduced to match the max value allowed on server, and the upload process will continue. An + * input of 0 assumes default retention setting. + */ + + retentionDays?: number } From 680dc94717f79cbb23fb0a9d183394d1b93924e2 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Thu, 17 Sep 2020 13:53:01 -0400 Subject: [PATCH 2/8] Validate against settings exposed as env var to give early feedback --- .../src/internal/__mocks__/config-variables.ts | 4 ++++ packages/artifact/src/internal/config-variables.ts | 6 ++++++ .../artifact/src/internal/upload-http-client.ts | 13 +++++++++++-- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/artifact/src/internal/__mocks__/config-variables.ts b/packages/artifact/src/internal/__mocks__/config-variables.ts index af7f7636fa..55aaf2e0c6 100644 --- a/packages/artifact/src/internal/__mocks__/config-variables.ts +++ b/packages/artifact/src/internal/__mocks__/config-variables.ts @@ -45,3 +45,7 @@ export function getRuntimeUrl(): string { export function getWorkFlowRunId(): string { return '15' } + +export function getRetentionDays(): string | undefined { + return '45' +} diff --git a/packages/artifact/src/internal/config-variables.ts b/packages/artifact/src/internal/config-variables.ts index 1d25f6ebfc..e8ea9c3b4e 100644 --- a/packages/artifact/src/internal/config-variables.ts +++ b/packages/artifact/src/internal/config-variables.ts @@ -1,3 +1,5 @@ +import { countReset } from "console" + // The number of concurrent uploads that happens at the same time export function getUploadFileConcurrency(): number { return 2 @@ -61,3 +63,7 @@ export function getWorkSpaceDirectory(): string { } return workspaceDirectory } + +export function getRetentionDays(): string | undefined { + return process.env['GITHUB_RETENTION_DAYS'] +} \ No newline at end of file diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 5cd078b902..7fdb42eb9b 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -23,7 +23,8 @@ import { import { getUploadChunkSize, getUploadFileConcurrency, - getRetryLimit + getRetryLimit, + getRetentionDays } from './config-variables' import {promisify} from 'util' import {URL} from 'url' @@ -65,7 +66,15 @@ export class UploadHttpClient { // calculate retention period if (options && options.retentionDays) { - parameters.RetentionDays = options.retentionDays + var retention = options.retentionDays; + const maxRetentionStr = getRetentionDays(); + if (maxRetentionStr) { + const maxRetentions = parseInt(maxRetentionStr); + if (!isNaN(maxRetentions) && maxRetentions < retention) { + core.warning(`Retention days is greater than max value allowed by repository setting, reduce retention to ${maxRetentions} days`); + } + } + parameters.RetentionDays = retention; } const data: string = JSON.stringify(parameters, null, 2) From 0125c31adf0d286c0b7626cdc5a5a8b60dc26760 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Thu, 17 Sep 2020 15:28:25 -0400 Subject: [PATCH 3/8] Fix lint --- .../artifact/src/internal/config-variables.ts | 6 ++---- .../artifact/src/internal/upload-http-client.ts | 15 +++++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/artifact/src/internal/config-variables.ts b/packages/artifact/src/internal/config-variables.ts index e8ea9c3b4e..0124358828 100644 --- a/packages/artifact/src/internal/config-variables.ts +++ b/packages/artifact/src/internal/config-variables.ts @@ -1,5 +1,3 @@ -import { countReset } from "console" - // The number of concurrent uploads that happens at the same time export function getUploadFileConcurrency(): number { return 2 @@ -65,5 +63,5 @@ export function getWorkSpaceDirectory(): string { } export function getRetentionDays(): string | undefined { - return process.env['GITHUB_RETENTION_DAYS'] -} \ No newline at end of file + return process.env['GITHUB_RETENTION_DAYS'] +} diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 7fdb42eb9b..25be2db5de 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -66,15 +66,18 @@ export class UploadHttpClient { // calculate retention period if (options && options.retentionDays) { - var retention = options.retentionDays; - const maxRetentionStr = getRetentionDays(); + let retention = options.retentionDays + const maxRetentionStr = getRetentionDays() if (maxRetentionStr) { - const maxRetentions = parseInt(maxRetentionStr); - if (!isNaN(maxRetentions) && maxRetentions < retention) { - core.warning(`Retention days is greater than max value allowed by repository setting, reduce retention to ${maxRetentions} days`); + const maxRetention = parseInt(maxRetentionStr) + if (!isNaN(maxRetention) && maxRetention < retention) { + core.warning( + `Retention days is greater than max value allowed by repository setting, reduce retention to ${maxRetention} days` + ) + retention = maxRetention } } - parameters.RetentionDays = retention; + parameters.RetentionDays = retention } const data: string = JSON.stringify(parameters, null, 2) From f38ac42f8e19082abd005dd7afffe3cb460696d8 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Fri, 18 Sep 2020 10:18:11 -0400 Subject: [PATCH 4/8] Add tests and addressing feedback --- packages/artifact/README.md | 5 ++-- packages/artifact/__tests__/upload.test.ts | 12 ++++++++++ packages/artifact/__tests__/util.test.ts | 14 +++++++++++ .../src/internal/upload-http-client.ts | 18 +++++---------- .../artifact/src/internal/upload-options.ts | 3 +-- packages/artifact/src/internal/utils.ts | 23 ++++++++++++++++++- 6 files changed, 58 insertions(+), 17 deletions(-) diff --git a/packages/artifact/README.md b/packages/artifact/README.md index 79f033d368..0b7cdbea4a 100644 --- a/packages/artifact/README.md +++ b/packages/artifact/README.md @@ -40,9 +40,10 @@ Method Name: `uploadArtifact` - If set to `true` and an error is encountered, the failed file will be skipped and ignored and all other queued files will be attempted to be uploaded. There will be an artifact available for download at the end with everything excluding the file that failed to upload - Optional, defaults to `true` if not specified - `retentionDays` - - Durantion after which artifact will expire in days + - Duration after which artifact will expire in days - Minimum value: 1 - - Maximum value: 90 unless changed by settings + - Maximum value: 90 unless changed by repository setting + - If this is set to a greater value than the retention settings allowed, the retention on artifacts will be reduced to match the max value allowed on server, and the upload process will continue. An input of 0 assumes default retention value. #### Example using Absolute File Paths diff --git a/packages/artifact/__tests__/upload.test.ts b/packages/artifact/__tests__/upload.test.ts index fd5ca362fa..fe8b6331ca 100644 --- a/packages/artifact/__tests__/upload.test.ts +++ b/packages/artifact/__tests__/upload.test.ts @@ -13,6 +13,7 @@ import { } from '../src/internal/contracts' import {UploadSpecification} from '../src/internal/upload-specification' import {getArtifactUrl} from '../src/internal/utils' +import {UploadOptions} from '../src/internal/upload-options' const root = path.join(__dirname, '_temp', 'artifact-upload') const file1Path = path.join(root, 'file1.txt') @@ -106,6 +107,17 @@ describe('Upload Tests', () => { ) }) + it('Create Artifact - Retention Less Than Min Value Error', async () => { + const artifactName = 'invalid-artifact-name' + const options: UploadOptions = { + retentionDays: -1 + } + const uploadHttpClient = new UploadHttpClient() + expect( + uploadHttpClient.createArtifactInFileContainer(artifactName, options) + ).rejects.toEqual(new Error('Invalid retention, minimum value is 1.')) + }) + it('Create Artifact - Storage Quota Error', async () => { const artifactName = 'storage-quota-hit' const uploadHttpClient = new UploadHttpClient() diff --git a/packages/artifact/__tests__/util.test.ts b/packages/artifact/__tests__/util.test.ts index 02930e1eb1..26d12e635b 100644 --- a/packages/artifact/__tests__/util.test.ts +++ b/packages/artifact/__tests__/util.test.ts @@ -106,6 +106,20 @@ describe('Utils', () => { } }) + it('Test negative artifact retention throws', () => { + expect(() => { + utils.getProperRetention(-1, undefined) + }).toThrow() + }) + + it('Test no setting specified take artifact retention inpput', () => { + expect(utils.getProperRetention(180, undefined)).toEqual(180) + }) + + it('Test artifact retention must conform to max allowed', () => { + expect(utils.getProperRetention(180, '45')).toEqual(45) + }) + it('Test constructing artifact URL', () => { const runtimeUrl = getRuntimeUrl() const runId = getWorkFlowRunId() diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts index 25be2db5de..b77e5fd0a2 100644 --- a/packages/artifact/src/internal/upload-http-client.ts +++ b/packages/artifact/src/internal/upload-http-client.ts @@ -18,7 +18,8 @@ import { isForbiddenStatusCode, displayHttpDiagnostics, getExponentialRetryTimeInMilliseconds, - tryGetRetryAfterValueTimeInMilliseconds + tryGetRetryAfterValueTimeInMilliseconds, + getProperRetention } from './utils' import { getUploadChunkSize, @@ -66,18 +67,11 @@ export class UploadHttpClient { // calculate retention period if (options && options.retentionDays) { - let retention = options.retentionDays const maxRetentionStr = getRetentionDays() - if (maxRetentionStr) { - const maxRetention = parseInt(maxRetentionStr) - if (!isNaN(maxRetention) && maxRetention < retention) { - core.warning( - `Retention days is greater than max value allowed by repository setting, reduce retention to ${maxRetention} days` - ) - retention = maxRetention - } - } - parameters.RetentionDays = retention + parameters.RetentionDays = getProperRetention( + options.retentionDays, + maxRetentionStr + ) } const data: string = JSON.stringify(parameters, null, 2) diff --git a/packages/artifact/src/internal/upload-options.ts b/packages/artifact/src/internal/upload-options.ts index 7baca5f789..dd952be02e 100644 --- a/packages/artifact/src/internal/upload-options.ts +++ b/packages/artifact/src/internal/upload-options.ts @@ -17,7 +17,7 @@ export interface UploadOptions { continueOnError?: boolean /** - * Durantion after which artifact will expire in days. + * Duration after which artifact will expire in days. * * By default artifact expires after 90 days: * https://docs.github.com/en/actions/configuring-and-managing-workflows/persisting-workflow-data-using-artifacts#downloading-and-deleting-artifacts-after-a-workflow-run-is-complete @@ -31,6 +31,5 @@ export interface UploadOptions { * will be reduced to match the max value allowed on server, and the upload process will continue. An * input of 0 assumes default retention setting. */ - retentionDays?: number } diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index 86b482bcef..8690c5349d 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -1,4 +1,4 @@ -import {debug, info} from '@actions/core' +import {debug, info, warning} from '@actions/core' import {promises as fs} from 'fs' import {HttpCodes, HttpClient} from '@actions/http-client' import {BearerCredentialHandler} from '@actions/http-client/auth' @@ -302,3 +302,24 @@ export async function createEmptyFilesForArtifact( await (await fs.open(filePath, 'w')).close() } } + +export function getProperRetention( + retentionInput: number, + retentionSetting: string | undefined +): number { + if (retentionInput < 0) { + throw new Error('Invalid retention, minimum value is 1.') + } + + let retention = retentionInput + if (retentionSetting) { + const maxRetention = parseInt(retentionSetting) + if (!isNaN(maxRetention) && maxRetention < retention) { + warning( + `Retention days is greater than max value allowed by the repository setting, reduce retention to ${maxRetention} days` + ) + retention = maxRetention + } + } + return retention +} From 144707fae88bd8d0430238d86bc3dfc0a2e87a54 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Fri, 18 Sep 2020 11:13:59 -0400 Subject: [PATCH 5/8] Update packages/artifact/__tests__/upload.test.ts Co-authored-by: Konrad Pabjan --- packages/artifact/__tests__/upload.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/__tests__/upload.test.ts b/packages/artifact/__tests__/upload.test.ts index fe8b6331ca..f473b2e0f2 100644 --- a/packages/artifact/__tests__/upload.test.ts +++ b/packages/artifact/__tests__/upload.test.ts @@ -108,7 +108,7 @@ describe('Upload Tests', () => { }) it('Create Artifact - Retention Less Than Min Value Error', async () => { - const artifactName = 'invalid-artifact-name' + const artifactName = 'valid-artifact-name' const options: UploadOptions = { retentionDays: -1 } From 27fe9da897bf5ba0fab41428a487c4ef877ac788 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Fri, 18 Sep 2020 11:14:08 -0400 Subject: [PATCH 6/8] Update packages/artifact/README.md Co-authored-by: Konrad Pabjan --- packages/artifact/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/README.md b/packages/artifact/README.md index 0b7cdbea4a..cdab7fdfa2 100644 --- a/packages/artifact/README.md +++ b/packages/artifact/README.md @@ -43,7 +43,7 @@ Method Name: `uploadArtifact` - Duration after which artifact will expire in days - Minimum value: 1 - Maximum value: 90 unless changed by repository setting - - If this is set to a greater value than the retention settings allowed, the retention on artifacts will be reduced to match the max value allowed on server, and the upload process will continue. An input of 0 assumes default retention value. + - If this is set to a greater value than the retention settings allowed, the retention on artifacts will be reduced to match the max value allowed on the server, and the upload process will continue. An input of 0 assumes default retention value. #### Example using Absolute File Paths From 21e06c16f0fc570c0e9669e9157c315ab55b8573 Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Fri, 18 Sep 2020 11:14:15 -0400 Subject: [PATCH 7/8] Update packages/artifact/src/internal/utils.ts Co-authored-by: Konrad Pabjan --- packages/artifact/src/internal/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts index 8690c5349d..a9f76b24be 100644 --- a/packages/artifact/src/internal/utils.ts +++ b/packages/artifact/src/internal/utils.ts @@ -316,7 +316,7 @@ export function getProperRetention( const maxRetention = parseInt(retentionSetting) if (!isNaN(maxRetention) && maxRetention < retention) { warning( - `Retention days is greater than max value allowed by the repository setting, reduce retention to ${maxRetention} days` + `Retention days is greater than the max value allowed by the repository setting, reduce retention to ${maxRetention} days` ) retention = maxRetention } From c93ce019798ab4da5303201205405dd8af18b6bb Mon Sep 17 00:00:00 2001 From: Yang Cao Date: Fri, 18 Sep 2020 11:14:24 -0400 Subject: [PATCH 8/8] Update packages/artifact/__tests__/util.test.ts Co-authored-by: Konrad Pabjan --- packages/artifact/__tests__/util.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/artifact/__tests__/util.test.ts b/packages/artifact/__tests__/util.test.ts index 26d12e635b..1513566f28 100644 --- a/packages/artifact/__tests__/util.test.ts +++ b/packages/artifact/__tests__/util.test.ts @@ -112,7 +112,7 @@ describe('Utils', () => { }).toThrow() }) - it('Test no setting specified take artifact retention inpput', () => { + it('Test no setting specified takes artifact retention input', () => { expect(utils.getProperRetention(180, undefined)).toEqual(180) })