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 client for http4s library #3118

Merged
merged 14 commits into from
Nov 10, 2024
Merged

Conversation

igor-vovk
Copy link
Contributor

@igor-vovk igor-vovk commented Jul 16, 2024

Hi! I don't know if there is space for another client, but it seemed appropriate for me to add it, since it's kinda mainstream client in cats-effect ecosystem (http4s/circe/cats-effect). I was kinda surprised not to find it, ended up implementing it, but got tired copying it between my projects where I use elastic4s.

If you think it's a good idea to add it, I will be happy to add tests in this PR as well. Thank you.

@igor-vovk
Copy link
Contributor Author

It just came to me, @Philippus if you think that elastic4s supports enough clients out of the box (I found that sttp for example already has http4s backend), I can just publish it as a separate library.

@lenguyenthanh
Copy link
Contributor

I would love to use this client! I think currently sttp use future backend (which is backed scala future & akka iiuc). So this is definitely worth adding imho.

Also We probably can have more perform IO backed client if We have this: #2379

@igor-vovk
Copy link
Contributor Author

@lenguyenthanh I was also thinking about that, addressed in #3119

@lenguyenthanh
Copy link
Contributor

@lenguyenthanh I was also thinking about that, addressed in #3119

this is great, thanks! I was about to do it.

@Philippus
Copy link
Owner

I think adding a new client is fine, provided someone is using it in production (like you). Maybe we can mark it as experimental, allowing us to make backwards incompatible changes for a certain time.

@igor-vovk
Copy link
Contributor Author

@Philippus nice! Then I'm going to add tests for it

@igor-vovk
Copy link
Contributor Author

@Philippus I've added some tests following the pattern from sttp. I think it is ready to be merged, WDYT?

@Philippus
Copy link
Owner

Philippus commented Nov 8, 2024

@igor-vovk now that #3124 is merged, care to get back to this PR and clean it for merging (or create a new one based on the current main branch)?

@igor-vovk
Copy link
Contributor Author

@Philippus thanks for merging #3124! Yes, cleaned up custom authentication from the implementation, please check

@Philippus
Copy link
Owner

@lenguyenthanh Do you have time to take a look at the PR to see if this will work for you as well?

@lenguyenthanh
Copy link
Contributor

@lenguyenthanh Do you have time to take a look at the PR to see if this will work for you as well?

of course, I'll take a look tomorrow.

Copy link
Contributor

@lenguyenthanh lenguyenthanh left a comment

Choose a reason for hiding this comment

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

I only have few nit comments, otherwise lgtm. And this will be so much better with the other of @igor-vovk's pr: #3119

@igor-vovk
Copy link
Contributor Author

@lenguyenthanh I've addressed all your comments

@igor-vovk
Copy link
Contributor Author

@Philippus I also noticed test flakiness, tried to add health check before proceeding on the level of GH actions in #3207, but failing to understand why it fails in GH Actions environment (works locally, probably something with bash options...)

@Philippus
Copy link
Owner

Philippus commented Nov 9, 2024

I think it's because of all the tests running on the same instance. If you change the test to "yellow" or "green" I think it will be fine. Even "red" would be a valid answer for the test I think, because it's about proof of connection.

@igor-vovk
Copy link
Contributor Author

@Philippus I think what happens is that this status changes from red to yellow and then to green (doc https://www.elastic.co/guide/en/elasticsearch/reference/current/cluster-health.html). And then depending on when the test runs it can receive any of these, thus flakiness of the test. In any case, I've deleted this test. Should I bring it back?

@igor-vovk
Copy link
Contributor Author

Marked #3207 as ready for review. It now waits elastic to start before continuing, should reduce tests flakiness

@Philippus
Copy link
Owner

@Philippus I think what happens is that this status changes from red to yellow and then to green (doc https://www.elastic.co/guide/en/elasticsearch/reference/current/cluster-health.html). And then depending on when the test runs it can receive any of these, thus flakiness of the test. In any case, I've deleted this test. Should I bring it back?

If you think it's worth having it as a test I think we can bring it back. We can always remove it again, if it turns out to be flaky again.

This reverts commit a7f8762.
@igor-vovk
Copy link
Contributor Author

Reverted the deletion of the test. After #3207 will be merged, the test should stop being flaky

@Philippus Philippus merged commit c93b424 into Philippus:main Nov 10, 2024
3 checks passed
@igor-vovk igor-vovk deleted the client-http4s branch November 10, 2024 20:34
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