Add a warning when `up` builds and a `--build` flag #2601

dnephin · 2016-01-04T22:43:02Z

Fixes #693
Closes #12

Many users have been confused by the default build behaviour of up.

This PR has been updated with the following behaviour:

if the image doesn't exist build it, but warn that this is a one-time thing, and that you need to use --build in the future
add --build to up and create to allow images to be built with a single command

dnephin · 2016-01-04T23:07:46Z

Oh, there are test failures on other hosts. I guess I had images built from previous runs that aren't being removed.

dnephin · 2016-01-07T22:16:40Z

I've added some more context in the description about the tradeoffs and backwards compatibility. Feedback is appreciated so that we can get this into 1.6.

aanand · 2016-01-08T13:47:59Z

For any existing environments, every up will incur an additional cost of building all services. When this is noticed by the user, they will have to go look into the docs and release notes to find out what changed.

We can partially mitigate this with a 2-step release plan: keep the current behaviour in 1.6, but show a warning on up/run if the image exists and they haven't passed --no-build:

$ docker-compose up
WARNING: In the next version of Compose, services with a 'build' key will be built
every time you run `docker-compose up`. If you do not want this behaviour, use
`docker-compose up --no-build` or set COMPOSE_AUTO_BUILD=false.

Creating myapp_web_1
...

(as demonstrated above, it might be nice to provide an environment variable too)

cr7pt0gr4ph7 · 2016-02-08T15:29:48Z

What's the current state on this?
In my opinion, docker-compose up should default to building always (i.e. #12), and allow explictly specifying --no-build if desired.

My reason behind this can be summed up as "make it easy for new users to NOT shoot themselves in the foot when using the tool`; applied to the two alternatives "always build" vs. "never build", this means:

When --build is the default (fig up should build on every invocation #12):
- When a user types in docker-compose up, he gets what he most likely wants: A group of running containers which reflect the current configuration.
- The only reason why one would skip building is performance (which could be considered an "advanced topic").
- When a user decides that rebuilding is not what he wanted, he can just abort the build using Ctrl+C, and then restart up with docker-compose up --no-build.
When --no-build is the default (Proposal: don't "build" as part of "docker-compose up" #693):
- When a user types in docker-compose up in a new environment, he gets a group of running containers which reflect the currently configured state.
- _When a user types in docker-compose up in an existing environment, after he has changed the build context of one or more containers, the configured state does _not* match the current state anymore.
  Even worse, the output of docker-compose up says: <containername> is up-to-date, which is misleading to newbies.*

In my personal expirience, I use docker-compose build <service> && docker-compose up -d <service> way more often than simply docker-compose up -d <service>, simply because:

The former is used when iteratively developing the Dockerfile for a service,
while the latter is used once to set up a finished environment.

I'd therefore argument for optimizing the most common use-case, which is rebuilding and recreating a container, as opposed to recreating a container based on a possible stale image.

"Smart recreate" is not broken by building always

Always build, unless --no-build flag is used (#12)

[...]
Building on every up would lose us "smart recreate". When up is run, we try to only recreate the services that have changed. There's no way to check if a build is stale without running it, so we'd be force to build every image, which changes the image id for every image. Since the image id has changed we have to assume that the service needs to be recreated. So we'd be recreating everything for no reason.

"Smart recreate" is not affected by building on every up. Due to Docker's build cache, the image ID only changes if either:

the instructions in the Dockerfile,
the files in the build context,
or a remote file added using ADD

has changed its contents, and thus no cached version is available.

aanand · 2016-02-08T17:13:08Z

In my personal expirience, I use docker-compose build <service> && docker-compose up -d <service> way more often than simply docker-compose up -d <service>, simply because:

The former is used when iteratively developing the Dockerfile for a service,

while the latter is used once to set up a finished environment.

Not everyone uses Compose this way, and not necessarily even most people. It's a recommended practice to mount your application code in a volume so that you don't have to rebuild the image or even restart your container to pick up changes.

So I'd argue that docker-compose up without a preceding build step is overwhelmingly common for many people.

dnephin · 2016-03-01T01:54:40Z

Ok, this PR has been reworked. The description has been updated with the new behaviour, and the tests are passing.a

It is ready for review.

cr7pt0gr4ph7 · 2016-03-01T13:18:05Z

After thinking a bit about it, I think that I'd be happy with the behavior that is outlined in the PR description. This includes both the first-time warning on up, and the --build flags to be added to up and create.

I'd addtionally suggest two things:

Make it possible to disable the warning on the first invocation of up (e.g. --build-only-once).
Maybe it is possible to merge --build, --no-build and --build-only-once into a single option:
- --build=never: This is equivalent to --no-build.
- --build=once: This is equivalent to the current behavior.
  This should be the default behavior; but if the flag is explicitly specified, the first-time build warning should be skipped.
- --build=always: This is equivalent to the proposed --build flag.
  If only --build is specified (without an explicit mode specification), it should be treated as --build=always.

dnephin · 2016-03-01T15:33:17Z

Sounds reasonable. The reason I went with this flag is because we can't just remove --no-build, since it's been there for a while, so it would be more work to handle that behaviour.

The --build flag already acts as a way to remove the warning, so I'd rather avoid adding another flag for it.

simonvanderveldt · 2016-03-01T22:02:08Z

As a counterweight to @cr7pt0gr4ph7's initial comment I never use docker-compose build let alone have docker-compose up do any building for me. IMHO it shouldn't be part of up at all. (Which was proposed in #693).

Just like @aanand says the "right" way to work on changes within a container is with volume mounts. This is by far the quickest way to iterate. Once I'm done with that I do a docker build and restart the app with compose.

It seems to me the change in this PR is a fair solution to these 2 points of view + solves the many questions/issues that are created because of unexpected behaviour of up.

Regarding the actual argument to up, since we're explicitly enabling building --build makes the most sense to me.
A different option for a one time event (first run) seems unnecessary.

Also I'd suggest creating an issue to not forget to remove --no-build.

dnephin · 2016-03-02T02:54:47Z

--no-build actually still has a very subtle use case. If you know that you've already built the image using --no-build allows you to skip an API call. This might not seem like much, but on a CI server with heavy load and lots of images, it can actually save a little bit of time.

simonvanderveldt · 2016-03-02T09:39:37Z

@dnephin wouldn't it make more sense to make up without an argument not build at all, instead of only sometimes (i.e. when an image can't be found)?
That would take the bit of ambiguity out of the up command regarding builds. If you want to build you use up --build, otherwise you just use up.

dnephin · 2016-03-02T15:19:12Z

That's what I wanted to do originally (see #693), but we came to a consensus with this approach instead.

aanand · 2016-03-02T19:31:22Z

compose/service.py

+        log.warn(
+            "Image for service {} was build because it was not found. To "
+            "rebuild this image you must use the `build` command or the "
+            "--build flag.".format(self.name))


Typo: "was build" -> "was built"

Perhaps "because it did not already exist"?

Also, might be nice to specify the exact commands:

To rebuild this image, use docker-compose build or docker-compose up --build.

dnephin · 2016-03-02T20:32:50Z

Fixed the warning test

Also adds a warning when up builds an image without the --build flag so that users know it wont happen on the next up. Signed-off-by: Daniel Nephin <[email protected]>

Signed-off-by: Daniel Nephin <[email protected]>

aanand · 2016-03-02T23:02:16Z

LGTM

Only build as part of `up` if `--build` flag is set

dnephin added the area/up label Jan 4, 2016

dnephin added this to the 1.6.0 milestone Jan 4, 2016

GordonTheTurtle added the status/0-triage label Jan 4, 2016

dnephin changed the title ~~Only build if --build flag is set~~ Only build as part of up if --build flag is set Jan 7, 2016

aanand modified the milestones: 1.6.0, 1.7.0 Jan 13, 2016

dnephin force-pushed the dont_build_on_up branch 3 times, most recently from 0b09c62 to a6d06ed Compare February 29, 2016 22:02

aanand reviewed Mar 2, 2016
View reviewed changes

dnephin force-pushed the dont_build_on_up branch from a6d06ed to e1b87d7 Compare March 2, 2016 20:32

dnephin added 2 commits March 2, 2016 15:32

Add --build flag for up and create

b7fb3a6

Also adds a warning when up builds an image without the --build flag so that users know it wont happen on the next up. Signed-off-by: Daniel Nephin <[email protected]>

Update reference docs for the new flag.

e1b87d7

Signed-off-by: Daniel Nephin <[email protected]>

aanand added a commit that referenced this pull request Mar 2, 2016

Merge pull request #2601 from dnephin/dont_build_on_up

1655be6

Only build as part of `up` if `--build` flag is set

aanand merged commit 1655be6 into docker:master Mar 2, 2016

dnephin deleted the dont_build_on_up branch March 2, 2016 23:24

dnephin changed the title ~~Only build as part of up if --build flag is set~~ Add a warning when up builds and a --build flag Mar 4, 2016

dnephin mentioned this pull request Mar 7, 2016

recreate container when image or dockerfile is updated #1383

Closed

dnephin mentioned this pull request Mar 21, 2016

Document that fig does not always rebuild images automatically #614

Closed

giavac mentioned this pull request Mar 31, 2016

docker_compose doesn't use the latest image for a service, after it's rebuilt garethr/garethr-docker#491

Open

dylanscott mentioned this pull request Sep 14, 2016

Expose docker-compose up --build flag xeger/docker-compose#39

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a warning when `up` builds and a `--build` flag #2601

Add a warning when `up` builds and a `--build` flag #2601

dnephin commented Jan 4, 2016

dnephin commented Jan 4, 2016

dnephin commented Jan 7, 2016

aanand commented Jan 8, 2016

cr7pt0gr4ph7 commented Feb 8, 2016

Always build, unless --no-build flag is used (#12)

aanand commented Feb 8, 2016

dnephin commented Mar 1, 2016

cr7pt0gr4ph7 commented Mar 1, 2016

dnephin commented Mar 1, 2016

simonvanderveldt commented Mar 1, 2016

dnephin commented Mar 2, 2016

simonvanderveldt commented Mar 2, 2016

dnephin commented Mar 2, 2016

aanand Mar 2, 2016

dnephin commented Mar 2, 2016

aanand commented Mar 2, 2016

Add a warning when up builds and a --build flag #2601

Add a warning when up builds and a --build flag #2601

Conversation

dnephin commented Jan 4, 2016

dnephin commented Jan 4, 2016

dnephin commented Jan 7, 2016

aanand commented Jan 8, 2016

cr7pt0gr4ph7 commented Feb 8, 2016

"Smart recreate" is not broken by building always

Always build, unless --no-build flag is used (#12)

aanand commented Feb 8, 2016

dnephin commented Mar 1, 2016

cr7pt0gr4ph7 commented Mar 1, 2016

dnephin commented Mar 1, 2016

simonvanderveldt commented Mar 1, 2016

dnephin commented Mar 2, 2016

simonvanderveldt commented Mar 2, 2016

dnephin commented Mar 2, 2016

aanand Mar 2, 2016

Choose a reason for hiding this comment

dnephin commented Mar 2, 2016

aanand commented Mar 2, 2016

Add a warning when `up` builds and a `--build` flag #2601

Add a warning when `up` builds and a `--build` flag #2601