fix: saveCache shouldn't error on existing cache, warn instead#40
Merged
Conversation
- if your workflow has a race condition where similar jobs with the same
cache key run in parallel, one will saveCache first and the next will
error out when trying to saveCache
- previously this was a warning, but after upgrading to official
@actions/cache, it is now an error
- this can occur on matrix installs with similar enough environments
(e.g. different Node version, same OS) and did occur in workflows
here too actually
- there's an upstream issue tracking this, so it might actually have
a different root cause
- previously only ran on `push`, meaning only for root repo branches,
and not PRs from forks
- meaning they could be failing unknowingly and it would only be seen
_after_ merge
- as happened with my previous PR
- this should make it run on PRs from forks as well, so failed CI
checks should be very visible
- (but branch protection needs to be turned on to prevent merging)
|
🎉 This PR is included in version 1.4.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Commits
hotfix: saveCache shouldn't error on existing cache, warn instead
cache key run in parallel, one will saveCache first and the next will
error out when trying to saveCache
@actions/cache, it is now an error
(e.g. different Node version, same OS) and did occur in workflows
here too actually
a different root cause
ci: also run all workflows on PRs to ensure they pass
push, meaning only for root repo branches,and not PRs from forks
after merge
checks should be very visible
Tags
Fixes #39 , workaround for upstream actions/cache#144
Follow-up to bug introduced by #37
Adding PR check to CI pretty much identical to jaredpalmer/tsdx#373
Review Notes
#37 passed the CI check / self-integration test matrix I added in my fork
and I believe it passed here too (says 0 checks now though, possibly due to the patch bump added there), but when it was merged here it then failed. Since then has caused many failures downstream per #39, which I too experiencedEDIT: nope, CI checks didn't run here as they weren't turned on for PRs, so I've added that to all CI checks now as a second commit in this PR
Testing
Screenshot differences in CI
Before on
master:After on cache-busted version of current PR:
While the same error is printed as I
console.warn'd it, it no longer throws and therefore doesn't fail the jobIt also passes on current PR, but these were all cache hits, probably because I was testing on this branch before.
As far as I understand, there is no way to force cache bust without changing the key in GitHub Actions right now (no API for it to be exact), which makes integration testing cache misses a bit difficult. If we were to add actual integration tests as opposed to this "self-CI integration test" as I've done here then we could randomize cache key there to force a cache miss