Shorten cache key? #486

Activity

1. eregon commented on Mar 22, 2023

   

   eregon

   on Mar 22, 2023

   Member

   More actions

   The difference is not including the working directory, right? I'd rather keep it explicit for simplicity and reliability.
2. MSP-Greg commented on Mar 22, 2023

   

   MSP-Greg

   on Mar 22, 2023

   CollaboratorAuthor

   More actions

   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...
3. eregon commented on Mar 22, 2023

   

   eregon

   on Mar 22, 2023

   Member

   More actions

   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.
4. MSP-Greg commented on Mar 22, 2023

   

   MSP-Greg

   on Mar 22, 2023

   CollaboratorAuthor

   More actions

   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...
5. eregon commented on Mar 24, 2023

   

   eregon

   on Mar 24, 2023

   Member

   More actions

   Sounds good, can you make a PR with it?
6. MSP-Greg commented on Mar 24, 2023

   

   MSP-Greg

   on Mar 24, 2023

   Last edited by MSP-Greg

   CollaboratorAuthor

   More actions

   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.
7. eregon commented on Mar 24, 2023

   

   eregon

   on Mar 24, 2023

   Last edited by eregon

   Member

   More actions

   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.
8. MSP-Greg commented on Mar 24, 2023

   

   MSP-Greg

   on Mar 24, 2023

   CollaboratorAuthor

   More actions

   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.
9. MSP-Greg commented on Mar 25, 2023

   

   MSP-Greg

   on Mar 25, 2023

   CollaboratorAuthor

   More actions

   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.
10. eregon commented on Dec 28, 2023

    

    eregon

    on Dec 28, 2023

    Member

    More actions

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

    closed this as completedon Dec 28, 2023