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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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

@otomatik otomatik mentioned this pull request Jul 18, 2024
Merged
millotp
millotp previously approved these changes Jul 18, 2024
View reviewed changes
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
View reviewed changes
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
View reviewed changes
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

DevinCodes
DevinCodes requested changes Jul 23, 2024
View reviewed changes
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
Reviewers

@DevinCodes DevinCodes DevinCodes requested changes

@millotp millotp millotp left review comments

@kombucha kombucha kombucha left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@otomatik @DevinCodes @kombucha @millotp