-
Notifications
You must be signed in to change notification settings - Fork 56
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
Update to Apache HttpClient 4.5.x #68
Conversation
The original PR: nemccarthy#103 I tested all 3 used methods (GET, POST, DELETE), everything appears to be OK. The error handling is reasonable. If multiple PR builders try to merge the same PR, one succeeds to merge, others fail to merge with HTTP error |
More improvements for the exception handling. |
a3d88c7
to
5bdcc09
Compare
The changes look good, but I'm worried about doing this with no test coverage. Could you consider adding some client tests against a mock server (e.g. http://www.mock-server.com/) ? |
Good idea. I'll try to add the tests when I have a chance. |
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.
see above
0cb4ccd
to
611cdbc
Compare
It looks like the If we remove running requests in a separate thread, we can test the code by mocking the Apache HTTP code without any mock server. As a first step, I'm trying to get all cleanups outside the main commit (HTTP client update) and get them merged separately. The main commit should not be doing any cleanups to be easy to review. |
All changes below the @loicalbertin's commit have been submitted as #83, they don't need to wait. I've checked other plugins. StashNotifier encoded the context into the client, which is probably a good idea, which would reduce the changes. See https://github.com/jenkinsci/stashnotifier-plugin/blob/39671348994312f6da45ac3a04639e1aa99edbe0/src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java#L422 I'm also not convinced if we should run HTTP requests in a separate thread, especially considering that we only use it to enforce the request timeout. There should be an easier way to do it. |
5a51f09
to
ad7378b
Compare
Updated to remove an extra dependency on httpclient 4.5.2. The dependency is already in the master branch. |
FTR I'm sill waiting for the TA coverage here |
I know, it should be easier now. |
I took a closer look at the dependencies. When Maven resolves versions, it uses two rules
More information here: https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html Without this PR, i.e. on the master branch
The commons-codes dependency wins because it's the shortest (direct). Other dependencies are OK with commons-codec 1.10.
If I remove the dependency on commons-codec, I have a conflict, as commons-httpclient has its way and sets commons-codec version to 1.2, breaking httpclient. However, if I put commons-httpclient below httpclient in pom.xml, the dependency issue is resolved. That would downgrade commons-codec from 1.10 to 1.9, but all dependencies are OK with it. With this PR as originally submitted
With this PR as it is now
|
Given that commons-codec is not our direct dependency, it's up to out dependencies to decide whether commons-codec is needed at the runtime. I did check the plugin build this PR on a Jenkins server, it works fine. Basic authentication works over HTTP and HTTPS. Builds work, that involves all methods we have in the code (GET, DELETE, POST). |
I meant that sometimes codec-like dependencies are optional. It's up to the user to add them if needed. If you checked it on a jenkins server that's enough to verify it. |
We shouldn't depend on dependecy order in pom.xml. This is too birttle. Ideally we would pass a full dependecy covergence check |
de57f26
to
397887e
Compare
I've removed the upgrade to httpclient 4.5.9 from this PR, including the commons-codec removal. I wanted to use the latest client to avoid depending on any obsolete methods. But everything is working with httpclient 4.5.2. Dependency cleanup can be done in a separate PR. |
I agree.
I tried that and I don't like the result. I had to declare 28 dependencies in In most cases I just need the oldest version that satisfies all dependencies. I wish I could just tell Maven to use that logic instead of what it does now. That said, I found some issues that could be easily fixed and submitted them as #121. |
3403829
to
404f649
Compare
If #120 is merged first, this PR would make us use HttpClient 4.5.5. So I'm not trying to document the exact version of HttpClient anymore. While at that, I added a little warning fix found by SpotBugs. We should not declare two exceptions in the |
904a263
to
9a559ba
Compare
The same variable |
Many properties of the connection between Jenkins and Bitbucket server can now be set as Java parameters. The list of the supported parameters is available at https://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/impl/client/HttpClientBuilder.html Additionally, many compiler warnings are fixed by removing obsolete code. Original patch by Albertin Loic <loic.albertin@atos.net>
Remove corresponding exclusions for Javadoc and FindBugs
The commits before "Update to Apache httpclient 4.5.2" are cleanups
"Update to Apache httpclient 4.5.2" has been salvaged from nemccarthy#103
The next commit removes
EasySSLProtocolSocketFactory
, which is no longer neededThe final commit contains additional cleanups.