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

Make cache key timestamping optional. #126

Merged
merged 2 commits into from
Jan 8, 2023

Conversation

fruffy
Copy link
Contributor

@fruffy fruffy commented Jan 6, 2023

Prompted by #117.

For Github repositories with many different actions and concurrent PRs, ccache-action's timestamps can quickly fill up the cache and evict other cached entries. This PR adds an option to disable timestamping.

I am not familiar with either Javascript or Github Actions, so suggestions on how to improve this are very much welcome.

@ppizarror
Copy link

We have the same problem! Having this option might help us a lot @hendrikmuhs

@hendrikmuhs
Copy link
Owner

Looks good to me, instead of useTimestamp I suggest appendTimestamp.

@hendrikmuhs
Copy link
Owner

Thanks! I can merge and fix the js files myself. The last dependabot updates broke the build anyway.

@hendrikmuhs hendrikmuhs merged commit cc79299 into hendrikmuhs:main Jan 8, 2023
@ppizarror
Copy link

Nice!! thanks a lot @hendrikmuhs, @fruffy

@vadz
Copy link
Contributor

vadz commented Jan 17, 2023

I was eager to try this as I also don't like having one cache for each of the runs, but it seems that this doesn't actually work, as saving the cache always fails when using the new option and the cache already exists.

According to this comment, an existing cache can't be overwritten, and if this is correct, then the new option is not very useful. Or am I missing something here?

@fruffy
Copy link
Contributor Author

fruffy commented Jan 17, 2023

Part of the motivation for this PR was to get some control over the key signature. You can still define a key that is flexible enough to avoid that issue (using PR/commit IDs). This is what we have been doing.

But yes, it is an issue right now. Hope they will fix that bug at some point.

@vadz
Copy link
Contributor

vadz commented Jan 18, 2023

Thanks for the clarification, it wasn't obvious that using this option required making the key unique in some other way. I think it would be nice to mention this in the README.

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.

4 participants