-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great 👍 Just a few small/nit issues.
@@ -45,3 +45,7 @@ export function getRuntimeUrl(): string { | |||
export function getWorkFlowRunId(): string { | |||
return '15' | |||
} | |||
|
|||
export function getRetentionDays(): string | undefined { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
One more thing, we will need to release a new version of the package so you need to update
|
I want to have the release related change separately in another PR. This way we can refer back to a single PR to see what needs to be done for releasing artifact in the future? Thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the tests! Just a few more small suggestions.
A separate PR for updating the release works! 🚀
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
* Add an option to specify retention days for artifacts * Validate against settings exposed as env var to give early feedback * Fix lint * Add tests and addressing feedback * Update packages/artifact/__tests__/upload.test.ts Co-authored-by: Konrad Pabjan <konradpabjan@github.com> * Update packages/artifact/README.md Co-authored-by: Konrad Pabjan <konradpabjan@github.com> * Update packages/artifact/src/internal/utils.ts Co-authored-by: Konrad Pabjan <konradpabjan@github.com> * Update packages/artifact/__tests__/util.test.ts Co-authored-by: Konrad Pabjan <konradpabjan@github.com> Co-authored-by: Konrad Pabjan <konradpabjan@github.com>
Introducing an option to reduce the default retention period for uploaded artifacts.
Today all artifacts have a 90 days retention value and there is no knob to reduce the value. We are revamping the experience so we can set custom expiry on individual artifact.
The new minimum value is
1
day.