Merge pull request #2601 from dnephin/dont_build_on_up

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

compose/cli/main.py

@@ -25,6 +25,7 @@
 from ..const import IS_WINDOWS_PLATFORM
 from ..progress_stream import StreamOutputError
 from ..project import NoSuchService
+from ..service import BuildAction
 from ..service import BuildError
 from ..service import ConvergenceStrategy
 from ..service import ImageType
@@ -249,14 +250,15 @@ def create(self, project, options):
                                    image haven't changed. Incompatible with --no-recreate.
             --no-recreate          If containers already exist, don't recreate them.
                                    Incompatible with --force-recreate.
-            --no-build             Don't build an image, even if it's missing
+            --no-build             Don't build an image, even if it's missing.
+            --build                Build images before creating containers.
         """
         service_names = options['SERVICE']
 
         project.create(
             service_names=service_names,
             strategy=convergence_strategy_from_opts(options),
-            do_build=not options['--no-build']
+            do_build=build_action_from_opts(options),
         )
 
     def down(self, project, options):
@@ -699,7 +701,8 @@ def up(self, project, options):
                                        Incompatible with --no-recreate.
             --no-recreate              If containers already exist, don't recreate them.
                                        Incompatible with --force-recreate.
-            --no-build                 Don't build an image, even if it's missing
+            --no-build                 Don't build an image, even if it's missing.
+            --build                    Build images before starting containers.
             --abort-on-container-exit  Stops all containers if any container was stopped.
                                        Incompatible with -d.
             -t, --timeout TIMEOUT      Use this timeout in seconds for container shutdown
@@ -721,7 +724,7 @@ def up(self, project, options):
                 service_names=service_names,
                 start_deps=start_deps,
                 strategy=convergence_strategy_from_opts(options),
-                do_build=not options['--no-build'],
+                do_build=build_action_from_opts(options),
                 timeout=timeout,
                 detached=detached)
 
@@ -775,6 +778,19 @@ def image_type_from_opt(flag, value):
         raise UserError("%s flag must be one of: all, local" % flag)
 
 
+def build_action_from_opts(options):
+    if options['--build'] and options['--no-build']:
+        raise UserError("--build and --no-build can not be combined.")
+
+    if options['--build']:
+        return BuildAction.force
+
+    if options['--no-build']:
+        return BuildAction.skip
+
+    return BuildAction.none
+
+
 def run_one_off_container(container_options, project, service, options):
     if not options['--no-deps']:
         deps = service.get_dependency_names()

compose/project.py

@@ -21,6 +21,7 @@
 from .network import build_networks
 from .network import get_networks
 from .network import ProjectNetworks
+from .service import BuildAction
 from .service import ContainerNetworkMode
 from .service import ConvergenceStrategy
 from .service import NetworkMode
@@ -249,7 +250,12 @@ def build(self, service_names=None, no_cache=False, pull=False, force_rm=False):
             else:
                 log.info('%s uses an image, skipping' % service.name)
 
-    def create(self, service_names=None, strategy=ConvergenceStrategy.changed, do_build=True):
+    def create(
+        self,
+        service_names=None,
+        strategy=ConvergenceStrategy.changed,
+        do_build=BuildAction.none,
+    ):
         services = self.get_services_without_duplicate(service_names, include_deps=True)
 
         plans = self._get_convergence_plans(services, strategy)
@@ -298,7 +304,7 @@ def up(self,
            service_names=None,
            start_deps=True,
            strategy=ConvergenceStrategy.changed,
-           do_build=True,
+           do_build=BuildAction.none,
            timeout=DEFAULT_TIMEOUT,
            detached=False):
 

compose/service.py

@@ -104,6 +104,14 @@ class ImageType(enum.Enum):
     all = 2
 
 
+@enum.unique
+class BuildAction(enum.Enum):
+    """Enumeration for the possible build actions."""
+    none = 0
+    force = 1
+    skip = 2
+
+
 class Service(object):
     def __init__(
         self,
@@ -243,7 +251,7 @@ def stop_and_remove(container):
 
     def create_container(self,
                          one_off=False,
-                         do_build=True,
+                         do_build=BuildAction.none,
                          previous_container=None,
                          number=None,
                          quiet=False,
@@ -266,20 +274,29 @@ def create_container(self,
 
         return Container.create(self.client, **container_options)
 
-    def ensure_image_exists(self, do_build=True):
+    def ensure_image_exists(self, do_build=BuildAction.none):
+        if self.can_be_built() and do_build == BuildAction.force:
+            self.build()
+            return
+
         try:
             self.image()
             return
         except NoSuchImageError:
             pass
 
-        if self.can_be_built():
-            if do_build:
-                self.build()
-            else:
-                raise NeedsBuildError(self)
-        else:
+        if not self.can_be_built():
             self.pull()
+            return
+
+        if do_build == BuildAction.skip:
+            raise NeedsBuildError(self)
+
+        self.build()
+        log.warn(
+            "Image for service {} was built because it did not already exist. To "
+            "rebuild this image you must use `docker-compose build` or "
+            "`docker-compose up --build`.".format(self.name))
 
     def image(self):
         try:
@@ -343,7 +360,7 @@ def _containers_have_diverged(self, containers):
 
     def execute_convergence_plan(self,
                                  plan,
-                                 do_build=True,
+                                 do_build=BuildAction.none,
                                  timeout=DEFAULT_TIMEOUT,
                                  detached=False,
                                  start=True):
@@ -392,7 +409,7 @@ def execute_convergence_plan(self,
     def recreate_container(
             self,
             container,
-            do_build=False,
+            do_build=BuildAction.none,
             timeout=DEFAULT_TIMEOUT,
             attach_logs=False,
             start_new_container=True):

docs/reference/create.md

@@ -12,14 +12,15 @@ parent = "smn_compose_cli"
 # create
 
 ```
+Creates containers for a service.
+
 Usage: create [options] [SERVICE...]
 
 Options:
---force-recreate       Recreate containers even if their configuration and
-                       image haven't changed. Incompatible with --no-recreate.
---no-recreate          If containers already exist, don't recreate them.
-                       Incompatible with --force-recreate.
---no-build             Don't build an image, even if it's missing
+    --force-recreate       Recreate containers even if their configuration and
+                           image haven't changed. Incompatible with --no-recreate.
+    --no-recreate          If containers already exist, don't recreate them.
+                           Incompatible with --force-recreate.
+    --no-build             Don't build an image, even if it's missing.
+    --build                Build images before creating containers.
 ```
-
-Creates containers for a service.

docs/reference/up.md

@@ -15,22 +15,24 @@ parent = "smn_compose_cli"
 Usage: up [options] [SERVICE...]
 
 Options:
--d                         Detached mode: Run containers in the background,
-                           print new container names.
-                           Incompatible with --abort-on-container-exit.
---no-color                 Produce monochrome output.
---no-deps                  Don't start linked services.
---force-recreate           Recreate containers even if their configuration
-                           and image haven't changed.
-                           Incompatible with --no-recreate.
---no-recreate              If containers already exist, don't recreate them.
-                           Incompatible with --force-recreate.
---no-build                 Don't build an image, even if it's missing
---abort-on-container-exit  Stops all containers if any container was stopped.
-                           Incompatible with -d.
--t, --timeout TIMEOUT      Use this timeout in seconds for container shutdown
-                           when attached or when containers are already
-                           running. (default: 10)
+    -d                         Detached mode: Run containers in the background,
+                               print new container names.
+                               Incompatible with --abort-on-container-exit.
+    --no-color                 Produce monochrome output.
+    --no-deps                  Don't start linked services.
+    --force-recreate           Recreate containers even if their configuration
+                               and image haven't changed.
+                               Incompatible with --no-recreate.
+    --no-recreate              If containers already exist, don't recreate them.
+                               Incompatible with --force-recreate.
+    --no-build                 Don't build an image, even if it's missing.
+    --build                    Build images before starting containers.
+    --abort-on-container-exit  Stops all containers if any container was stopped.
+                               Incompatible with -d.
+    -t, --timeout TIMEOUT      Use this timeout in seconds for container shutdown
+                               when attached or when containers are already
+                               running. (default: 10)
+
 ```
 
 Builds, (re)creates, starts, and attaches to containers for a service.

tests/unit/service_test.py

@@ -2,6 +2,7 @@
 from __future__ import unicode_literals
 
 import docker
+import pytest
 from docker.errors import APIError
 
 from .. import mock
@@ -15,6 +16,7 @@
 from compose.container import Container
 from compose.service import build_ulimits
 from compose.service import build_volume_binding
+from compose.service import BuildAction
 from compose.service import ContainerNetworkMode
 from compose.service import get_container_data_volumes
 from compose.service import ImageType
@@ -427,7 +429,12 @@ def test_create_container_with_build(self):
             '{"stream": "Successfully built abcd"}',
         ]
 
-        service.create_container(do_build=True)
+        with mock.patch('compose.service.log', autospec=True) as mock_log:
+            service.create_container(do_build=BuildAction.none)
+            assert mock_log.warn.called
+            _, args, _ = mock_log.warn.mock_calls[0]
+            assert 'was built because it did not already exist' in args[0]
+
         self.mock_client.build.assert_called_once_with(
             tag='default_foo',
             dockerfile=None,
@@ -444,14 +451,37 @@ def test_create_container_no_build(self):
         service = Service('foo', client=self.mock_client, build={'context': '.'})
         self.mock_client.inspect_image.return_value = {'Id': 'abc123'}
 
-        service.create_container(do_build=False)
+        service.create_container(do_build=BuildAction.skip)
         self.assertFalse(self.mock_client.build.called)
 
     def test_create_container_no_build_but_needs_build(self):
         service = Service('foo', client=self.mock_client, build={'context': '.'})
         self.mock_client.inspect_image.side_effect = NoSuchImageError
-        with self.assertRaises(NeedsBuildError):
-            service.create_container(do_build=False)
+        with pytest.raises(NeedsBuildError):
+            service.create_container(do_build=BuildAction.skip)
+
+    def test_create_container_force_build(self):
+        service = Service('foo', client=self.mock_client, build={'context': '.'})
+        self.mock_client.inspect_image.return_value = {'Id': 'abc123'}
+        self.mock_client.build.return_value = [
+            '{"stream": "Successfully built abcd"}',
+        ]
+
+        with mock.patch('compose.service.log', autospec=True) as mock_log:
+            service.create_container(do_build=BuildAction.force)
+
+        assert not mock_log.warn.called
+        self.mock_client.build.assert_called_once_with(
+            tag='default_foo',
+            dockerfile=None,
+            stream=True,
+            path='.',
+            pull=False,
+            forcerm=False,
+            nocache=False,
+            rm=True,
+            buildargs=None,
+        )
 
     def test_build_does_not_pull(self):
         self.mock_client.build.return_value = [