-
Notifications
You must be signed in to change notification settings - Fork 24
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
#27 cache the volta inventory to reduce times tools need to be downloaded #137
base: master
Are you sure you want to change the base?
Conversation
Oh no! I'm very sorry for missing this PR. Reviewing now... |
@@ -19,7 +19,7 @@ | |||
"scripts": { | |||
"build": "npm-run-all build:clean build:ts build:dist build:docs", | |||
"build:clean": "rimraf dist lib", | |||
"build:dist": "ncc build src/index.ts", | |||
"build:dist": "ncc build src/index.ts && ncc build src/cache.ts -o dist-post", |
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.
Let's split this into separate tasks, then use npm-run-all
in build:dist
. Something like:
"build:dist": "ncc build src/index.ts && ncc build src/cache.ts -o dist-post", | |
"build:dist": "npm-run-all build:dist:*", | |
"build:dist:main": "ncc build src/index.ts", | |
"build:dist:post": "ncc build src/cache.ts -o dist-post", |
@@ -19,7 +19,7 @@ | |||
"scripts": { | |||
"build": "npm-run-all build:clean build:ts build:dist build:docs", | |||
"build:clean": "rimraf dist lib", |
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.
Can you add dist-post
here also?
import * as core from '@actions/core'; | ||
import * as stateKeys from './state-keys'; | ||
|
||
async function run(): Promise<void> { |
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.
Lets call this main
(it makes more sense in my head as main
, since it's basically just the mechanism to execute this script itself and still be able to use async
/await
).
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.
Maybe we can call this post.ts
?
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 we call the other file post.ts
, we can call this one src/cache.ts
, what do you think?
); | ||
} | ||
|
||
function createToolFilePatter(tool: Tool): string { |
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.
function createToolFilePatter(tool: Tool): string { | |
function createToolFilePattern(tool: Tool): string { |
@@ -0,0 +1 @@ | |||
export const VOLTA_HOME = 'VOLTA_HOME'; |
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'd rather just use the string where we need it (instead of using this constant).
} | ||
} | ||
|
||
async function cleanInventory(voltaHome: string, installedTools: Tool[]): Promise<void> { |
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 don't fully grok how this is going to work to actually prune the inventory. If we call restoreInventory
in the setup flow of the main action all of the tools & inventory will be copied over from prior runs, then (eventually) call cleanInventory
in the post
script for the action -- how will we ever prune things out?
Can you walk me through the high level details?
Fixes #27
This uses the
@actions/cache
package provided to cache the inventory (cache) of Volta. So that installing the tools needed for the build can work even if the download would be down.I was triggered to do this by the instability of the node-download-servers in resent weeks.
Since I have no expertise in how exactly Volta works internally and therefore am not 100% sure if this is enough for Volta to work with, I would greatly appreciate feedback.
Ideas on how to cover this with tests, are also very welcome.
Inner workings:
I try to clean up the Volta inventory during teardown and only keep the tools that where actually installed during the run. When uploading the cache I create a cash-key based on the content of the inventory, so that a change in it's content also results in new and updated cache entry.
Limitations right now:
As far as I can tell, the cache is shared across all workflows in a repository. Therefore a project with pipelines all installing different tools might lead to continuous outdated cache entries, since the newest one will be pulled during setup, no matter the hash-suffix.