-
Notifications
You must be signed in to change notification settings - Fork 41
feat: add built-in caching via inputs #89
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
Conversation
cache-hit: | ||
description: A boolean indicating whether the cache was hit. |
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.
This output can be used to conditionally run deno install
. E.g.:
- uses: denoland/setup-deno@v2
id: deno
with:
cache: true
- if: steps.deno.outputs.cache-hit != 'true'
run: deno install
- run: deno test
d844df8
to
b402db6
Compare
Here's a link to a PR where the Actions passes (after installing |
I've now rebased this PR on top of #95, which address remaining TODO's and issues, as long as bundling is seen as a valid addition by the Deno team. Personally, I think the improved performance and being able to use TS makes it worth it. |
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.
Very nice. LGTM!
with: | ||
cache: true | ||
cache-hash: ${{ hashFiles('**/deno.lock') }} | ||
``` |
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 wonder if maybe this should be an implicit cache: true
? Maybe something for a follow-up.
- uses: denoland/setup-deno@v2
with:
cache-hash: ${{ hashFiles('**/deno.lock') }}
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.
In addition to that, maybe doing:
with:
cache: true
should be an implicit cache-hash: ${{ hashFiles('**/deno.lock') }}
and someone can disable this by doing cache-hash: ''
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.
Discussed internally and we'd like to make this automatically hash the deno.lock by default similar to how https://github.com/Swatinem/rust-cache does it by default. I'll open a PR that adds this shortly after merging this one then do a release.
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.
Ahh, yeah that could be nice; if cache-hash
is set, cache: true
is implied. I can for sure do a follow-up PR with that 😃
Note worth mentioning, cache-hash
is not mandatory. I just had it in the README.md as I think it's best practice. So it's possible to just use cache: true
. The primary cache key should then become `deno-cache-${RUNNER_OS}-${RUNNER_ARCH}-${GITHUB_JOB}-`
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.
Discussed internally and we'd like to make this automatically hash the deno.lock by default similar to how https://github.com/Swatinem/rust-cache does it by default. I'll open a PR that adds this shortly after merging this one then do a release.
Ahh, sure. Sounds like a good idea. I just looking at actions/setup-node
, and did not look much at others. Improving defaults and having less boilerplate sounds great!
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.
Added here: #98
Thanks for fixing the remaining lockfile issues and more @dsherret, really glad to have this merged! It should simplify our GitHub Action config a lot 🎉 |
(cherry picked from commit fd6b0ad)
The `denoland/setup-deno` action recently added built-in caching (denoland/setup-deno#89, denoland/setup-deno#98). This makes it a lot simpler to add caching of the `DENO_DIR` during CI runs, without needing `@actions/cache` manual setup. This should speed up CI runs by avoiding downloading dependencies between every run.
This PR adds support for built-in caching to
denoland/setup-deno
. I tried to take inspiration from howactions/setup-node
did things, but the code here can be way simpler because we're only supporting downloads by Deno and the default cache location orDENO_DIR
.Cache keys
restoreKey
:deno-cache-${env.RUNNER_OS}-${env.RUNNER_ARCH}
${restoreKey}-${env.GITHUB_JOB}-${input.cache-hash}
One difference compared to the existing
setup-node
action, is that I includedGITHUB_JOB
in the cache key. This means it's possible to restore a cache from another job, but each job will have it's own cache key.The benefit of including the job in the cache key, is we only download necessary files. For instance,
test
may download@std/testing
, while other jobs do not. We also avoid corrupted caches, if e.g. abuild
job installs all dependencies except testing dependencies, then the cache will miss files needed by atest
job, even while using the same cache key.This is no issue for other package managers in NodeJS, which installs all packages. But with Deno, it's possible to install packages/files automatically while running a program. The user of this action can solve that themselves though, by adding to the
cache-hash
input (e.g.cache-hash: test-${{ hashFiles(...) }}
).cache-hash
input for more granular cachesbuild
/test
jobs have the same cache key, thus keeps downloading dependencies for one or the otherUsage comparison
Given the requirements below, here's before/after compared to when being able to use the
setup-deno
action with changes in this PR:DENO_DIR
filesrestore-keys
for the following benefits:Making caching of installed dependencies built-in makes it a lot easier and less error prone to get great caching with 1 or 2 extra inputs to
denoland/setup-deno
.Closes #31