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

Replace Cypress fork of actions/cache with official @actions/cache package #62

Merged
merged 4 commits into from
Dec 4, 2020
Merged

Conversation

mondash
Copy link
Contributor

@mondash mondash commented Sep 29, 2020

Linked issue

#61

Additional context

I haven't included any fix for the concurrency issue I mentioned in #61, but it looks like that issue stems from further upstream and isn't necessarily in the scope of this PR anyway. Nevertheless, I'll be keeping an eye on that issue (and maybe proposing a fix for it if I have the time) since it does affect me. If this PR is merged and that issue does get resolved, I'd be happy to make another PR to introduce that fix into this action.

Edit: This PR does now include a fix for the above mentioned issue

Feel free to hit me up if you have any questions or concerns. Thanks!

Note

I ran yarn upgrade since I noticed a dependabot warning on my fork.
I have not updated the caching section of the README

@byCedric
Copy link
Member

byCedric commented Oct 2, 2020

This looks great, thanks a lot! I'll test run this tomorrow too and fix the CI error. It basically says that the current build isn't up to date and needs to be rebuilt. 😄

@byCedric byCedric self-requested a review October 2, 2020 13:10
@mondash
Copy link
Contributor Author

mondash commented Oct 3, 2020

Thanks for taking a quick interest in this. I really appreciate it!

Since I mentioned above (and so you don't need to go searching yourself), here's the stack of issues/PRs blocking the fix for the concurrency issue:

The issue itself

"Cache already exists" due to concurrency: actions/toolkit#537

Needs to be merged before resolving the above issue

Throw HttpClientError instead of Error: actions/http-client#32

Causing tests to fail in above issue

/redirect-to returns 404: postmanlabs/httpbin#617

Bump @actions/cache from 1.0.2 to 1.0.3
Add missing caret to specify @actions/cache version range
Add handling for ReserveCacheError
@mondash
Copy link
Contributor Author

mondash commented Oct 21, 2020

@byCedric The PR resolving actions/toolkit#537 was merged recently, so I took the liberty of adding the additional error handling to bring that fix into this repo.

Edit: Just noting again here that I haven't updated the README or CHANGELOG (I'll leave that for you)

@mondash
Copy link
Contributor Author

mondash commented Nov 17, 2020

@byCedric I don't want to rush you (especially if you have other priorities), but I did just want to bump this PR. Thanks again!

@byCedric
Copy link
Member

Hey @mondash! I'm so sorry, you are right to ping me 🙈 We are working hard to push SDK 40 and some other cool stuff out the door. I'll get back to you asap!

@byCedric byCedric merged commit 0eb46e9 into expo:master Dec 4, 2020
@byCedric
Copy link
Member

byCedric commented Dec 4, 2020

I'm so sorry for putting this off for so long! Thanks for your PR and persistence 🙈 . I'll tweak some small things, like logging with core.info, and will release asap after some testing! In the meantime, you can try your PR by using the @master branch.

@mondash
Copy link
Contributor Author

mondash commented Dec 4, 2020

@byCedric No worries, I know how schedules can be! :) Thanks a lot for getting this through!

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