-
-
Notifications
You must be signed in to change notification settings - Fork 183
Http download retry issue 929 3 #936
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
Http download retry issue 929 3 #936
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## beta #936 +/- ##
=======================================
Coverage 89.62% 89.62%
=======================================
Files 15 15
Lines 2072 2072
Branches 528 528
=======================================
Hits 1857 1857
Misses 204 204
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, though some documentation for at least some errors would be great.
Also did you already test it in your CI with those changes?
const retryableStatusCodes = [429, 500, 503]; | ||
|
||
const retryableErrorCodes = [ | ||
'ECONNRESET', | ||
'ETIMEDOUT', | ||
'ENOTFOUND', | ||
'ECONNREFUSED', | ||
'EPIPE', | ||
'EHOSTUNREACH', | ||
'EAI_AGAIN', | ||
'ENETUNREACH', | ||
'ECONNABORTED', | ||
'aborted', | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a good idea to add some comments to non-self-explaining errors, for example what is EAI_AGAIN
? (i did not even know about this error before looking it up)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes there are lots of such error codes - https://nodejs.org/api/errors.html#nodejs-error-codes
No. I have not tested with these retryable codes in Jenkins CI (running inside docker) We use artifactory which takes 1 hour to reflect the public npmjs changes. I can test with 10.2.0-beta.4 once that is available. |
🎉 This PR is included in version 10.2.0-beta.4 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Added more retryable error codes.