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

Remove netty transport, rework shading #1307

Merged
merged 11 commits into from
Mar 19, 2019
Merged

Remove netty transport, rework shading #1307

merged 11 commits into from
Mar 19, 2019

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Mar 14, 2019

We were running OkHttp transport by default since version 1.9.0 without any (reported) issues.
Since Netty shading caused some problems in the past (especially with native dependencies), it doesn't make sense to keep it as an alternative transport because there aren't any features that supported by Netty but not by OkHttp.

Note that once docker-java is split into modules, we should contribute the OkHttp transport back to docker-java and remove the shading all together.

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Provides such a nice cleanup and removes complexity from the build, thanks!

project.afterEvaluate {
dependencies {
for (id in project.configurations.compile.resolvedConfiguration.resolvedArtifacts*.moduleVersion*.id) {
exclude(dependency("${id.group}:${id.name}"))
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting - what's this doing? It looks like we're excluding all transitive deps. Is this in order to make all deps explicit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Read it as: "shade everything, except the dependencies from the compile scope". It means that if docker-java adds a dependency on X, it will never appear in our dependencies unless we add it to compile scope (aka add it as a public dependency of the library)

Copy link
Member

Choose a reason for hiding this comment

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

Got it 👍

@rnorth
Copy link
Member

rnorth commented Mar 18, 2019

Would you like to do the honours and merge, @bsideup? 😄

@bsideup
Copy link
Member Author

bsideup commented Mar 19, 2019

@rnorth :D yeah, waiting for CI status after the master merge, will merge shortly after

@bsideup bsideup merged commit 99e2c1c into master Mar 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the remove_netty_transport branch March 19, 2019 10:00
@rnorth
Copy link
Member

rnorth commented Mar 22, 2019

Releasing this in 1.11.0 🎉

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

Successfully merging this pull request may close these issues.

None yet

3 participants