Conversation
henrymercer
left a comment
There was a problem hiding this comment.
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.
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.
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
henrymercer
left a comment
There was a problem hiding this comment.
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 |
henrymercer
left a comment
There was a problem hiding this comment.
Thanks, looking forward to seeing the results of this internally!
Work-in-progress to add support for dependency caching to the
initAction.Merge / deployment checklist