-
Notifications
You must be signed in to change notification settings - Fork 146
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
Support to retain cache #211
base: master
Are you sure you want to change the base?
Conversation
c54303c
to
63216ab
Compare
9ce6fa8
to
3e3b65a
Compare
Hey @crazy-max , can I get a review on this one? |
/cc @jedevc |
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.
Sorry for the delay. I left some comments.
Also have you tested this change on your side? Keeping the BuildKit state will only work on persistent runners (self-hosted) and not public ones that are ephemeral. If this is your intent then it should be documented to avoid confusion.
src/context.ts
Outdated
export async function getBuilderName(driver: string): Promise<string> { | ||
return driver == 'docker' ? await Docker.context() : `builder-${uuid.v4()}`; | ||
export function getBuilderName(customName: string, driver: string): string { | ||
if (customName && customName.length > 0) { |
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.
if (customName && customName.length > 0) { | |
if (customName) { |
src/state-helper.ts
Outdated
@@ -7,6 +7,7 @@ export const builderDriver = process.env['STATE_builderDriver'] || ''; | |||
export const containerName = process.env['STATE_containerName'] || ''; | |||
export const certsDir = process.env['STATE_certsDir'] || ''; | |||
export const cleanup = /true/i.test(process.env['STATE_cleanup'] || ''); | |||
export const keepState = process.env['STATE_keepState'] || false; |
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.
export const keepState = process.env['STATE_keepState'] || false; | |
export const keepState = !!process.env['STATE_keepState']; |
action.yml
Outdated
description: 'Keep state on cleanup' | ||
default: 'false' | ||
required: false | ||
custom-name: |
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'm not sure about naming this custom-name
, this is confusing. Better to follow the buildx UX of the create command and use name
.
Signed-off-by: Balaji Arun <[email protected]>
3e3b65a
to
ff70196
Compare
Thanks for the review @crazy-max. I've addressed your feedback. We have been using this change at aptos-labs/aptos-core for over a month now with persistent self-hosted runners and have seen speed ups of up to 50% in build times. |
@crazy-max can we get this merged please? |
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.
Sorry got pending reviews I forgot to submit 😣
It would be better if we have another PR for the name
input being added.
} | ||
|
||
export async function getInputs(): Promise<Inputs> { | ||
return { | ||
version: core.getInput('version'), | ||
name: await getBuilderName(core.getInput('driver') || 'docker-container'), | ||
name: getBuilderName(core.getInput('name'), core.getInput('driver') || 'docker-container'), |
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.
name
needs to be defined in https://github.com/docker/setup-buildx-action/blob/master/action.yml. Also needs to update README to add this new input.
Any movement on this? |
just stumpled upon this, the what is needed to get any movement on this? |
@ibalajiarun @crazy-max can we take another look at this? Would be great to have. |
This PR might be the missing piece of my ultimate caching pipeline quest. Any chance it will be merged in the near future? |
This PR adds support to keep the cache state when the builder is removed in the post-action step. For certain usecases such as CI builds, it is helpful to keep the cache and reuse it with re-created builders. For this purpose, there is a new
keep-state
optional actions flag that defaults tofalse
.To support cache reuse, this PR also allows specifying a custom name to the builder, since cache reuse is no possible with the exiting random names generated using
uuid.v4()
. For this purpose, there is a newname
optional actions flag.