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

Retry all non 400/401/403 errors instead of only 500s #5

Merged
merged 2 commits into from
Jan 1, 2019

Conversation

SGrondin
Copy link
Collaborator

BREAKING CHANGE: Now retrying all non 400/401/403 errors instead of only 500s

BREAKING CHANGE: Now retrying all non 400/401/403 errors instead of only 500s
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, but we should also update the README. Instead of

Implements request retries for server error responses.

We should say sth like

Implements request retries for request error status codes other than 400, 4001 and 403?

I’m still not sure about the 404 🤔 I think the replication lag problem is mostly a problem when handling webhook events and I think we should solve it there. Otherwise I imagine people writing tests for their apps with deliberate 404s and then wonder why Octokit sends the same request again :/

Maybe we should add the 404 to the ignore codes and document how you can have retries on a per-request basis (like we already do)

What do you think?

@gr2m
Copy link
Contributor

gr2m commented Dec 30, 2018

We should also ignore 304 error codes. "Not Modified" Responses are thrown so users can implement their caching solutions.

https://runkit.com/gr2m/octokit---conditional-request-example/1.0.0

@SGrondin
Copy link
Collaborator Author

SGrondin commented Dec 31, 2018

Maybe we should add the 404 to the ignore codes and document how you can have retries on a per-request basis (like we already do)

I think you're right. I changed it and also documented how to override the ignore codes.

We should also ignore 304 error codes. "Not Modified" Responses are thrown so users can implement their caching solutions.

I didn't expect 3xx to be treated as errors at all. I've updated it so it doesn't retry 3xx.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking great!

@gr2m gr2m merged commit 0b0360b into master Jan 1, 2019
@gr2m gr2m deleted the retry-improvements branch January 1, 2019 18:06
@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

3 participants