Add support for dependency caching - #2383
mbg merged 24 commits into
Conversation
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Looks good so far, some initial comments:
Sorry, something went wrong.
| // 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.
Sorry, something went wrong.
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).
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Thanks, this LGTM to start shipping disabled by default! Some final minor suggestions.
Sorry, something went wrong.
|
@henrymercer I have addressed your comments and rebased the PR branch on main to resolve the merge conflicts. 21e6a62 onwards are the changes in response to your comments. |
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Thanks, looking forward to seeing the results of this internally!
Sorry, something went wrong.
Work-in-progress to add support for dependency caching to the init Action.
Merge / deployment checklist