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

Docker mappings - incomplete? #424

Merged
merged 1 commit into from
Dec 18, 2014
Merged

Docker mappings - incomplete? #424

merged 1 commit into from
Dec 18, 2014

Conversation

huntc
Copy link
Contributor

@huntc huntc commented Dec 15, 2014

I've just noticed that docker:mappings does not include the DockerFile that is produced. The sbt-bundle plugin uses docker:mappings as a general approach to including files when staging or producing a dist (it actually doesn't know anothing about the 'docker' config as this a setting to it). Should docker:mappings also include the DockerFile itself? I'm thinking so...

As an alternative the File result of stage could be used... only again it does not include the DockerFile as it points to /files. I'd prefer to use mappings though anyhow given that it retains the relative path.

What are your thoughts? mappings appears to be inclusive of the entire package with the other configurations doesn't it? /cc @jsuereth

@huntc
Copy link
Contributor Author

huntc commented Dec 2, 2014

Any thoughts anyone? Shall I raise a PR?

@huntc huntc added the bug label Dec 2, 2014
@muuki88
Copy link
Contributor

muuki88 commented Dec 13, 2014

Sorry for responding so late.

Yes, the file is missing. I'm not sure about the meaning of packaging the
Dockerfile within the package. For me it sounds like packaging the build.sbt
into a jar. However if there are good reasons, feel free to raise a pr :)

cc @fiadliel

@huntc
Copy link
Contributor Author

huntc commented Dec 15, 2014

The reason to package the DockerFile is down to determining what the best way is to ship a Docker image. You can certainly publish to a repo, but what if I want to pass it around, perhaps even in some offline mode? I don't want to pass the image itself because they are too big. Instead, I want to package the DockerFile and its associated files so that I can pass it around and retain it for posterity. The archive is then quite small. Make sense?

I shall work on this PR right now. Thanks.

The Dockerfile wasn't being included in the `mappings` setting, but it now is. Along the way I took the opportunity of cleaning up how stage works, bringing it more inline with the approach taken by the Universal plugin.

One change that occurred as a result of the above is that `target/docker` is now used to generate working files (`Dockerfile`) as well as holding a `stage` folder. The `stage` folder contains the `Dockerfile` for staging add its context files. This is very similar to the structure of the `target/universal` folder.
@huntc
Copy link
Contributor Author

huntc commented Dec 15, 2014

@muuki88 @fiadliel I've now morphed this issue into a PR. Lemme know what you think.

@huntc
Copy link
Contributor Author

huntc commented Dec 15, 2014

Provisionally released as 1.0.0-5d703a5ccc1e7a2dce305fdbe4260a80c5c6a676.

@fiadliel
Copy link
Contributor

I considered the Docker target as a runtime "archive", as much as RPMs, Debs, etc. You wouldn't expect the debian/ folder to be present in a Debian package, it just describes how to create a Debian package. Same with RPM SPEC files. The Dockerfile is completely unnecessary for runtime, so I would prefer if it were a choice, not the default.

What would you think if we had something like a docker:packageSrc command which archives up target/docker/stage for convenience?

@huntc
Copy link
Contributor Author

huntc commented Dec 15, 2014

The native packager components have both a staging folder and an archive. Staging is where files are prepared for the purposes of archiving. In the case of Docker archiving is the act of building an image from the staged files and publishing it. There is no Dockerfile included in the image that is published.

All that I've done here (aside from some tidy-up) is added the DockerFile to the mappings property. mappings provide the input to the stage task.

Does that help?

@@ -117,9 +116,10 @@ object DockerPlugin extends AutoPlugin {
}

val dockerCommands = Seq(
Cmd("ADD", "files /"),
Cmd("ADD", "* /"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always include the Dockerfile, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it will, hence the removal of it further down. I could be more explicit about what is included if you want me to. This seemed like the simpler approach though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will the internal path change? I mean, currently it is /opt/docker/<app> and for me
it looks like /stage/opt/docker/<app> now

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah... the stagingDirectory is used for this?

@huntc
Copy link
Contributor Author

huntc commented Dec 16, 2014

Any movement on this? Don't like?

@huntc
Copy link
Contributor Author

huntc commented Dec 17, 2014

Just to provide a little more background, we have another plugin named sbt-bundle which is for our new Reactive Runtime project. The idea of this plugin is to take all files for any archive produced by the native packager and we use mappings to source these files. It then "bundles" these files into a tgz, takes the digest and encodes the digest in the filename i.e. it fingerprints the filename so that you have a filename representing the contents. It also includes a "bundle descriptor" describing the thing that was bundled.

The user declares a bundleType setting when using sbt-bundle and assigns a config. By default Universal is used. However you can do this:

bundleType := Docker

The config declared above is then used to scope as mappings in bundleType.value effectively yielding mappings in Docker in the case above.

sbt-bundle itself has no knowledge of the types of archive that can be produced by the native packager. In fact all that it really wants is the mappings and the config as described. The plugin does depend on the native packager for a few other things though such as the archive name and stuff like that.

Does this make more sense now?

@muuki88
Copy link
Contributor

muuki88 commented Dec 17, 2014

I'm willing to merge this pull request as we will do a lot more refactoring
withing 1.0.0-Mx as we have a lot of request. Actually I'm thinking about providing
more flexibility, but I haven't made up my mind yet what would be the best way.

Main points users want to customize (and are not yet available ) are

  • CMD
  • ENTRYPOINT

I'm not sure if there should be a single task which generates the list of docker commands.
E.g.

val dockerFileCommands = taskKey[Seq[Cmd]]

WDYT?

huntc added a commit that referenced this pull request Dec 18, 2014
Docker mappings - incomplete?
@huntc huntc merged commit 300ed33 into sbt:master Dec 18, 2014
@huntc huntc deleted the docker-stage branch December 18, 2014 03:47
@huntc
Copy link
Contributor Author

huntc commented Dec 18, 2014

Thanks for the review - should we do another release?

@huntc
Copy link
Contributor Author

huntc commented Dec 18, 2014

Main points users want to customize (and are not yet available ) are

CMD
ENTRYPOINT
I'm not sure if there should be a single task which generates the list of docker commands.
E.g.

val dockerFileCommands = taskKey[Seq[Cmd]]
WDYT

May be we could allow the user to specify a function as a task where the function receives the settings that we already use to form a DockerFile so that they can customise the Dockerfile easily within the context of sbt. So the goal of the function would be a string containing the contents of the Dockerfile. The default setting for this function could be something along the lines of the function we have presently.

@muuki88
Copy link
Contributor

muuki88 commented Dec 18, 2014

I'll clean up the documentation (at least to a point where every example works for both the 0.8.x and the 1.0.x versions) and the we do another round of releases. Is this weekend early enough for you?

@huntc
Copy link
Contributor Author

huntc commented Dec 18, 2014

Sure thing. No rush! Thanks.

@muuki88
Copy link
Contributor

muuki88 commented Dec 28, 2014

@huntc . This pull request broke most of the docker tests. They all fail with

[error] 2014/12/28 13:53:51 *: no such file or directory
java.lang.RuntimeException: Nonzero exit value: 1

Did this happen on your system as well? My configuration

  • Ubuntu 14.04 LTS
  • Docker version 1.2.0, build fa7b24f

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

The PR validator suceeded.

The Docker tests also suceeded on my machine (OS X).

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

The build is also passing according to the indicator in the README.

@muuki88
Copy link
Contributor

muuki88 commented Dec 28, 2014

The docker tests don't run on the automated tests, We haven't figured out how to configure
travis-ci to run our docker tests.

We have two options.

  1. I'm reverting the PR and we try a new one and I'm releasing the current state as M4
  2. We fix this issue

IMHO, I prefer option 1 and I'll prepare a bigger refactoring for the docker plugin, which
includes your changes :)

@muuki88
Copy link
Contributor

muuki88 commented Dec 28, 2014

DId you run sbt scripted docker/* ?

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

I can take a look right now

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

Running sbt clean "scripted docker/*" passes on my machine with the branch I had.

Now trying the master branch...

BTW: Is docker itself working correctly on your machine? Can you do a docker ps?

@muuki88
Copy link
Contributor

muuki88 commented Dec 28, 2014

docker ps works fine on my machine.

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

OK, the master branch is passing on my machine... docker -v yields:

Docker version 1.3.2, build 39fa2fa

I dunno whether that'd make a difference...

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

Which of the docker tests are failing?

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

This is a long shot, but could you please try inserting the following:

stageDirectory.mkdirs

...before this line: https://github.com/sbt/sbt-native-packager/blob/master/src/main/scala/com/typesafe/sbt/packager/Stager.scala#L22

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

Also, could you trying running the test-project-docker and performing a docker:stage from there? It should be faster to debug things that way.

@muuki88
Copy link
Contributor

muuki88 commented Dec 28, 2014

Already tried. I get the same error as in all the tests

*: no such file or directory

The ADD * / command doesn't seem to work correctly. When I run my docker client
directly from the cli I get the same error.

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

So you should have a test-project-docker/target/docker folder. cd into that and try a docker build --force-rm -t something . from there.

@muuki88
Copy link
Contributor

muuki88 commented Dec 28, 2014

Yep. Already did that. I tried it again with this output:

[muki@Proteus test-project-docker]$ cd target/docker 
[muki@Proteus docker]$ ls
Dockerfile  stage
[muki@Proteus docker]$ docker build --force-rm -t something .
Sending build context to Docker daemon 7.153 MB
Sending build context to Docker daemon 
Step 0 : FROM dockerfile/java:latest
 ---> 93894ab6d6a4
Step 1 : MAINTAINER Gary Coady <[email protected]>
 ---> Using cache
 ---> b8b64521c218
Step 2 : ADD * /
2014/12/29 00:08:22 *: no such file or directory

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

I just noticed a commit on Docker that may be having an effect: moby/moby#8076

Can you try upgrading your docker?

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

It looks as though 1.3.0 introduced the support for wildcards back in October: https://github.com/docker/docker/blob/master/CHANGELOG.md#130-2014-10-14

Docker is now also at 1.4.1. We should probably document what our baseline Docker support is WDYT?

@muuki88
Copy link
Contributor

muuki88 commented Dec 28, 2014

My last ubuntu update removed the docker ppas. Still nice to know that we need docker 1.3 or higher?

Works now.

@muuki88
Copy link
Contributor

muuki88 commented Dec 28, 2014

Yep. We need to document this. Most of the strange docker errors come from version gaps

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

What also really need are our tests working on CI so that we can enforce the version constraint.

Do you want me to raise a PR stipulating 1.3?

@muuki88
Copy link
Contributor

muuki88 commented Dec 28, 2014

That would be awesome :)
If you have any experience running docker in travisci :) :)

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

Yeah, travis has a problem running Docker - I have another project that would benefit from this.

@huntc
Copy link
Contributor Author

huntc commented Dec 28, 2014

PR raised: #448

huntc added a commit to huntc/sbt-native-packager that referenced this pull request Jan 7, 2015
PR sbt#424 moved to a ADD wildcard approach, and then removed the Dockerfile. There were two problems with this:
* ADD seems to behave weirdly regarding recursion. /opt/docker and /docker were both added to the output.
* I had incorrectly set the current working directory to the parent of the context directory, where it really should be the context directory itself. This only worked because there was a Dockerfile in the parent also.
huntc added a commit to huntc/sbt-native-packager that referenced this pull request Jan 8, 2015
PR sbt#424 moved to a ADD wildcard approach, and then removed the Dockerfile. There were two problems with this:
* ADD seems to behave weirdly regarding recursion. /opt/docker and /docker were both added to the output.
* I had incorrectly set the current working directory to the parent of the context directory, where it really should be the context directory itself. This only worked because there was a Dockerfile in the parent also.
littleRoundaer pushed a commit to littleRoundaer/sbt-native-packager that referenced this pull request Jul 19, 2022
PR sbt/sbt-native-packager#424 moved to a ADD wildcard approach, and then removed the Dockerfile. There were two problems with this:
* ADD seems to behave weirdly regarding recursion. /opt/docker and /docker were both added to the output.
* I had incorrectly set the current working directory to the parent of the context directory, where it really should be the context directory itself. This only worked because there was a Dockerfile in the parent also.
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.

3 participants