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

Add 502 to specific list of retryable HTTP errors #45

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

alexstoick
Copy link
Contributor

Fix an issue where we check the code (string) presence in an array of
Int values.

@alexstoick alexstoick force-pushed the feat/add-502-to-retryable-errors branch from bee75c7 to 2638cf1 Compare July 12, 2024 10:00
@alexstoick alexstoick changed the title Add 504 to specific list of retryable HTTP errors Add 502 to specific list of retryable HTTP errors Jul 12, 2024
@alexstoick alexstoick marked this pull request as ready for review July 12, 2024 10:03
Fix an issue where we check the code (string) presence in an array of
Int values.
@alexstoick alexstoick force-pushed the feat/add-502-to-retryable-errors branch from 2638cf1 to 3ca8f3c Compare July 12, 2024 10:14
@alexstoick alexstoick merged commit c0414b9 into master Jul 12, 2024
2 checks passed
@alexstoick alexstoick deleted the feat/add-502-to-retryable-errors branch July 12, 2024 10:17
@@ -223,7 +223,7 @@ def raise_on_bad_response(response)
def retryable_http_response_code?(code)
# retry (in order): bad request, forbidden (token expired in flight), method not allowed,
# request timeout, too many requests, anything in the 500 range (504 is fairly common)
[400, 403, 405, 408, 429, 504].include?(code.to_i) || (500..599).include?(code)
[400, 403, 405, 408, 429, 502, 504].include?(code.to_i) || (500..599).include?(code.to_i)
Copy link
Member

Choose a reason for hiding this comment

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

@alexstoick is this the same as

[400, 403, 405, 408, 429].include?(code.to_i) || (500..599).include?(code.to_i)

? i.e. is it redundant to have 502 and 504 in the first list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep... that is indeed redundant now.

Copy link
Member

Choose a reason for hiding this comment

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

made a quick PR: #46

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