Skip to content

feat: include a hash of deno.lock files in the cache key automatically #98

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

Merged
merged 10 commits into from
May 13, 2025

Conversation

dsherret
Copy link
Member

No description provided.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear to me from the readme that there is a restore key and a cache key, and which parts of the cache key are part of which of these keys.

@csvn
Copy link
Contributor

csvn commented May 12, 2025

It is not clear to me from the readme that there is a restore key and a cache key, and which parts of the cache key are part of which of these keys.

A cache is not like an artifact, it can be missing or a restored cache that does not match the primary key. The cache-hit output will only match when the cache is restored from the primary key.

Maybe we can add to the README.md that it's only OK to skip deno install when primary cache key hits?

  - uses: denoland/setup-deno@v2
    id: deno
    with:
      cache: true
  - if: steps.deno.outputs.cache-hit != 'true'
    run: deno install

Using the restore key from cache should at worst use a cache with some dependencies that are not used, and the deno install command and post job should then create a new cache. I adapted the existing code from @actions/setup-node which did not explicitly state what the resulting keys are in the README (but they can be seen in the job log).

@dsherret
Copy link
Member Author

dsherret commented May 12, 2025

It is not clear to me from the readme that there is a restore key and a cache key, and which parts of the cache key are part of which of these keys.

I'm not sure it's useful for someone to know about this? At a high level (from my understanding), the setup-deno action just caches their dependencies at a single key and if they want further granularity they can use @actions/cache, but from my experience setting that up is a PITA to do properly, which is what led me to looking if there was a PR for built-in caching.

@dsherret
Copy link
Member Author

 - if: steps.deno.outputs.cache-hit != 'true'
   run: deno install

One reason I'm hesitent to add an example doing this, is that deno install should be very fast when using caching and it might be setting up a node_modules directory when not using only the global cache.

@csvn
Copy link
Contributor

csvn commented May 12, 2025

One reason I'm hesitent to add an example doing this, is that deno install should be very fast when using caching and it might be setting up a node_modules directory when not using only the global cache.

Yeah, this could indeed be described as an advanced pattern (opt-in). So not promoting it sounds reasonable.

The reason I thought including the job-name as part of the cache key is that when not using deno install and just running deno run or deno test, then different sets of dependencies may be needed and thus smaller and granular caches are created.

I think there's pros and cons to both, if every job does deno install then including the job-name as part of the cache key is wasted (creates more unnecessary caches). So in the end it depends a bit on what kind of project it is.

@csvn
Copy link
Contributor

csvn commented May 12, 2025

Sorry if I was annoying just jumping in here with my feedback. Just excited to see build-in action caching landed after #89 being merged. Thought it might be a good idea to upgrade my workflow files first after this PR and hopefully a v2 tag update. 😅

@dsherret dsherret requested a review from lucacasonato May 13, 2025 13:07
Copy link
Contributor

@csvn csvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My approval doesn't really matter but thought I could chime in since I worked on #89, but looks great to me! Looking forward to this feature! 🎉

Co-authored-by: Bartek Iwańczuk <[email protected]>
@dsherret dsherret merged commit 3169cf9 into main May 13, 2025
37 checks passed
@dsherret dsherret deleted the feat_hash_deno_lock_automatically branch May 13, 2025 16:53
crowlKats pushed a commit that referenced this pull request May 15, 2025
marvinhagemeister pushed a commit to denoland/fresh that referenced this pull request May 16, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants