remove client config and unify authentication methods #236
Conversation
lucidd
force-pushed
the
master-async-http-client
branch
from
5b585d9
to
6717d32
Compare
renner
force-pushed
the
master-async-http-client
branch
from
6717d32
to
a0488ff
Compare
lucidd
force-pushed
the
master-config-cleanup
branch
from
6e4d570
to
70069d5
Compare
There was a problem hiding this comment.
It looks good to me in general, just consider my comments on the patch and please fix all the javadoc warnings that show up in travis.
| config.put(ClientConfig.PROXY_PASSWORD, settings.getPassword()); | ||
| } | ||
| } | ||
| public SaltClient(URI url, AsyncConnection asyncHttpClient) { |
There was a problem hiding this comment.
It is not really clear here: what does this object represent, a connection or an HTTP client? Let's maybe rename one of them (the class or variable name)?
There was a problem hiding this comment.
yeah the name is still the old but at this point its more of a http client then a connection. i will rename it.
| @@ -15,13 +18,14 @@ | |||
| private static final String PASSWORD = "saltdev"; | |||
|
|
|||
| public static void main(String[] args) { | |||
| CloseableHttpAsyncClient httpClient = TestUtils.defaultClient(); | |||
There was a problem hiding this comment.
If the defaultClient() is something that could be used in production I would suggest to move it away from TestUtils to a better place.
There was a problem hiding this comment.
i don't think its suitable for production since its disables all timeout which you probably don't want in a production environment.
There was a problem hiding this comment.
Can't or shouldn't we provide some reasonable defaults for those timeouts, for example the values that were used before? I think in order to make it easy for people to get started quickly we could provide reasonable defaults with this default client.
There was a problem hiding this comment.
i added a helper method for creating an async http client with the default timeout values used in our config before
There was a problem hiding this comment.
Nice, so I think we should use HttpClientUtils.defaultClient() everywhere throughout the examples, and even throughout the tests while removing the currently still existing TestUtils.defaultClient()? If we don't remove the TestUtils method for a reason then let's use it only in tests, please.
lucidd
force-pushed
the
master-config-cleanup
branch
2 times, most recently
from
e72bfc9
to
71baa12
Compare
|
@renner javadoc warning are addressed |
lucidd
force-pushed
the
master-config-cleanup
branch
2 times, most recently
from
c93ebe1
to
c7c77ec
Compare
| private final JsonParser<T> parser; | ||
| public static HttpAsyncClient defaultClient() { | ||
| //TODO | ||
| return null; |
There was a problem hiding this comment.
@lucidd: The default client can now be generated via HttpClientUtils as far as I understand, so should we remove the TODO and this whole method here?
| import com.suse.salt.netapi.event.WebSocketEventStream; | ||
|
|
||
| import java.net.URI; | ||
|
|
||
|
|
lucidd
force-pushed
the
master-config-cleanup
branch
from
a3ee642
to
5924c87
Compare
lucidd
force-pushed
the
master-config-cleanup
branch
from
5924c87
to
e2a6f7d
Compare
lucidd
force-pushed
the
master-config-cleanup
branch
from
722fb66
to
d8f7d08
Compare
This PR removes the ClientConfig since its generally insufficient if you want to customize http client config. Instead a user can now configure the http client backend they want to use and then simply pass it to us. Since the token has since been in the configuration each call method now uses an AuthMethod parameter to decide how to authenticate the call. AuthMethod either contains the password authentication parameters or a token. Overall this PR leads to a lot less code more unification between call method variants and also gets rid of the non thread safety we had due to the mutable config.