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

Shorten cache key? #486

Closed
MSP-Greg opened this issue Mar 21, 2023 · 10 comments
Closed

Shorten cache key? #486

MSP-Greg opened this issue Mar 21, 2023 · 10 comments

Comments

@MSP-Greg
Copy link
Collaborator

MSP-Greg commented Mar 21, 2023

Below is a current cache key, which is 179 characters:

setup-ruby-bundler-cache-v5-ubuntu-20.04-x64-ruby-3.2.1-wd-/home/runner/work/puma/puma-with--without--Gemfile.lock-303ee77f72da24cc1a1a294e0bd11ffbd39f28ed6ffa7baea81ba8ed6f202e80

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:

setup-ruby-bundler-cache-v5-ubuntu-20.04-x64-ruby-3.2.1-303ee77f72da24cc1a1a294e0bd11ffbd39f28ed6ffa7baea81ba8ed6f202e80

See any problems with that?

@eregon
Copy link
Member

eregon commented Mar 22, 2023

The difference is not including the working directory, right? I'd rather keep it explicit for simplicity and reliability.

@MSP-Greg
Copy link
Collaborator Author

Almost. If all are defaults, -wd-/home/runner/work/#{repo_name}/#{repo_name}-with--without- is removed. The key is a lot more 'human-readable' without, and I suspect many users are not changing the defaults. Not that important...

@eregon
Copy link
Member

eregon commented Mar 22, 2023

Right. I'd be fine to not include -with and -without if they are unset.

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.

@MSP-Greg
Copy link
Collaborator Author

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...

@eregon
Copy link
Member

eregon commented Mar 24, 2023

Sounds good, can you make a PR with it?

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Mar 24, 2023

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 @actions/cache for whether the 'verification' key changed, not sure.

@eregon
Copy link
Member

eregon commented Mar 24, 2023

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.
And I would definitely not bundle this change with another cache change, so it's much easier to find out which one is problematic is one of them would be.

@MSP-Greg
Copy link
Collaborator Author

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.

@MSP-Greg
Copy link
Collaborator Author

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.

@eregon
Copy link
Member

eregon commented Dec 28, 2023

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.

@eregon eregon closed this as completed Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants