-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
Shorten cache key? #486
Comments
The difference is not including the working directory, right? I'd rather keep it explicit for simplicity and reliability. |
Almost. If all are defaults, |
Right. I'd be fine to not include The hash is not so readable anyway and the beginning is the only readable part, so I'd keep the working dir to keep things simple. |
The code I have right now changes the following (not contiguous lines): if (bundleWith !== '') { key += `-with-${bundleWith}` }
if (bundleWithout !== '') { key += `-without-${bundleWithout}` }
if (cacheVersion !== DEFAULT_CACHE_VERSION) { key += `-v-${cacheVersion}` }
if (lockFile !== 'Gemfile.lock') { key += `-${lockFile}` } Pretty simply code... |
Sounds good, can you make a PR with it? |
If we blow everyone's keys, should we also upgrade @actions/cache? No one has reacted to actions/toolkit#1378, and although I would consider it a breaking change, others might say that consumers should account for changes in objects passed to its functions. Thoughts? EDIT: I don't think we should invalidate everyone's caches without good reason. Hence, should we add this to PR #485 for the next release? Upping our cache version seemed like a good idea. I could review the code at |
I don't think we should be afraid to change the key, caches are anyway deleted after 2 weeks IIRC or if the ABI version changes, etc, it's no big deal. |
To clarify, I'm not suggesting the same commit and/or PR, just the same release. I doubt updating @actions/cache with PR #485 will cause any problems, and it protects our object (paths). Re @actions/cache, the release page shows several fixes, and the downloads page seems to show that most people are using it. |
Above I linked to items about @actions/cache. Looking around further, there are very few other 'setup' actions using the current version, many are using 3.0.x, as we are here. But, the current version of actions/cache (3.3.1) is using @actions/cache 3.2.1, see its package-lock.json. It is used 17 times in ruby/ruby workflows. I suspect many repos are updating it automatically with dependabot. Hence, I don't think we need to worry about updating @actions/cache to 3.2.1, given its widespread use in actions/cache. |
It seems best to keep this logic as simple and explicit as possible for the key, unless the key length would cause some issue in practice, so I'll close this. |
Below is a current cache key, which is 179 characters:
It could be shortened by removing keys that are considered the 'default values', which would then have a key as shown below, which is 120 characters:
See any problems with that?
The text was updated successfully, but these errors were encountered: