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.

Sign up for GitHub

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

Jump to bottom

Migrate HTTP client to okhttp. #188

Merged

szpak merged 5 commits into Codearte:master from anuraaga:migrate-to-okhttp

Feb 26, 2021

Contributor

anuraaga commented Feb 15, 2021 •

edited

When I try to use this plugin with gradle-jib-plugin, I get dependency conflicts due to the version of the Apache HTTP client. I tried several attempts to constrain versions to get something to work but couldn't. https://github.com/jgritman/httpbuilder hasn't been updated for 7 years so it's probably good to migrate off of it. I went with okhttp which is a very popular HTTP client.

I haven't gotten to e2e test this yet on my end but sending for initial check. Thank you!

Fixes #42

Anuraag Agrawal added 3 commits

February 15, 2021 18:13


          Migrate HTTP client to okhttp.

f4ac2df


          Password null check

1b59d6a


          Fix headers and empty response parsing

bfa84f2

szpak requested changes

View reviewed changes

Member

szpak left a comment

Thank for your PR! HttpBuilder, indeed, is somehow problematic. On 19 Mar 2017, I even created a task to do it, but it had to wait for you :).

I put some suggestions and I requested a build with e2e-test (with real Nexus instance) to check it in practice :).

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Show resolved Hide resolved

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Outdated

+                          Response intercept(@NotNull Interceptor.Chain chain) throws IOException {
+                              Request.Builder request = chain.request().newBuilder()
+                                  .header("Content-Type", "application/json")
+                                  .header("Accept", "application/json")

Member

szpak Feb 16, 2021

Is there any ContentType-like class in OkHttp that could be used to get that "application/json" string?

Contributor Author

anuraaga Feb 17, 2021

Unfortunately seems to only support parsing with no list of constants

https://square.github.io/okhttp/4.x/okhttp/okhttp3/-media-type/#companion-object-functions

Member

szpak Feb 17, 2021

Ok, in that case maybe just a constant String in that class?

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Outdated Show resolved Hide resolved

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Outdated Show resolved Hide resolved

szpak added this to the 0.30.0 milestone

szpak reviewed

View reviewed changes

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Show resolved Hide resolved


          Cleanups and try raising timeouts

5d101a6

anuraaga commented

View reviewed changes

Contributor Author

anuraaga left a comment

Thanks, updated

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Show resolved Hide resolved

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Outdated

+                          Response intercept(@NotNull Interceptor.Chain chain) throws IOException {
+                              Request.Builder request = chain.request().newBuilder()
+                                  .header("Content-Type", "application/json")
+                                  .header("Accept", "application/json")

Contributor Author

anuraaga Feb 17, 2021

Unfortunately seems to only support parsing with no list of constants

https://square.github.io/okhttp/4.x/okhttp/okhttp3/-media-type/#companion-object-functions

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Outdated Show resolved Hide resolved

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Outdated Show resolved Hide resolved

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Show resolved Hide resolved

szpak reviewed

View reviewed changes

Member

szpak left a comment •

edited

I will play with it with out e2e-test, but in general it look nice!

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Show resolved Hide resolved

src/main/groovy/io/codearte/gradle/nexus/infra/SimplifiedHttpJsonRestClient.groovy Outdated

+                          Response intercept(@NotNull Interceptor.Chain chain) throws IOException {
+                              Request.Builder request = chain.request().newBuilder()
+                                  .header("Content-Type", "application/json")
+                                  .header("Accept", "application/json")

Member

szpak Feb 17, 2021

Ok, in that case maybe just a constant String in that class?


          Extract constant

acd0e55

Member

szpak commented Feb 18, 2021

Increased connection timeout helped e2e-test and I'm willing to merged it, but the other thing came into my mind. OkHttp seemed to be rewritten to Kotlin on the way. We would mix 3 languages in the project :). Do you have experience with the Kotlin dependencies? Should Kotlin (or even OkHttp and Kotlin) be shadowed? Or it should not be a problem and it could leave as is to make it easier to just override OkHttp (if needed) in the future, without having to release the new version of GNSP?

Contributor Author

anuraaga commented Feb 18, 2021

@szpak That's an interesting point - okhttp 3.x is still a common dependency and pure Java which is why I use 3.x in most projects, but I picked 4.x here because Gradle mixes 3 languages already :) So Kotlin is already available for free I think and since it needs to be compatible for plugin authors anyways, I'm not too worried about shading. Shading okhttp is also an option, but in my experience they're great on not making backwards incompatible changes (I think they would change the package name when doing so) so I'd lean towards not worrying.

Contributor Author

anuraaga commented Feb 25, 2021

Hi @szpak - I just found https://github.com/gradle-nexus/publish-plugin which seems to be the successor this plugin, and is already using okhttp. Should we go ahead and drop this PR? Or happy to let it merge if it's useful to anyone. I'm going to go ahead and give that plugin a go :D

Member

szpak commented Feb 25, 2021

I'm happy that you proposed that change and I plan to merge it (soon) and release 0.40.0 right after that, probably the last release, unless there are some important bugs found. We created the new unified plugin to make it easier to release to Maven Central, which is important especially these days :).

After checking if the new version of that plugin works fine for you, feel free to switch to the new plugin :-)

Member

szpak commented Feb 25, 2021

okhttp 3.x is still a common dependency and pure Java which is why I use 3.x in most projects, but I picked 4.x here because Gradle mixes 3 languages already :)

Hopefully Gradle will not upgrade Kotlin to the incompatible version anytime soon :)

szpak approved these changes

View reviewed changes

szpak merged commit 5c6ed29 into Codearte:master

Member

szpak commented Feb 26, 2021

I've just released 0.30.0, please let me know if it works in your project, before migrating to gradle-nexus-publish-plugin :-).

Thanks for your contribution!

dependabot bot mentioned this pull request

Bump gradle-nexus-staging-plugin from 0.11.0 to 0.30.0 otto-de/synapse#66

Merged

szpak mentioned this pull request

Got java.lang.NoSuchMethodError when running ./closeAndReleaseRepository #204

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment