Skip to content
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

Custom Cache key #286

Closed
mbrevda opened this issue Jul 4, 2021 · 13 comments
Closed

Custom Cache key #286

mbrevda opened this issue Jul 4, 2021 · 13 comments
Labels
enhancement New feature or request

Comments

@mbrevda
Copy link

mbrevda commented Jul 4, 2021

The new cache option is great, but is also tightly coupled to the lock file hash. While this is a great way to ensure the cache is always a 1:1 match with the lock file, it ultimately makes the case a lot less useful in often updating, multiple branch setups - such as with dependabot. The ability to define a custom cache key would allow for longer term caches which with a wider ranges of package versions cached. To combat snowballing growth of the cache, the key could be set to include the current month, ensuing the cache is reset one a month.

@dmitry-shibanov dmitry-shibanov added the enhancement New feature or request label Jul 5, 2021
@dmitry-shibanov
Copy link
Contributor

Hello @mbrevda. Thank you for your report. I'll mark your issue as an enhancement. We'll investigate your proposal.

@lharress
Copy link

lharress commented Jul 5, 2021

or even the option to set custom restore keys. We use a rolling cache as our fallback when the hash of the lock file does not have a hit. Makes a considerable difference running install from few day old cache rather than from scratch.

I'd be happy to put together a PR?

@roikoren755
Copy link

We have a different issue that will benefit from this enhancement, or from a separate solution.

We are running a matrix of flows, with the node version changing between each flow.
Because of the way the key is built, the node version has no influence on the cache key, and all of the flows try and upload their node_modules cache using the same key, with only one of them succeeding.

The node_modules to be uploaded are also different between the node versions, with packages running different compilation depending on the version, and the way things are now, that isn't taken into consideration.

Allowing us to provide a custom cache key will solve these issues for us, but a simpler to implement fix for our issue would be to add the node version used to the cache key.

@dmitry-shibanov
Copy link
Contributor

Hello everyone. By our investigation we've decided not to add support of custom cache keys. The role of caching for node is to cover the majority of use cases without deep customization. It'll help novice users to setup faster their yml files. If you need more custom logic, the preferred solution will be using the action/cache task.

@roikoren755,
The current implementation of caching with common key will work for your use-case and matrix with different Node.js versions.
We don’t cache node_modules folder (it could cause a bunch of other problems too). Instead, we cache global npm / yarn cache on machine ($HOME/.npm or $HOME/.yarn). It means that post_install steps for packages are not cached and cache can be reused by different Node.js versions. It is approach recommended by actions/cache

@mbrevda
Copy link
Author

mbrevda commented Jul 13, 2021

Is adding another param really that big of a deal?

@lharress
Copy link

lharress commented Jul 13, 2021

Could we at the very least have the recommended implementation from https://github.com/actions/cache? All the node examples recommend a restore key and with good reason. Restoring from something is much faster than restoring from nothing.

https://github.com/actions/cache/blob/main/examples.md#node---npm
https://github.com/actions/cache/blob/main/examples.md#node---yarn

@maxim-lobanov
Copy link
Contributor

@mbrevda , from ADR

We don't pursue the goal to provide wide customization of caching in scope of actions/setup-node action. The purpose of this integration is covering ~90% of basic use-cases. If user needs flexible customization, we should advice them to use actions/cache directly.

Customizing of caching key is definitely not basic use-case so we should use actions/cache directly for this use-case. We don't want to duplicate functionality of actions/cache in scope of actions/setup-node.

@lharress , restore-keys doesn't make any sense for setup-node integration of caching.

restore-keys is an ordered list of alternative keys to use for finding the cache if no cache hit occurred for primary-key.

Example in actions/cache repo recommends to set restore-keys to ${{ runner.os }}-node-. We never push cache for this key so the cache for this key will always be empty and won't be restored.

@lharress
Copy link

lharress commented Jul 13, 2021

@maxim-lobanov I do not believe that is the case, if it is then the documentation is misleading.

https://docs.github.com/en/actions/guides/caching-dependencies-to-speed-up-workflows#matching-a-cache-key

You can provide a list of restore keys to use when there is a cache miss on key. You can create multiple restore keys ordered from the most specific to least specific. The cache action searches for restore-keys in sequential order. When a key doesn't match directly, the action searches for keys prefixed with the restore key. If there are multiple partial matches for a restore key, the action returns the most recently created cache.

Specifically, this piece;

When a key doesn't match directly, the action searches for keys prefixed with the restore key.

Unless I am misunderstanding, this mens if I have 1 item in the cache with the key node-cache-one and then I attempt a restore using the primary key node-cache-two and a restore key node-cache- it will cause a cache miss on the primary key. It will then search for matching restore keys, miss those as well, proceed to prefixes of the restore keys and finally find node-cache-one. Since it is the most recently created key (in this scenario) it will restore from there. It will still be a cache miss allowing the workflow to handle the scenario accordingly and then save the correct key for a future action as part of the post @actions/cache process.

Better explained with an example here - https://docs.github.com/en/actions/guides/caching-dependencies-to-speed-up-workflows#example-using-multiple-restore-keys

@maxim-lobanov
Copy link
Contributor

@lharress let me follow up with my team regarding it and return back with answer

@lharress
Copy link

lharress commented Aug 2, 2021

@maxim-lobanov Any update on this? Could the issue be re-opened while it's being discussed?

@Klohto
Copy link

Klohto commented Oct 26, 2021

Can we have this reopened or just updated? The presented arguments are strong enough and warrant a wider discussion.

@ngraef
Copy link

ngraef commented Nov 2, 2021

Related: #328 requests the addition of a default restore key.

nigelzor added a commit to thrivehealth/setup-node that referenced this issue Dec 8, 2021
it's a lot faster to start from a 90% correct cache than an empty one

re: actions#286
re: actions#323
re: actions#328
nigelzor added a commit to thrivehealth/setup-node that referenced this issue Feb 28, 2023
it's a lot faster to start from a 90% correct cache than an empty one

re: actions#286
re: actions#323
re: actions#328
@dbrugger
Copy link

dbrugger commented Sep 1, 2023

I would also like this. My use case is when changing the yarn setup from a local repo to a global cache, the cache needs to be busted.
The lock file does not change in this case - does someone know a good way to get it changed without changing packages?
An alternative to a completely custom cache key could be a custom pre- or suffix.

deining pushed a commit to deining/setup-node that referenced this issue Nov 9, 2023
…actions#286)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 4.30.0 to 4.31.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v4.31.0/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants