-
Notifications
You must be signed in to change notification settings - Fork 694
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
Support authentication on the level of ElasticClient #3124
Conversation
064153b
to
b1a4292
Compare
b1a4292
to
0461cc4
Compare
0461cc4
to
084cbe4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to add some tests for the different cases?
elastic4s-core/src/main/scala/com/sksamuel/elastic4s/ElasticClient.scala
Outdated
Show resolved
Hide resolved
084cbe4
to
95e9f3e
Compare
Sorry @Philippus I haven't added tests yet. I was thinking whether I should make an IT-test, e.g. try to setup a credentials in ES, and then test against it, or just do a unit test? I was kind of blocked by not being able to choose, maybe you could share what would be the preferred way? Will be happy to add tests after |
I think ideally, set up an Elasticsearch test container requiring credentials and test against that. But I don't know how easy it would be to set that up. I'm fine with someone adding that in a later PR. |
ea70679
to
90e4ec3
Compare
Addresses #3122. I copied authentication settings that were introduced in http4s PR #3118, will be happy to remove them from http4s PR after this will be merged.
Also, they are now can be removed from AkkaHttpClient, that also implements adding credentials by itself.
By defining credentials on the ElasticClient level there is now a single place where credentials are used, so there is no need to rely on client-specific implementations of credential passing.