From 4bc4d273ace15229853e1da41b71c104df33dce9 Mon Sep 17 00:00:00 2001 From: Aanand Prasad Date: Wed, 8 Jul 2015 14:48:03 +0100 Subject: [PATCH] Merge pull request #1643 from aanand/warn-about-legacy-one-off-containers Show an error on 'run' when there are legacy one-off containers (cherry picked from commit 81707ef1ad94403789166d2fe042c8a718a4c748) Signed-off-by: Aanand Prasad --- compose/cli/main.py | 24 ++++-- compose/legacy.py | 94 +++++++++++++++++----- compose/project.py | 3 +- compose/service.py | 3 +- tests/integration/legacy_test.py | 129 +++++++++++++++++++++++++++++-- tests/unit/cli_test.py | 6 +- 6 files changed, 221 insertions(+), 38 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 6c7ccf0da..d5d15177c 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -33,7 +33,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: @@ -334,12 +334,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) diff --git a/compose/legacy.py b/compose/legacy.py index 340511a76..c9ec65817 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 @@ 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 LegacyContainersError(Exception): - def __init__(self, names): +class LegacyError(Exception): + def __unicode__(self): + return self.msg + + __str__ = __unicode__ + + +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 288afc5f5..11c1e1ce9 100644 --- a/compose/project.py +++ b/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) diff --git a/compose/service.py b/compose/service.py index ddabbfc4a..2bf22eac3 100644 --- a/compose/service.py +++ b/compose/service.py @@ -110,8 +110,7 @@ class Service(object): 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 346c84f2e..806b9a457 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 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) diff --git a/tests/unit/cli_test.py b/tests/unit/cli_test.py index d10cb9b30..ab3ea56a9 100644 --- a/tests/unit/cli_test.py +++ b/tests/unit/cli_test.py @@ -127,7 +127,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, @@ -156,7 +156,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, @@ -180,7 +180,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,