-
Notifications
You must be signed in to change notification settings - Fork 323
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 support for dependency caching #2383
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.
Looks good so far, some initial comments:
// with an empty string. | ||
const globber = await makeGlobber(cacheConfig.hash); | ||
|
||
if ((await globber.glob()).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.
Does this mean we list files twice (once here and once in cacheKey
)? Might not be a performance problem in practice.
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.
Yes, it does. I looked at this before your review but, unfortunately, the hashFiles
implementation in @action/glob
isn't exposed in a way that we can just throw an existing array of paths at it, so we'd have to copy the implementation (or a variant of it, depending on how much we care about the intricacies of theirs).
04e457d
to
c07c5b0
Compare
a2bb99e
to
ff98043
Compare
c898cb9
to
ad48d53
Compare
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, this LGTM to start shipping disabled by default! Some final minor suggestions.
ba64dbe
to
bab8f1d
Compare
@henrymercer I have addressed your comments and rebased the PR branch on |
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, looking forward to seeing the results of this internally!
Work-in-progress to add support for dependency caching to the
init
Action.Merge / deployment checklist