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

optim: add secondary cache key if lock hash doesn't exist #43

Closed
wants to merge 1 commit into from

Conversation

agilgur5
Copy link
Contributor

Description

Tags

Doesn't quite resolve, but should make things like #28 happen less. #26 would still be good to solve

Review Notes

Unclear why the primary key is also used duplicatively as a restore key?

- follows `restore-keys` example from
  https://docs.github.com/en/actions/configuring-and-managing-workflows/caching-dependencies-to-speed-up-workflows#example-using-the-cache-action
  and https://github.com/actions/cache/blame/59a8d125e7227efbef01813bf1ecf30fa8b6bd8c/examples.md#L234
  - these specify to use the key with everything but the lock hash in
    case the specific cache with the lock hash doesn't exist
    - the thought being that a partial cache is better than no cache
@agilgur5
Copy link
Contributor Author

@bahmutov any word on this? This PR's been up for about a month now.

You didn't add me as a maintainer after #37 (comment), so I can't merge this myself.
I'm looking to add this action to our templates for TSDX, so write access would be nice to fix any issues that arise if you're not actively maintaining this yourself.

@agilgur5
Copy link
Contributor Author

@bahmutov following up again

@agilgur5
Copy link
Contributor Author

agilgur5 commented Oct 9, 2020

@bahmutov following up a third time...

@agilgur5
Copy link
Contributor Author

agilgur5 commented Oct 15, 2020

@bahmutov following up for a fourth time... Nearly 2 months without response now...

It seems like this action is unmaintained (issues not responded to for many months) and I'm having to ping several times again even for simple, well-documented PRs. Looks like I'll have to keep a fork then

@bahmutov
Copy link
Owner

bahmutov commented Nov 9, 2020

Question: will it allow https://glebbahmutov.com/blog/do-not-let-npm-cache-snowball/ ?

@agilgur5
Copy link
Contributor Author

@bahmutov It depends on when your cache is evicted, but yes it can cause that. NPM's cache is basically designed to grow infinitely with no eviction, but you can run npm cache clean --force if it grows too large for one's liking.
Dependencies don't change that frequently, or at least not all dependencies, so I'm not sure the performance hit of not caching is worthwhile. It seems like it would make more sense to periodically clean if dependencies have changed, just as one would do locally.
Yarn's cache allows one to evict specific modules.
GitHub Actions will also clear cache after 7 days of not being used.

If you believe the official recommendation GitHub has made is faulty, I would suggest filing an issue with @actions/cache or the Actions docs similar to what I suggested in jaredpalmer/tsdx#799 / actions/cache#400. I would think they made that recommendation of restore-keys knowingly and may have a good rationale for that; it would affect a lot of people if changed.

@bahmutov
Copy link
Owner

I disagree. I have no idea how often my project will be tested on CI, especially when there are automatic dependency updates. Plus I do not trust the cache clean method to work reliably - and should I run it after every install? Or sometimes?

Why increase the complexity when a simple "start from scratch" is enough?

@bahmutov bahmutov closed this Nov 13, 2020
@agilgur5
Copy link
Contributor Author

Looks like this was eventually replaced by #73 , which follows a very similar rationale as mine here and adds a periodic clean that I also suggested here to automatically occur within the Action.

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.

2 participants