diff --git a/compose/cli/main.py b/compose/cli/main.py index ae91eeb5fc..adb11df43d 100644 --- a/compose/cli/main.py +++ b/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 @@ def run(self, project, options): 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) diff --git a/compose/legacy.py b/compose/legacy.py index 340511a767..c9ec658178 100644 --- a/compose/legacy.py +++ b/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 @@ $ 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) diff --git a/compose/project.py b/compose/project.py index 288afc5f5e..11c1e1ce9d 100644 --- a/compose/project.py +++ b/compose/project.py @@ -308,8 +308,7 @@ def matches_service_names(container): self.client, self.name, self.service_names, - stopped=stopped, - one_off=one_off) + ) return filter(matches_service_names, containers) diff --git a/compose/service.py b/compose/service.py index 6fec794fac..f852b9bb73 100644 --- a/compose/service.py +++ b/compose/service.py @@ -106,8 +106,7 @@ def containers(self, stopped=False, one_off=False): self.client, self.project, [self.name], - stopped=stopped, - one_off=one_off) + ) return containers diff --git a/tests/integration/legacy_test.py b/tests/integration/legacy_test.py index 346c84f2e7..806b9a457d 100644 --- a/tests/integration/legacy_test.py +++ b/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 .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 @@ def setUp(self): # 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 @@ def tearDown(self): 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) diff --git a/tests/unit/cli_test.py b/tests/unit/cli_test.py index 1a0a37aac4..7f06f5e3ee 100644 --- a/tests/unit/cli_test.py +++ b/tests/unit/cli_test.py @@ -97,7 +97,7 @@ def test_setup_logging(self): 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 @@ def test_run_with_environment_merged_with_options_list(self, mock_dockerpty): 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 @@ 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,