Elasticsearch has Header and HttpsContext Settings #2652
Conversation
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
|
At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user |
jabbaugh
force-pushed
the
master
branch
4 times, most recently
from
7ebaf08
to
def1c17
Compare
| copy(connectionContext = Option(connectionContext)) | ||
|
|
||
| def hasConnectionContextDefined: Boolean = connectionContext.isDefined | ||
|
|
||
| def copy(baseUrl: String = baseUrl, |
There was a problem hiding this comment.
@seglo - should this method be private? Because it is not, this class might end up being restricted for future updates because changes could affect binary compatibility. Note that other classes in this directory have the copy method set with private scope. Specifically, the ElasticsearchSourceSettings and the ElasticsearchWriteSettings. In my understanding, these classes are set up with very intentional member scopeto allow for binary compatible expansions on the alpakka api.
(Sorry, I know this line was not changed in this PR, but I am pretty sure this public copy method is the reason that @jabbaugh needed to update the mima filters.)
There was a problem hiding this comment.
For reference, this class is new to the 3.0.0 release, so I don't think changing this method to private qualifies as a break change yet.
There was a problem hiding this comment.
Yes, I think it's fine to make it private.
There was a problem hiding this comment.
We'll still need to add the MiMa exceptions since there's been a milestone release, but that's fine too.
jabbaugh
force-pushed
the
master
branch
2 times, most recently
from
008b181
to
51e30a5
Compare
| @@ -0,0 +1 @@ | |||
| ProblemFilters.exclude[DirectMissingMethodProblem]("akka.stream.alpakka.elasticsearch.ElasticsearchConnectionSettings.*") | |||
There was a problem hiding this comment.
Can you add the individual violations instead of using the wildcard?
| copy(connectionContext = Option(connectionContext)) | ||
|
|
||
| def hasConnectionContextDefined: Boolean = connectionContext.isDefined | ||
|
|
||
| def copy(baseUrl: String = baseUrl, |
There was a problem hiding this comment.
Yes, I think it's fine to make it private.
| copy(connectionContext = Option(connectionContext)) | ||
|
|
||
| def hasConnectionContextDefined: Boolean = connectionContext.isDefined | ||
|
|
||
| def copy(baseUrl: String = baseUrl, |
There was a problem hiding this comment.
We'll still need to add the MiMa exceptions since there's been a milestone release, but that's fine too.
...earch/src/main/scala/akka/stream/alpakka/elasticsearch/ElasticsearchConnectionSettings.scala
Show resolved
Hide resolved
| final class ElasticsearchConnectionSettings private ( | ||
| val baseUrl: String, | ||
| val username: Option[String], | ||
| val password: Option[String] | ||
| val password: Option[String], | ||
| val headers: Option[List[HttpHeader]], |
There was a problem hiding this comment.
Could probably be initialized as an empty list.
There was a problem hiding this comment.
Changed to an initialized empty list instead of an option.
...earch/src/main/scala/akka/stream/alpakka/elasticsearch/ElasticsearchConnectionSettings.scala
Show resolved
Hide resolved
| @@ -40,6 +40,8 @@ Java | |||
| | baseUrl | Empty | The base URL of Elasticsearch. Should not include a trailing slash. | | |||
| | username | None | The username to authenticate with | | |||
| | password | None | The password to authenticate with | | |||
| | headers | None | List of headers that should be sent with the http request. | | |||
| | connectionContext | None | The connectionContext that will be used with the http request. | | |||
There was a problem hiding this comment.
Maybe add a note here or somewhere else in the docs about why you would use this configuration, to use a TLS connection.
There was a problem hiding this comment.
I added some more details for the connectionContext
jabbaugh
force-pushed
the
master
branch
8 times, most recently
from
2d0f6cc
to
80d5890
Compare
The current ElasticsearchAPI only supports basic auth. By including header options as well as the HttpsContext there would be additional options to keep previous functionality but still moving forward with AkkaHttp.
The current ElasticsearchAPI only supports basic auth. By including
header options as well as the HttpsContext there would be additional
options to keep previous functionality but still moving forward with
AkkaHttp.
References #xxxx