Migrate to httpclient5 #197
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| private void configureTimeoutAndSsl(HttpClientBuilder clientBuilder) throws NoSuchAlgorithmException, KeyManagementException { |
Check warning
Code scanning / Jenkins Security Scan
Jenkins: Generally unsafe method calls Warning
| } | ||
| return items; | ||
| } | ||
| public ListBoxModel doFillResponseHandleItems() { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing permission check Warning
| } | ||
| return items; | ||
| } | ||
| public ListBoxModel doFillResponseHandleItems() { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing POST/RequirePOST annotation Warning
| @QueryParameter String url) { | ||
| return HttpRequest.DescriptorImpl.fillAuthenticationItems(project, url); | ||
| } | ||
| public ListBoxModel doFillProxyAuthenticationItems(@AncestorInPath Item project, |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing POST/RequirePOST annotation Warning
| } | ||
|
|
||
| public FormValidation doCheckValidResponseCodes(@QueryParameter String value) { | ||
| public FormValidation doCheckValidResponseCodes(@QueryParameter String value) { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing permission check Warning
| } | ||
|
|
||
| public FormValidation doCheckValidResponseCodes(@QueryParameter String value) { | ||
| public FormValidation doCheckValidResponseCodes(@QueryParameter String value) { |
Check warning
Code scanning / Jenkins Security Scan
Stapler: Missing POST/RequirePOST annotation Warning
| private static final long serialVersionUID = 4818288270720177069L; | ||
|
|
||
| private final String keyName; | ||
| private final String keyName; |
Check warning
Code scanning / Jenkins Security Scan
Jenkins: Plaintext password storage Warning
| private final String keyName; | ||
| @Serial | ||
| private static final long serialVersionUID = -4370238820437831639L; | ||
| private final String keyName; |
Check warning
Code scanning / Jenkins Security Scan
Jenkins: Plaintext password storage Warning
|
Jenkins Security Scan alerts are unrelated to these changes and should be adressed in a separate PR. |
|
It looks like the build failure has nothing to do with your code modification: |
* Migrate imports, classes and methods * Reduce usage of deprecated classes and methods * Minor code cleanup
|
@jenkinsci/http-request-plugin-developers Kindly requesting a review. |
I'm the only active maintainer of this plugin and I'm deeply involved in other activities that are higher priority for me and for Jenkins. It will likely be several months before I'm able to review this. |
# Conflicts: # src/main/java/jenkins/plugins/http_request/HttpRequestExecution.java
|
In v1.20 of this plugin, I was able to use GitHub App credentials. |
@strangelookingnerd would you be willing to investigate this? I'd rather not revert back to httpclient 4 after all your good work to make the switch. |
I can give it a shot. @dgonz782 Would you be able to share some more details so I could create a reproducible test case? The pipeline script you are using or similar would help solving this issue faster. |
|
@strangelookingnerd - turns out the 404 err was due to an extra / in the constructed api url from clone url. def prData = httpRequest (
httpMode: 'GET',
authentication: config.gitHttpsCredential,
- url: getRepoApiUrl(config.repos.<repo>, "/pulls/${env.CHANGE_ID}")
+ url: getRepoApiUrl(config.repos.<repo>, "pulls/${env.CHANGE_ID}")
)A url with |
|
According to RFC2396 a double-slash is not a allowed in the path section of a URI. The It appears @MarkEWaite WDYT? |
I agree. I think that we should accept that this new behavior is the more correct behavior. |
… 5 minutes When the timeout parameter is not provided or set to 0 (documented as 'no timeout'), httpclient5 was using its default 5-minute timeout instead. This fixes the regression introduced in v1.22 during the httpclient5 migration (PR jenkinsci#197) by explicitly setting timeout to DISABLED when timeout <= 0, restoring the documented behavior.
#225) * Fix JENKINS-76283: httpRequest ignores default timeout 0 and enforces 5 minutes When the timeout parameter is not provided or set to 0 (documented as 'no timeout'), httpclient5 was using its default 5-minute timeout instead. This fixes the regression introduced in v1.22 during the httpclient5 migration (PR #197) by explicitly setting timeout to DISABLED when timeout <= 0, restoring the documented behavior. * Reduce white space diffs --------- Co-authored-by: Mark Waite <[email protected]>
|
@strangelookingnerd I'm afraid I have a regression to report, too. Any idea? |
|
@Khaos66 Please have a look at https://issues.jenkins.io/browse/JENKINS-76280 and see if the your problem can be fixed by the incremental plugin build I linked in there. At least from the error message it seems to be of similar nature. |
5 participants
Replaces outdated
httpclient4with the successorhttpclient5.I'm well aware that this is a quite large changeset however I hope that there is still interest in this PR and it will be reviewed.
If there are any questions, please do not hesitate to ping me.
Testing done
mvn clean verifyand some manual testing.Submitter checklist