Add support for --chown flag for ADD/COPY Docker commands #1044
Conversation
|
Hi @mrfyda, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
|
Thanks a lot for taking the time to work on this. I'll review this when I'm back from vacation ( 22.10 ) 😍 |
| Cmd("ADD", s"$files /$files") | ||
|
|
||
| dockerVersion match { | ||
| case Some(DockerVersion(major, minor, _, _)) if major >= 17 && minor >= 9 => |
There was a problem hiding this comment.
The if statement looks buggy to me.
If the server has version 18.0 it will yield false, which is not want we want, right?
It must be something like if majro >= (17 && minor >= 9) || major > 17
At this point I'm fine with putting this into a if statement. If we have more of this special case, we should
extract this in somethin like a DockerSupport object:
object DockerSupport {
def chownFlag(version: DockerVersion): Boolean = ...
}
|
I updated the branch so all tests should work fine now. |
|
@muuki88 Nice catch. Fixed the condition and extracted it to a separate object. Also, updated both the spotify docker client implementation and the docs. |
|
Thanks @mrfyda 💪🤗 |
|
@muuki88 Please cut a release :) |
|
Sure sure sure 😜 Merged one last PR today. With sbt cross build and all scripted tests a release always |
|
👍 |
| @@ -80,6 +83,9 @@ object DockerPlugin extends AutoPlugin { | |||
| dockerEntrypoint := Seq("bin/%s" format executableScriptName.value), | |||
| dockerCmd := Seq(), | |||
| dockerExecCommand := Seq("docker"), | |||
| dockerVersion := Try(Process(dockerExecCommand.value ++ Seq("version --format '{{.Server.Version}}'")).!!) | |||
There was a problem hiding this comment.
Guys, I don't think this works...
I believe it needs to be Seq("version", "--format", "'{{.Server.Version}}'"), otherwise your shell will add another set of ticks around the whole thing and Docker will report that it does not know the command.
I will try to confirm this and send a PR is necessary. I'd love to use this feature :)
There was a problem hiding this comment.
nvm me, already fixed somewhere else :D
ghost
mentioned this pull request
Closes #1029