Use spotify docker-client for docker:publishLocal goal

[gbougeard]

Handle #558 only for publishLocal.

publish is a little bit complex as it requires to handle push to private repositories and credentials.

[muuki88]

Sweet. Looks pretty simple to me. One big thing. We shouldn't replace the native implementation, but instead provide a plugin overriding the native task. See the JDebPackaging as an example

[gbougeard]

You mean not just a val dockerUseNativeClient = SettingKey[Boolean]("dockerUseNativeClient", "Use native client for publishLocal") ?
You prefer to transform object DockerPlugin into trait DockerPluginLike and a object DockerPluginNative and object DockerPluginClient ?
I'm not familiar with sbt plugins development yet :)

[muuki88]

I had to do this for the Debian stuff as it's a lot more messy and it was written before autoplugins were introduced.

So for docker you would put exactly your code in a DockerSpotifyClientPlugin autoplugin that requires the normal DockerPlugin. SBT then loads the DockerPlugin, which sets all sensible defaults. After that the DockerSpotifyClientPlugin gets initialised and overwrites all the "native" task, that that use the CLI.

By this we keep the Spotify part to a minimum and makes use of improvements in the core plugin.

[muuki88]

Adding conversation from gitter (@jsuereth , @fiadliel )

gbougeard
hi, I might do a PR about #558 but in a first time handling only publishLocal. publishis a little bit > complex as it needs to handle credentials and push to private repositories. Are you interested?

jsuereth 12:17
Regarding credentials, what http client does it use? Ivy will automatically configure the native JDK one

fiadliel 12:33
@gbougeard I think it would be cool. Though I’m concerned by extra dependencies in sbt-native-packager, especially a common one like async-http-client (a lot of people won’t be able to upgrade this library because they’re tied by Play Framework). My preference would be to have it as an external plugin, with some really easy way to enable it.
gbougeard 12:54

@fiadliel I did use spotify docker-client in publishLocal and just needed to add it as depencency (an no other one).
@jsuereth it uses https://github.com/spotify/docker-client/blob/master/pom.xml#L130
fiadliel 12:59

@gbougeard that’s not the problem; it’s when this plugin needs one version of httpclient, and another plugin needs another (incompatible) version; as a required dependency of a popular system like Play, I’d consider it an important thing to bear in mind. Making this functionality optional is a relatively cheap way of providing some extra flexibility around library dependencies. But I’m not a maintainer, it’s just my 2c.

[gbougeard]

@muuki88 I think I coded it but I'm getting trouble on how to activate DockerSpotifyClientPlugin.
I thought adding enablePlugins(DockerSpotifyClientPlugin) would be enough but when I launch docker:publishLocal it still uses the Native implementation (so DockerPlugin instead of DockerSpotifyClientPlugin)

[muuki88]

Can you push your code here? otherwise it's hard to say what went wrong ;)

[gbougeard]

I finally did it.
I modified the test-project-docker to use the DockerSpotifyClientPlugin and it builds the image using the Remote API.

[muuki88]

That looks pretty smooth. The only thing I'm still struggling is, if we should put this in sbt-native-packager itself or in a new project and feature it here.

@fiadliel has a point. We don't know on what libraries clients depend and what conflicts can occur. The spotify client depends on a few stuff that can really be critical (even for this lib alone)

• org.bouncycastle
• apache compress

On the other hand, keeping it here in the code base it's a lot easier for users and for developers to use (in case of no conflicts)

[muuki88]

@gbougeard can you squash the commits into one? Actually I want to merge this and release an milestone release, hoping to gather some feedback.

[gbougeard]

I agree but spotify docker-client is a java lib, how could it be done another way?

[muuki88]

For example

Option(message.error()) match {
  case Some(error) if error.nonEmpty => log.error(message.error())
  case _ => log.info(message.stream())
}

[gbougeard]

Is it ok for you now?

[bjaglin]

this is actually not a requirement, via an env var you can just point at a remote endpoint

[muuki88]

Awesome. I release a soon I have time

[gbougeard]

Great we need that milestone release :)

[muuki88]

@gbougeard 1.0.5-M1 is out