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

Add cache fallback with rolling timed expiration #702

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

voxpelli
Copy link

This PR builds on #323 by @mmorel-35 but with two changes:

  1. It adds a new cache-invalidate-after-days option that defaults to 120 days and which sets a rollable prefix between 0 and 9 to all of the cache keys, making it so that a cache key will change after 120 days. Can be set to 0 to deactivate. Reason for this is that npm will not remove data by itself: the cache will grow as new packages are installed.
  2. Removes the fallback to a package manager independent cache key, only falling back to a cache from the same package manager.

This fixes #328 without creating an ever expanding cache size.

This also helps the cache be much more useful with a Dependabot / Renovatebot setup as a project that is dominated by PR:s from such sources will with the current cache setup in actions/setup-node pretty much only get cache misses and thus only ever set caches, rarely ever use them.

This adds a simple mechanism of falling back to a recent cache while avoiding the ever growing cache problem.

Feedback wanted:

  • The rolling prefix might be a premature optimization on my part. Maybe one should just leave it out)

@voxpelli voxpelli requested a review from a team as a code owner February 27, 2023 18:27
@voxpelli
Copy link
Author

Will rebase

voxpelli pushed a commit to SocketDev/workflows that referenced this pull request Feb 28, 2023
Similar to what is added in actions/setup-node#702, prevents the npm cache from growing indefinitely while still providing a general fallback
@nwalters512
Copy link

@voxpelli thanks for opening this! I think you'll need to run npm build/yarn build to update the files in dist/ for this to pass CI.

@voxpelli
Copy link
Author

@nwalters512 Did so now, waiting for workflow runs to be approved again :)

@nwalters512
Copy link

@dmitry-shibanov any chance you could take a look at this PR? The introduction of fallback restore keys matches documented best practices and should allow the cache to be used in many more scenarios.

@voxpelli
Copy link
Author

This has conflicts after #744 was merged, I'm on it

@voxpelli
Copy link
Author

Ping @dsame, I adapted this to the changes made in #744, does it look like its in the spirit of what you did there?

Copy link

@Mikhai56 Mikhai56 left a comment

Choose a reason for hiding this comment

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

of

@voxpelli
Copy link
Author

voxpelli commented Jul 1, 2024

Anything I can do to help out on getting this reviewed?

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.

Caching fallback if there is no exact hit
3 participants