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

Add an option to specify retention days for artifacts #575

Merged
merged 8 commits into from
Sep 18, 2020
Merged
5 changes: 5 additions & 0 deletions packages/artifact/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ 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`
- 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 the server, and the upload process will continue. An input of 0 assumes default retention value.

#### Example using Absolute File Paths

Expand Down
12 changes: 12 additions & 0 deletions packages/artifact/__tests__/upload.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -106,6 +107,17 @@ describe('Upload Tests', () => {
)
})

it('Create Artifact - Retention Less Than Min Value Error', async () => {
const artifactName = 'valid-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()
Expand Down
14 changes: 14 additions & 0 deletions packages/artifact/__tests__/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,20 @@ describe('Utils', () => {
}
})

it('Test negative artifact retention throws', () => {
expect(() => {
utils.getProperRetention(-1, undefined)
}).toThrow()
})

it('Test no setting specified takes artifact retention input', () => {
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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,7 @@ export function getRuntimeUrl(): string {
export function getWorkFlowRunId(): string {
return '15'
}

export function getRetentionDays(): string | undefined {
Copy link
Contributor

@konradpabjan konradpabjan Sep 17, 2020

Choose a reason for hiding this comment

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

I would suggest adding a test that uses this mocked value.

The entire POST call to create an artifact is mocked so you can use the existing code to simulate a response from the actions service and add some verification to see if the correct value is going through: https://github.com/actions/toolkit/blob/main/packages/artifact/__tests__/upload.test.ts#L373:L410

Here is an existing test that you can use as reference: https://github.com/actions/toolkit/blob/main/packages/artifact/__tests__/upload.test.ts#L81:L83

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! I refactored the code to make unit tests easier. Now all the logic to compute the retention value is moved to a util function so we can test it independently.

return '45'
}
3 changes: 2 additions & 1 deletion packages/artifact/src/internal/artifact-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
4 changes: 4 additions & 0 deletions packages/artifact/src/internal/config-variables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,7 @@ export function getWorkSpaceDirectory(): string {
}
return workspaceDirectory
}

export function getRetentionDays(): string | undefined {
return process.env['GITHUB_RETENTION_DAYS']
yacaovsnc marked this conversation as resolved.
Show resolved Hide resolved
}
1 change: 1 addition & 0 deletions packages/artifact/src/internal/contracts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export interface ArtifactResponse {
export interface CreateArtifactParameters {
Type: string
Name: string
RetentionDays?: number
}

export interface PatchArtifactSize {
Expand Down
19 changes: 16 additions & 3 deletions packages/artifact/src/internal/upload-http-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ import {
isForbiddenStatusCode,
displayHttpDiagnostics,
getExponentialRetryTimeInMilliseconds,
tryGetRetryAfterValueTimeInMilliseconds
tryGetRetryAfterValueTimeInMilliseconds,
getProperRetention
} from './utils'
import {
getUploadChunkSize,
getUploadFileConcurrency,
getRetryLimit
getRetryLimit,
getRetentionDays
} from './config-variables'
import {promisify} from 'util'
import {URL} from 'url'
Expand Down Expand Up @@ -55,12 +57,23 @@ 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<ArtifactResponse> {
const parameters: CreateArtifactParameters = {
Type: 'actions_storage',
Name: artifactName
}

// calculate retention period
if (options && options.retentionDays) {
const maxRetentionStr = getRetentionDays()
parameters.RetentionDays = getProperRetention(
options.retentionDays,
maxRetentionStr
)
}

const data: string = JSON.stringify(parameters, null, 2)
const artifactUrl = getArtifactUrl()

Expand Down
17 changes: 17 additions & 0 deletions packages/artifact/src/internal/upload-options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,21 @@ export interface UploadOptions {
*
*/
continueOnError?: boolean

/**
* 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
*
* 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
}
23 changes: 22 additions & 1 deletion packages/artifact/src/internal/utils.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -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 the max value allowed by the repository setting, reduce retention to ${maxRetention} days`
)
retention = maxRetention
}
}
return retention
}