-
Notifications
You must be signed in to change notification settings - Fork 95
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
remove client config and unify authentication methods #236
Conversation
5b585d9
to
6717d32
Compare
6717d32
to
a0488ff
Compare
6e4d570
to
70069d5
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
e72bfc9
to
71baa12
Compare
@renner javadoc warning are addressed |
c93ebe1
to
c7c77ec
Compare
private final JsonParser<T> parser; | ||
public static HttpAsyncClient defaultClient() { | ||
//TODO | ||
return null; |
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.
@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; | ||
|
||
|
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.
Unnecessary new line here.
a3ee642
to
5924c87
Compare
5924c87
to
e2a6f7d
Compare
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.