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

fix(retry): better error #501

Open
wants to merge 9 commits into
base: v2stable
Choose a base branch
from
Open

fix(retry): better error #501

wants to merge 9 commits into from

Conversation

otomatik
Copy link
Contributor

@otomatik otomatik commented Jul 16, 2024

Q A
Bug fix? yes
New feature? no
BC breaks? no
Related Issue N/A
Need Doc update no

Describe your change

  • fixing debug.log file not being properly opened (and make sure if it fails to always fallback on stderr)
  • add more details (with retry errors) to AlgoliaUnreachableHostError when a request fails

What problem is this fixing?

saving time when debugging a connection issue with more info about what is happening

millotp
millotp previously approved these changes Jul 18, 2024
Copy link
Contributor

@millotp millotp left a comment

Choose a reason for hiding this comment

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

looks good

@otomatik otomatik marked this pull request as ready for review July 19, 2024 09:35
kombucha
kombucha previously approved these changes Jul 22, 2024
Copy link
Contributor

@kombucha kombucha left a comment

Choose a reason for hiding this comment

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

LGTM 👍

kombucha
kombucha previously approved these changes Jul 23, 2024
Copy link
Contributor

@kombucha kombucha left a comment

Choose a reason for hiding this comment

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

Still LGTM

Copy link

@DevinCodes DevinCodes left a comment

Choose a reason for hiding this comment

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

This should make the CI pass. Without this change, do the tests pass locally?

test/algolia/unit/retry_strategy_test.rb Outdated Show resolved Hide resolved
@otomatik
Copy link
Contributor Author

This should make the CI pass. Without this change, do the tests pass locally?

yeah it passes locally, maybe it's too specific to the env of test execution then?

@DevinCodes
Copy link

My guess is that you have a process running on your localhost that accepts connections (but return an SSL error), and for the CI that's not the case.

Maybe instead of checking the error message exactly, you can check the keys in the hash that's returned instead so we know that an error per host is provided. WDYT?

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.

4 participants