Show an error on 'run' when there are legacy one-off containers

Also warn the user about the one-off containers in the standard error
message about legacy containers.

Signed-off-by: Aanand Prasad <[email protected]>

compose/cli/main.py

@@ -34,7 +34,7 @@ def main():
     except KeyboardInterrupt:
         log.error("\nAborting.")
         sys.exit(1)
-    except (UserError, NoSuchService, ConfigurationError, legacy.LegacyContainersError) as e:
+    except (UserError, NoSuchService, ConfigurationError, legacy.LegacyError) as e:
         log.error(e.msg)
         sys.exit(1)
     except NoSuchCommand as e:
@@ -336,12 +336,22 @@ class TopLevelCommand(Command):
         if not options['--service-ports']:
             container_options['ports'] = []
 
-        container = service.create_container(
-            quiet=True,
-            one_off=True,
-            insecure_registry=insecure_registry,
-            **container_options
-        )
+        try:
+            container = service.create_container(
+                quiet=True,
+                one_off=True,
+                insecure_registry=insecure_registry,
+                **container_options
+            )
+        except APIError as e:
+            legacy.check_for_legacy_containers(
+                project.client,
+                project.name,
+                [service.name],
+                allow_one_off=False,
+            )
+
+            raise e
 
         if options['-d']:
             service.start_container(container)

compose/legacy.py

@@ -1,6 +1,7 @@
 import logging
 import re
 
+from .const import LABEL_VERSION
 from .container import get_container_name, Container
 
 
@@ -24,41 +25,82 @@ Alternatively, remove them:
     $ docker rm -f {rm_args}
 """
 
+ONE_OFF_ADDENDUM_FORMAT = """
+You should also remove your one-off containers:
+
+    $ docker rm -f {rm_args}
+"""
+
+ONE_OFF_ERROR_MESSAGE_FORMAT = """
+Compose found the following containers without labels:
+
+{names_list}
+
+As of Compose 1.3.0, containers are identified with labels instead of naming convention.
+
+Remove them before continuing:
+
+    $ docker rm -f {rm_args}
+"""
+
 
 def check_for_legacy_containers(
         client,
         project,
         services,
-        stopped=False,
-        one_off=False):
+        allow_one_off=True):
     """Check if there are containers named using the old naming convention
     and warn the user that those containers may need to be migrated to
     using labels, so that compose can find them.
     """
-    containers = list(get_legacy_containers(
-        client,
-        project,
-        services,
-        stopped=stopped,
-        one_off=one_off))
+    containers = get_legacy_containers(client, project, services, one_off=False)
 
     if containers:
-        raise LegacyContainersError([c.name for c in containers])
+        one_off_containers = get_legacy_containers(client, project, services, one_off=True)
+
+        raise LegacyContainersError(
+            [c.name for c in containers],
+            [c.name for c in one_off_containers],
+        )
+
+    if not allow_one_off:
+        one_off_containers = get_legacy_containers(client, project, services, one_off=True)
+
+        if one_off_containers:
+            raise LegacyOneOffContainersError(
+                [c.name for c in one_off_containers],
+            )
+
+
+class LegacyError(Exception):
+    def __unicode__(self):
+        return self.msg
+
+    __str__ = __unicode__
 
 
-class LegacyContainersError(Exception):
-    def __init__(self, names):
+class LegacyContainersError(LegacyError):
+    def __init__(self, names, one_off_names):
         self.names = names
+        self.one_off_names = one_off_names
 
         self.msg = ERROR_MESSAGE_FORMAT.format(
             names_list="\n".join("    {}".format(name) for name in names),
             rm_args=" ".join(names),
         )
 
-    def __unicode__(self):
-        return self.msg
+        if one_off_names:
+            self.msg += ONE_OFF_ADDENDUM_FORMAT.format(rm_args=" ".join(one_off_names))
 
-    __str__ = __unicode__
+
+class LegacyOneOffContainersError(LegacyError):
+    def __init__(self, one_off_names):
+        self.one_off_names = one_off_names
+
+        self.msg = ONE_OFF_ERROR_MESSAGE_FORMAT.format(
+            names_list="\n".join("    {}".format(name) for name in one_off_names),
+            rm_args=" ".join(one_off_names),
+        )
 
 
 def add_labels(project, container):
@@ -76,8 +118,8 @@ def migrate_project_to_labels(project):
         project.client,
         project.name,
         project.service_names,
-        stopped=True,
-        one_off=False)
+        one_off=False,
+    )
 
     for container in containers:
         add_labels(project, container)
@@ -87,13 +129,29 @@ def get_legacy_containers(
         client,
         project,
         services,
-        stopped=False,
         one_off=False):
 
-    containers = client.containers(all=stopped)
+    return list(_get_legacy_containers_iter(
+        client,
+        project,
+        services,
+        one_off=one_off,
+    ))
+
+
+def _get_legacy_containers_iter(
+        client,
+        project,
+        services,
+        one_off=False):
+
+    containers = client.containers(all=True)
 
     for service in services:
         for container in containers:
+            if LABEL_VERSION in container['Labels']:
+                continue
+
             name = get_container_name(container)
             if has_container(project, service, name, one_off=one_off):
                 yield Container.from_ps(client, container)

compose/project.py

@@ -308,8 +308,7 @@ class Project(object):
                 self.client,
                 self.name,
                 self.service_names,
-                stopped=stopped,
-                one_off=one_off)
+            )
 
         return filter(matches_service_names, containers)
 

compose/service.py

@@ -106,8 +106,7 @@ class Service(object):
                 self.client,
                 self.project,
                 [self.name],
-                stopped=stopped,
-                one_off=one_off)
+            )
 
         return containers
 

tests/integration/legacy_test.py

@@ -1,3 +1,5 @@
+import unittest
+
 from docker.errors import APIError
 
 from compose import legacy
@@ -5,6 +7,64 @@ from compose.project import Project
 from .testcases import DockerClientTestCase
 
 
+class UtilitiesTestCase(unittest.TestCase):
+    def test_has_container(self):
+        self.assertTrue(
+            legacy.has_container("composetest", "web", "composetest_web_1", one_off=False),
+        )
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "composetest_web_run_1", one_off=False),
+        )
+
+    def test_has_container_one_off(self):
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "composetest_web_1", one_off=True),
+        )
+        self.assertTrue(
+            legacy.has_container("composetest", "web", "composetest_web_run_1", one_off=True),
+        )
+
+    def test_has_container_different_project(self):
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "otherapp_web_1", one_off=False),
+        )
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "otherapp_web_run_1", one_off=True),
+        )
+
+    def test_has_container_different_service(self):
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "composetest_db_1", one_off=False),
+        )
+        self.assertFalse(
+            legacy.has_container("composetest", "web", "composetest_db_run_1", one_off=True),
+        )
+
+    def test_is_valid_name(self):
+        self.assertTrue(
+            legacy.is_valid_name("composetest_web_1", one_off=False),
+        )
+        self.assertFalse(
+            legacy.is_valid_name("composetest_web_run_1", one_off=False),
+        )
+
+    def test_is_valid_name_one_off(self):
+        self.assertFalse(
+            legacy.is_valid_name("composetest_web_1", one_off=True),
+        )
+        self.assertTrue(
+            legacy.is_valid_name("composetest_web_run_1", one_off=True),
+        )
+
+    def test_is_valid_name_invalid(self):
+        self.assertFalse(
+            legacy.is_valid_name("foo"),
+        )
+        self.assertFalse(
+            legacy.is_valid_name("composetest_web_lol_1", one_off=True),
+        )
+
+
 class LegacyTestCase(DockerClientTestCase):
 
     def setUp(self):
@@ -30,7 +90,7 @@ class LegacyTestCase(DockerClientTestCase):
 
         # Create a single one-off legacy container
         self.containers.append(self.client.create_container(
-            name='{}_{}_run_1'.format(self.project.name, self.services[0].name),
+            name='{}_{}_run_1'.format(self.project.name, db.name),
             **self.services[0].options
         ))
 
@@ -47,27 +107,84 @@ class LegacyTestCase(DockerClientTestCase):
                 pass
 
     def get_legacy_containers(self, **kwargs):
-        return list(legacy.get_legacy_containers(
+        return legacy.get_legacy_containers(
             self.client,
             self.project.name,
             [s.name for s in self.services],
             **kwargs
-        ))
+        )
 
     def test_get_legacy_container_names(self):
         self.assertEqual(len(self.get_legacy_containers()), len(self.services))
 
     def test_get_legacy_container_names_one_off(self):
-        self.assertEqual(len(self.get_legacy_containers(stopped=True, one_off=True)), 1)
+        self.assertEqual(len(self.get_legacy_containers(one_off=True)), 1)
 
     def test_migration_to_labels(self):
+        # Trying to get the container list raises an exception
+
         with self.assertRaises(legacy.LegacyContainersError) as cm:
-            self.assertEqual(self.project.containers(stopped=True), [])
+            self.project.containers(stopped=True)
 
         self.assertEqual(
             set(cm.exception.names),
             set(['composetest_db_1', 'composetest_web_1', 'composetest_nginx_1']),
         )
 
+        self.assertEqual(
+            set(cm.exception.one_off_names),
+            set(['composetest_db_run_1']),
+        )
+
+        # Migrate the containers
+
         legacy.migrate_project_to_labels(self.project)
-        self.assertEqual(len(self.project.containers(stopped=True)), len(self.services))
+
+        # Getting the list no longer raises an exception
+
+        containers = self.project.containers(stopped=True)
+        self.assertEqual(len(containers), len(self.services))
+
+    def test_migration_one_off(self):
+        # We've already migrated
+
+        legacy.migrate_project_to_labels(self.project)
+
+        # Trying to create a one-off container results in a Docker API error
+
+        with self.assertRaises(APIError) as cm:
+            self.project.get_service('db').create_container(one_off=True)
+
+        # Checking for legacy one-off containers raises an exception
+
+        with self.assertRaises(legacy.LegacyOneOffContainersError) as cm:
+            legacy.check_for_legacy_containers(
+                self.client,
+                self.project.name,
+                ['db'],
+                allow_one_off=False,
+            )
+
+        self.assertEqual(
+            set(cm.exception.one_off_names),
+            set(['composetest_db_run_1']),
+        )
+
+        # Remove the old one-off container
+
+        c = self.client.inspect_container('composetest_db_run_1')
+        self.client.remove_container(c)
+
+        # Checking no longer raises an exception
+
+        legacy.check_for_legacy_containers(
+            self.client,
+            self.project.name,
+            ['db'],
+            allow_one_off=False,
+        )
+
+        # Creating a one-off container no longer results in an API error
+
+        self.project.get_service('db').create_container(one_off=True)
+        self.assertIsInstance(self.client.inspect_container('composetest_db_run_1'), dict)

tests/unit/cli_test.py

@@ -97,7 +97,7 @@ class CLITestCase(unittest.TestCase):
     def test_run_with_environment_merged_with_options_list(self, mock_dockerpty):
         command = TopLevelCommand()
         mock_client = mock.create_autospec(docker.Client)
-        mock_project = mock.Mock()
+        mock_project = mock.Mock(client=mock_client)
         mock_project.get_service.return_value = Service(
             'service',
             client=mock_client,
@@ -126,7 +126,7 @@ class CLITestCase(unittest.TestCase):
     def test_run_service_with_restart_always(self):
         command = TopLevelCommand()
         mock_client = mock.create_autospec(docker.Client)
-        mock_project = mock.Mock()
+        mock_project = mock.Mock(client=mock_client)
         mock_project.get_service.return_value = Service(
             'service',
             client=mock_client,
@@ -150,7 +150,7 @@ class CLITestCase(unittest.TestCase):
 
         command = TopLevelCommand()
         mock_client = mock.create_autospec(docker.Client)
-        mock_project = mock.Mock()
+        mock_project = mock.Mock(client=mock_client)
         mock_project.get_service.return_value = Service(
             'service',
             client=mock_client,