Skip to content
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

OkHttp3 #223

Merged
merged 3 commits into from
Jun 4, 2020
Merged

OkHttp3 #223

merged 3 commits into from
Jun 4, 2020

Conversation

bitwiseman
Copy link
Contributor

@bitwiseman bitwiseman commented May 2, 2019

okhttp3 3.12.12

@jglick
Copy link
Member

jglick commented May 2, 2019

@bitwiseman no need to upload plugin archives to Drive! And never hand out *-SNAPSHOT versions. Just fix the build here (currently seems broken by INFRA-2075, which can be worked around simply by closing & reopening), and then wait for JEP-305 deployment and you can just link people to Artifactory for download.

@bitwiseman
Copy link
Contributor Author

@jglick Yes, if the build weren't breaking the incremental-release would be what I'd use. But it is, so ...

@jglick
Copy link
Member

jglick commented May 2, 2019

if the build weren't breaking

Well now it looks legit:

java.lang.UnsupportedOperationException: clientBuilder.sslSocketFactory(SSLSocketFactory) not supported on JDK 9+
	at okhttp3.internal.platform.Jdk9Platform.trustManager(Jdk9Platform.java:81)
	at okhttp3.internal.platform.Platform.buildCertificateChainCleaner(Platform.java:176)
	at okhttp3.OkHttpClient$Builder.sslSocketFactory(OkHttpClient.java:768)
	at org.jenkinsci.plugins.github_branch_source.OkHttp3Connector.<init>(OkHttp3Connector.java:40)
	at …

@bitwiseman
Copy link
Contributor Author

@jglick Legit on java9+. This is still an early alpha experiment.

@jglick
Copy link
Member

jglick commented May 3, 2019

Legit on java9+.

FWIW I would then just patch Jenkinsfile in this PR to remove the Java 11 build configurations, so we at least get a clean build & Incrementals publishing on Java 8, and later figure out how to fix 11 compatibility before moving out of draft.

pom.xml Outdated Show resolved Hide resolved
@jsoref
Copy link
Contributor

jsoref commented May 7, 2019

So, I don't think that was sufficient, although I think it was necessary.

Dependencies and Class Loading suggests:
pluginFirstClassLoader and its discontents

  • pluginFirstClassLoader
  • maskClasses

I think the lazy choice is the former, which appears to look like this:
https://github.com/jenkinsci/hidden-parameter-plugin/blob/46b50d0747c748d84e8ccb40b7c9330813a31e84/pom.xml#L30-L37

@jsoref
Copy link
Contributor

jsoref commented May 7, 2019

This PR is supposed to help fix JENKINS-54126

@bitwiseman
Copy link
Contributor Author

bitwiseman commented May 8, 2019

@jsoref
I manually tested it this time on my local jenkins. Builds run. Scan runs.
https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/github-branch-source/2.5.3-rc841.52a0b90bff37/

@bitwiseman
Copy link
Contributor Author

Most of the code that was in this change has been added to github-api and is tested in that library.

Copy link
Contributor

@jsoref jsoref left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me...

@bitwiseman bitwiseman closed this Feb 23, 2020
@bitwiseman bitwiseman reopened this Feb 24, 2020
@bitwiseman bitwiseman closed this Feb 24, 2020
@bitwiseman bitwiseman reopened this Feb 24, 2020
@bitwiseman bitwiseman force-pushed the okhttp3 branch 2 times, most recently from a5c4f37 to 5bac543 Compare May 1, 2020 20:08
pom.xml Outdated Show resolved Hide resolved
@bitwiseman bitwiseman force-pushed the okhttp3 branch 2 times, most recently from e2163c3 to a5716d0 Compare June 2, 2020 04:04
@bitwiseman bitwiseman marked this pull request as ready for review June 2, 2020 04:07
@bitwiseman bitwiseman force-pushed the okhttp3 branch 2 times, most recently from ecde54c to f73096b Compare June 2, 2020 22:39
GitHubAppCredentials could not use the existing Connector.connect() and so created its own
GithubBuilder.  However, that means that is missing a number of standard settings provided by
Connector.connect(), including okhttp, Jenkins proxy settings, and rate limit handling.

This change refactors Connector to add an internal createGitHubBuilder() method that returns a
GitHubBuilder with those features configured. For simplicity, The returned GitHubBuilder
is not cached and also does not cache responses.  If there turns out to be a need for it,
that behavior can be added later.
Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, but there are some test failures that don't make sense to me:

java.lang.Error: Plugin github-api failed to start
...
Caused by: java.io.IOException: GitHub API Plugin version 1.111.4 failed to load.

  • You must update Jenkins from version 2.164.1 to version 2.164.3 or later to run this plugin.

This plugin already depends on 2.164.3, same as GitHub API Plugin 1.111.4, so I'm not sure what's happening here.

@bitwiseman
Copy link
Contributor Author

@dwnusbaum It's the jenkinsfile build configurations.

@bitwiseman bitwiseman requested review from dwnusbaum and timja June 3, 2020 22:42
@bitwiseman
Copy link
Contributor Author

@timja I updated GitHubAppCredentials to use okhttp. Does this look good to you?

buildPlugin(configurations: [
[ platform: "linux", jdk: "8"],
[ platform: "windows", jdk: "8"],
[ platform: "linux", jdk: "11", jenkins: "2.176.4", javaLevel: "8" ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you aren’t testing a more recent lts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to keep things simple-ish in this PR.

@bitwiseman bitwiseman merged commit b012689 into jenkinsci:master Jun 4, 2020
@timja
Copy link
Member

timja commented Jun 10, 2020

Created https://issues.jenkins-ci.org/browse/JENKINS-62652 for "A connection to https://api.github.com/ was leaked. Did you forget to close a response body?"

@timja
Copy link
Member

timja commented Jun 10, 2020

also:
https://issues.jenkins-ci.org/browse/JENKINS-62655

IllegalStateException: Jenkins has not been started, or was already shut down

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants