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

feat: adds "cache-on-failure" propagation to Swatinem/rust-cache #39

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

samuelhnrq
Copy link
Contributor

Pretty self explanatory, adds the propagation of cache-on-failure alongside cache-workspaces.

This action works really nicely and I didn't want to manually setup cache just because of this, neither did I want to recompile axum every-time my unit tests failed 😆

Backwards compatible, false already is the default value.

@robjtede robjtede requested a review from jonasbb June 5, 2024 04:41
@jonasbb
Copy link
Member

jonasbb commented Jun 5, 2024

The change looks fine and the setting important enough to change. Is there a downside to enabling cache-on-failure by default for this action? It feels like the more useful default to have as it could mean a higher cache hit rate. Unless storing after a failure could ruin/overwrite a good cache state. And if it is the default do we need to expose that as a toggle?

@samuelhnrq
Copy link
Contributor Author

I do think its a saner default and but I also like the toggle, to avoid people having the same dilemma as me: the action works perfectly fine but now I have to drop it and internalise a lot of functionality myself for a silly config value without a toggle.

I have no idea on the details implementation of the actual caching of but I would expect it to keep the cache 1:1 even if unread/unwritten. I guess the point of turning this off is not to download a Gig of cache to fail on something unrelated to cargo and upload the gig back, unchanged, because I really dont expect it to be a diffing on the cache (pretty sure it's GIGO)

I'm pretty confident that cargo is smart enough to not ruin its own cache on failed runs (except on absurdities like getting a SIGKILL midway)

@jonasbb jonasbb merged commit 74e1b40 into actions-rust-lang:main Jun 5, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants