From 718ae13ae1702ca5766a79b395c939023b7c15db Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 28 Oct 2015 13:16:19 -0400 Subject: [PATCH 01/40] Move config hash tests to service_test.py Signed-off-by: Daniel Nephin --- tests/integration/service_test.py | 37 +++++++++++++++++++++++++++++++ tests/integration/state_test.py | 36 ------------------------------ 2 files changed, 37 insertions(+), 36 deletions(-) diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 4ac04545e..df9190e70 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -14,6 +14,7 @@ from .. import mock from .testcases import DockerClientTestCase from .testcases import pull_busybox from compose import __version__ +from compose.const import LABEL_CONFIG_HASH from compose.const import LABEL_CONTAINER_NUMBER from compose.const import LABEL_ONE_OFF from compose.const import LABEL_PROJECT @@ -23,6 +24,7 @@ from compose.container import Container from compose.service import build_extra_hosts from compose.service import ConfigError from compose.service import ConvergencePlan +from compose.service import ConvergenceStrategy from compose.service import Net from compose.service import Service from compose.service import VolumeFromSpec @@ -929,3 +931,38 @@ class ServiceTest(DockerClientTestCase): self.assertEqual(set(service.containers(stopped=True)), set([original, duplicate])) self.assertEqual(set(service.duplicate_containers()), set([duplicate])) + + +def converge(service, + strategy=ConvergenceStrategy.changed, + do_build=True): + """Create a converge plan from a strategy and execute the plan.""" + plan = service.convergence_plan(strategy) + return service.execute_convergence_plan(plan, do_build=do_build, timeout=1) + + +class ConfigHashTest(DockerClientTestCase): + def test_no_config_hash_when_one_off(self): + web = self.create_service('web') + container = web.create_container(one_off=True) + self.assertNotIn(LABEL_CONFIG_HASH, container.labels) + + def test_no_config_hash_when_overriding_options(self): + web = self.create_service('web') + container = web.create_container(environment={'FOO': '1'}) + self.assertNotIn(LABEL_CONFIG_HASH, container.labels) + + def test_config_hash_with_custom_labels(self): + web = self.create_service('web', labels={'foo': '1'}) + container = converge(web)[0] + self.assertIn(LABEL_CONFIG_HASH, container.labels) + self.assertIn('foo', container.labels) + + def test_config_hash_sticks_around(self): + web = self.create_service('web', command=["top"]) + container = converge(web)[0] + self.assertIn(LABEL_CONFIG_HASH, container.labels) + + web = self.create_service('web', command=["top", "-d", "1"]) + container = converge(web)[0] + self.assertIn(LABEL_CONFIG_HASH, container.labels) diff --git a/tests/integration/state_test.py b/tests/integration/state_test.py index 3230aefc6..cb9045726 100644 --- a/tests/integration/state_test.py +++ b/tests/integration/state_test.py @@ -10,7 +10,6 @@ import tempfile from .testcases import DockerClientTestCase from compose.config import config -from compose.const import LABEL_CONFIG_HASH from compose.project import Project from compose.service import ConvergenceStrategy @@ -180,14 +179,6 @@ class ProjectWithDependenciesTest(ProjectTestCase): self.assertEqual(len(containers), 2) -def converge(service, - strategy=ConvergenceStrategy.changed, - do_build=True): - """Create a converge plan from a strategy and execute the plan.""" - plan = service.convergence_plan(strategy) - return service.execute_convergence_plan(plan, do_build=do_build, timeout=1) - - class ServiceStateTest(DockerClientTestCase): """Test cases for Service.convergence_plan.""" @@ -278,30 +269,3 @@ class ServiceStateTest(DockerClientTestCase): self.assertEqual(('recreate', [container]), web.convergence_plan()) finally: shutil.rmtree(context) - - -class ConfigHashTest(DockerClientTestCase): - def test_no_config_hash_when_one_off(self): - web = self.create_service('web') - container = web.create_container(one_off=True) - self.assertNotIn(LABEL_CONFIG_HASH, container.labels) - - def test_no_config_hash_when_overriding_options(self): - web = self.create_service('web') - container = web.create_container(environment={'FOO': '1'}) - self.assertNotIn(LABEL_CONFIG_HASH, container.labels) - - def test_config_hash_with_custom_labels(self): - web = self.create_service('web', labels={'foo': '1'}) - container = converge(web)[0] - self.assertIn(LABEL_CONFIG_HASH, container.labels) - self.assertIn('foo', container.labels) - - def test_config_hash_sticks_around(self): - web = self.create_service('web', command=["top"]) - container = converge(web)[0] - self.assertIn(LABEL_CONFIG_HASH, container.labels) - - web = self.create_service('web', command=["top", "-d", "1"]) - container = converge(web)[0] - self.assertIn(LABEL_CONFIG_HASH, container.labels) From 23d4eda2a5de23a72842132f606e545811c93a85 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 3 Nov 2015 20:00:54 -0500 Subject: [PATCH 02/40] Fix service recreate when image changes to build. Signed-off-by: Daniel Nephin --- compose/service.py | 13 ++++--- tests/integration/state_test.py | 65 ++++++++++++++++++-------------- tests/unit/config/config_test.py | 2 + 3 files changed, 46 insertions(+), 34 deletions(-) diff --git a/compose/service.py b/compose/service.py index 66c90b0e0..17746c3b5 100644 --- a/compose/service.py +++ b/compose/service.py @@ -418,6 +418,7 @@ class Service(object): return [ self.recreate_container( container, + do_build=do_build, timeout=timeout, attach_logs=should_attach_logs ) @@ -439,10 +440,12 @@ class Service(object): else: raise Exception("Invalid action: {}".format(action)) - def recreate_container(self, - container, - timeout=DEFAULT_TIMEOUT, - attach_logs=False): + def recreate_container( + self, + container, + do_build=False, + timeout=DEFAULT_TIMEOUT, + attach_logs=False): """Recreate a container. The original container is renamed to a temporary name so that data @@ -454,7 +457,7 @@ class Service(object): container.stop(timeout=timeout) container.rename_to_tmp_name() new_container = self.create_container( - do_build=False, + do_build=do_build, previous_container=container, number=container.labels.get(LABEL_CONTAINER_NUMBER), quiet=True, diff --git a/tests/integration/state_test.py b/tests/integration/state_test.py index cb9045726..7830ba32c 100644 --- a/tests/integration/state_test.py +++ b/tests/integration/state_test.py @@ -4,9 +4,7 @@ by `docker-compose up`. """ from __future__ import unicode_literals -import os -import shutil -import tempfile +import py from .testcases import DockerClientTestCase from compose.config import config @@ -232,40 +230,49 @@ class ServiceStateTest(DockerClientTestCase): image_id = self.client.images(name='busybox')[0]['Id'] self.client.tag(image_id, repository=repo, tag=tag) + self.addCleanup(self.client.remove_image, image) - try: - web = self.create_service('web', image=image) - container = web.create_container() + web = self.create_service('web', image=image) + container = web.create_container() - # update the image - c = self.client.create_container(image, ['touch', '/hello.txt']) - self.client.commit(c, repository=repo, tag=tag) - self.client.remove_container(c) + # update the image + c = self.client.create_container(image, ['touch', '/hello.txt']) + self.client.commit(c, repository=repo, tag=tag) + self.client.remove_container(c) - web = self.create_service('web', image=image) - self.assertEqual(('recreate', [container]), web.convergence_plan()) - - finally: - self.client.remove_image(image) + web = self.create_service('web', image=image) + self.assertEqual(('recreate', [container]), web.convergence_plan()) def test_trigger_recreate_with_build(self): - context = tempfile.mkdtemp() + context = py.test.ensuretemp('test_trigger_recreate_with_build') + self.addCleanup(context.remove) + base_image = "FROM busybox\nLABEL com.docker.compose.test_image=true\n" + dockerfile = context.join('Dockerfile') + dockerfile.write(base_image) - try: - dockerfile = os.path.join(context, 'Dockerfile') + web = self.create_service('web', build=str(context)) + container = web.create_container() - with open(dockerfile, 'w') as f: - f.write(base_image) + dockerfile.write(base_image + 'CMD echo hello world\n') + web.build() - web = self.create_service('web', build=context) - container = web.create_container() + web = self.create_service('web', build=str(context)) + self.assertEqual(('recreate', [container]), web.convergence_plan()) - with open(dockerfile, 'w') as f: - f.write(base_image + 'CMD echo hello world\n') - web.build() + def test_image_changed_to_build(self): + context = py.test.ensuretemp('test_image_changed_to_build') + self.addCleanup(context.remove) + context.join('Dockerfile').write(""" + FROM busybox + LABEL com.docker.compose.test_image=true + """) - web = self.create_service('web', build=context) - self.assertEqual(('recreate', [container]), web.convergence_plan()) - finally: - shutil.rmtree(context) + web = self.create_service('web', image='busybox') + container = web.create_container() + + web = self.create_service('web', build=str(context)) + plan = web.convergence_plan() + self.assertEqual(('recreate', [container]), plan) + containers = web.execute_convergence_plan(plan) + self.assertEqual(len(containers), 1) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 2835e9c80..fc5e22bf2 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -177,6 +177,7 @@ class ConfigTest(unittest.TestCase): details = config.ConfigDetails('.', [base_file, override_file]) tmpdir = py.test.ensuretemp('config_test') + self.addCleanup(tmpdir.remove) tmpdir.join('common.yml').write(""" base: labels: ['label=one'] @@ -412,6 +413,7 @@ class ConfigTest(unittest.TestCase): def test_load_yaml_with_yaml_error(self): tmpdir = py.test.ensuretemp('invalid_yaml_test') + self.addCleanup(tmpdir.remove) invalid_yaml_file = tmpdir.join('docker-compose.yml') invalid_yaml_file.write(""" web: From cf933623682f5483ea41fbbb7cab5bca2402b996 Mon Sep 17 00:00:00 2001 From: Aanand Prasad Date: Mon, 9 Nov 2015 17:24:21 +0000 Subject: [PATCH 03/40] Fix parallel output We were outputting an extra line, which in *some* cases, on *some* terminals, was causing the output of parallel actions to get messed up. In particular, it would happen when the terminal had just been cleared or hadn't yet filled up with a screen's worth of text. Signed-off-by: Aanand Prasad --- compose/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compose/utils.py b/compose/utils.py index c8fddc5f1..14cca61b0 100644 --- a/compose/utils.py +++ b/compose/utils.py @@ -164,7 +164,7 @@ def write_out_msg(stream, lines, msg_index, msg, status="done"): stream.write("%c[%dA" % (27, diff)) # erase stream.write("%c[2K\r" % 27) - stream.write("{} {} ... {}\n".format(msg, obj_index, status)) + stream.write("{} {} ... {}\r".format(msg, obj_index, status)) # move back down stream.write("%c[%dB" % (27, diff)) else: From 3daecfa8e434966b00214aade861aafdb0236c3b Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Mon, 9 Nov 2015 13:07:26 -0800 Subject: [PATCH 04/40] Update service config_dict computation to include volumes_from mode Ensure config_hash is updated when volumes_from mode is changed, and service is recreated on next up as a result. Signed-off-by: Joffrey F --- compose/service.py | 4 +++- tests/unit/service_test.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/compose/service.py b/compose/service.py index 17746c3b5..d841c0cc6 100644 --- a/compose/service.py +++ b/compose/service.py @@ -511,7 +511,9 @@ class Service(object): 'image_id': self.image()['Id'], 'links': self.get_link_names(), 'net': self.net.id, - 'volumes_from': self.get_volumes_from_names(), + 'volumes_from': [ + (v.source.name, v.mode) for v in self.volumes_from if isinstance(v.source, Service) + ], } def get_dependency_names(self): diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index d86f80f73..c77c6a364 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -417,7 +417,7 @@ class ServiceTest(unittest.TestCase): 'options': {'image': 'example.com/foo'}, 'links': [('one', 'one')], 'net': 'other', - 'volumes_from': ['two'], + 'volumes_from': [('two', 'rw')], } self.assertEqual(config_dict, expected) From e317d2db9dbe4a6fea5ceda4d5c0bc67a88b9d3e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 2 Nov 2015 13:33:13 -0500 Subject: [PATCH 05/40] Remove service.start_container() It has been an unnecessary wrapper around container.start() for a little while now, so we can call it directly. Signed-off-by: Daniel Nephin --- compose/cli/main.py | 2 +- compose/service.py | 13 ++++--------- tests/integration/cli_test.py | 2 +- tests/integration/resilience_test.py | 4 ++-- tests/integration/service_test.py | 27 ++++++++++++++------------- 5 files changed, 22 insertions(+), 26 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index b54b307ef..11aeac38c 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -448,7 +448,7 @@ class TopLevelCommand(DocoptCommand): raise e if detach: - service.start_container(container) + container.start() print(container.name) else: dockerpty.start(project.client, container.id, interactive=not options['-T']) diff --git a/compose/service.py b/compose/service.py index d841c0cc6..19aa78380 100644 --- a/compose/service.py +++ b/compose/service.py @@ -410,7 +410,7 @@ class Service(object): if should_attach_logs: container.attach_log_stream() - self.start_container(container) + container.start() return [container] @@ -464,21 +464,16 @@ class Service(object): ) if attach_logs: new_container.attach_log_stream() - self.start_container(new_container) + new_container.start() container.remove() return new_container def start_container_if_stopped(self, container, attach_logs=False): - if container.is_running: - return container - else: + if not container.is_running: log.info("Starting %s" % container.name) if attach_logs: container.attach_log_stream() - return self.start_container(container) - - def start_container(self, container): - container.start() + container.start() return container def remove_duplicate_containers(self, timeout=DEFAULT_TIMEOUT): diff --git a/tests/integration/cli_test.py b/tests/integration/cli_test.py index d621f2d13..e8f0df8cc 100644 --- a/tests/integration/cli_test.py +++ b/tests/integration/cli_test.py @@ -599,7 +599,7 @@ class CLITestCase(DockerClientTestCase): def test_restart(self): service = self.project.get_service('simple') container = service.create_container() - service.start_container(container) + container.start() started_at = container.dictionary['State']['StartedAt'] self.command.dispatch(['restart', '-t', '1'], None) container.inspect() diff --git a/tests/integration/resilience_test.py b/tests/integration/resilience_test.py index befd72c7f..53aedfecf 100644 --- a/tests/integration/resilience_test.py +++ b/tests/integration/resilience_test.py @@ -13,7 +13,7 @@ class ResilienceTest(DockerClientTestCase): self.project = Project('composetest', [self.db], self.client) container = self.db.create_container() - self.db.start_container(container) + container.start() self.host_path = container.get('Volumes')['/var/db'] def test_successful_recreate(self): @@ -31,7 +31,7 @@ class ResilienceTest(DockerClientTestCase): self.assertEqual(container.get('Volumes')['/var/db'], self.host_path) def test_start_failure(self): - with mock.patch('compose.service.Service.start_container', crash): + with mock.patch('compose.container.Container.start', crash): with self.assertRaises(Crash): self.project.up(strategy=ConvergenceStrategy.always) diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index df9190e70..804f5219a 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -32,7 +32,8 @@ from compose.service import VolumeFromSpec def create_and_start_container(service, **override_options): container = service.create_container(**override_options) - return service.start_container(container) + container.start() + return container class ServiceTest(DockerClientTestCase): @@ -117,19 +118,19 @@ class ServiceTest(DockerClientTestCase): def test_create_container_with_unspecified_volume(self): service = self.create_service('db', volumes=['/var/db']) container = service.create_container() - service.start_container(container) + container.start() self.assertIn('/var/db', container.get('Volumes')) def test_create_container_with_volume_driver(self): service = self.create_service('db', volume_driver='foodriver') container = service.create_container() - service.start_container(container) + container.start() self.assertEqual('foodriver', container.get('Config.VolumeDriver')) def test_create_container_with_cpu_shares(self): service = self.create_service('db', cpu_shares=73) container = service.create_container() - service.start_container(container) + container.start() self.assertEqual(container.get('HostConfig.CpuShares'), 73) def test_build_extra_hosts(self): @@ -167,7 +168,7 @@ class ServiceTest(DockerClientTestCase): extra_hosts = ['somehost:162.242.195.82', 'otherhost:50.31.209.229'] service = self.create_service('db', extra_hosts=extra_hosts) container = service.create_container() - service.start_container(container) + container.start() self.assertEqual(set(container.get('HostConfig.ExtraHosts')), set(extra_hosts)) def test_create_container_with_extra_hosts_dicts(self): @@ -175,33 +176,33 @@ class ServiceTest(DockerClientTestCase): extra_hosts_list = ['somehost:162.242.195.82', 'otherhost:50.31.209.229'] service = self.create_service('db', extra_hosts=extra_hosts) container = service.create_container() - service.start_container(container) + container.start() self.assertEqual(set(container.get('HostConfig.ExtraHosts')), set(extra_hosts_list)) def test_create_container_with_cpu_set(self): service = self.create_service('db', cpuset='0') container = service.create_container() - service.start_container(container) + container.start() self.assertEqual(container.get('HostConfig.CpusetCpus'), '0') def test_create_container_with_read_only_root_fs(self): read_only = True service = self.create_service('db', read_only=read_only) container = service.create_container() - service.start_container(container) + container.start() self.assertEqual(container.get('HostConfig.ReadonlyRootfs'), read_only, container.get('HostConfig')) def test_create_container_with_security_opt(self): security_opt = ['label:disable'] service = self.create_service('db', security_opt=security_opt) container = service.create_container() - service.start_container(container) + container.start() self.assertEqual(set(container.get('HostConfig.SecurityOpt')), set(security_opt)) def test_create_container_with_mac_address(self): service = self.create_service('db', mac_address='02:42:ac:11:65:43') container = service.create_container() - service.start_container(container) + container.start() self.assertEqual(container.inspect()['Config']['MacAddress'], '02:42:ac:11:65:43') def test_create_container_with_specified_volume(self): @@ -210,7 +211,7 @@ class ServiceTest(DockerClientTestCase): service = self.create_service('db', volumes=['%s:%s' % (host_path, container_path)]) container = service.create_container() - service.start_container(container) + container.start() volumes = container.inspect()['Volumes'] self.assertIn(container_path, volumes) @@ -283,7 +284,7 @@ class ServiceTest(DockerClientTestCase): ] ) host_container = host_service.create_container() - host_service.start_container(host_container) + host_container.start() self.assertIn(volume_container_1.id + ':rw', host_container.get('HostConfig.VolumesFrom')) self.assertIn(volume_container_2.id + ':rw', @@ -302,7 +303,7 @@ class ServiceTest(DockerClientTestCase): self.assertEqual(old_container.get('Config.Cmd'), ['-d', '1']) self.assertIn('FOO=1', old_container.get('Config.Env')) self.assertEqual(old_container.name, 'composetest_db_1') - service.start_container(old_container) + old_container.start() old_container.inspect() # reload volume data volume_path = old_container.get('Volumes')['/etc'] From 3c4bb5358e5ebee180c62a7d4d7cfa329a607f59 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 5 Nov 2015 10:26:54 -0500 Subject: [PATCH 06/40] Upgrade pyyaml to 3.11 Signed-off-by: Daniel Nephin --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index daaaa9502..60327d728 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,4 @@ -PyYAML==3.10 +PyYAML==3.11 docker-py==1.5.0 dockerpty==0.3.4 docopt==0.6.1 From 0375dccf640c1f94482da4f3f8f7acfc6a924700 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 27 Oct 2015 17:37:48 +0000 Subject: [PATCH 07/40] Handle non-ascii chars in volume directories Signed-off-by: Mazz Mosley --- compose/config/config.py | 2 +- tests/unit/config/config_test.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/compose/config/config.py b/compose/config/config.py index 21549e9b3..f664ec8d5 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -501,7 +501,7 @@ def resolve_volume_path(volume, working_dir, service_name): if host_path.startswith('.'): host_path = expand_path(working_dir, host_path) host_path = os.path.expanduser(host_path) - return "{}:{}".format(host_path, container_path) + return u"{}:{}".format(host_path, container_path) else: return container_path diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index fc5e22bf2..69b235852 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -575,6 +575,11 @@ class VolumeConfigTest(unittest.TestCase): }, working_dir='.') self.assertEqual(d['volumes'], ['~:/data']) + def test_volume_path_with_non_ascii_directory(self): + volume = u'/Füü/data:/data' + container_path = config.resolve_volume_path(volume, ".", "test") + self.assertEqual(container_path, volume) + class MergePathMappingTest(object): def config_name(self): From a5959d9be2167b5a9d671ca5611955eb34c20549 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 19 Oct 2015 12:52:38 -0400 Subject: [PATCH 08/40] Some minor style cleanup - fixed a docstring to make it PEP257 compliant - wrapped some long lines - used a more specific error Signed-off-by: Daniel Nephin --- compose/config/config.py | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index f664ec8d5..b9d71e9d7 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -213,9 +213,16 @@ def load(config_details): class ServiceLoader(object): - def __init__(self, working_dir, filename, service_name, service_dict, already_seen=None): + def __init__( + self, + working_dir, + filename, + service_name, + service_dict, + already_seen=None + ): if working_dir is None: - raise Exception("No working_dir passed to ServiceLoader()") + raise ValueError("No working_dir passed to ServiceLoader()") self.working_dir = os.path.abspath(working_dir) @@ -312,33 +319,33 @@ class ServiceLoader(object): return merge_service_dicts(other_service_dict, self.service_dict) def get_extended_config_path(self, extends_options): - """ - Service we are extending either has a value for 'file' set, which we + """Service we are extending either has a value for 'file' set, which we need to obtain a full path too or we are extending from a service defined in our own file. """ if 'file' in extends_options: - extends_from_filename = extends_options['file'] - return expand_path(self.working_dir, extends_from_filename) - + return expand_path(self.working_dir, extends_options['file']) return self.filename def signature(self, name): - return (self.filename, name) + return self.filename, name def validate_extended_service_dict(service_dict, filename, service): error_prefix = "Cannot extend service '%s' in %s:" % (service, filename) if 'links' in service_dict: - raise ConfigurationError("%s services with 'links' cannot be extended" % error_prefix) + raise ConfigurationError( + "%s services with 'links' cannot be extended" % error_prefix) if 'volumes_from' in service_dict: - raise ConfigurationError("%s services with 'volumes_from' cannot be extended" % error_prefix) + raise ConfigurationError( + "%s services with 'volumes_from' cannot be extended" % error_prefix) if 'net' in service_dict: if get_service_name_from_net(service_dict['net']) is not None: - raise ConfigurationError("%s services with 'net: container' cannot be extended" % error_prefix) + raise ConfigurationError( + "%s services with 'net: container' cannot be extended" % error_prefix) def process_container_options(service_dict, working_dir=None): From 805ed344c010df3a0aa82acfd7c15beec7fbdbc3 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 19 Oct 2015 13:40:13 -0400 Subject: [PATCH 09/40] Refactor ServiceLoader to be immutable. Mutable objects are harder to debug and harder to reason about. ServiceLoader was almost immutable. There was just a single function which set fields for a second function. Instead of mutating the object, we can pass those values as parameters to the next function. Signed-off-by: Daniel Nephin --- compose/config/config.py | 89 +++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 46 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index b9d71e9d7..5bc534fe9 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -240,80 +240,61 @@ class ServiceLoader(object): raise CircularReference(self.already_seen + [self.signature(name)]) def make_service_dict(self): - self.resolve_environment() - if 'extends' in self.service_dict: - self.validate_and_construct_extends() - self.service_dict = self.resolve_extends() + service_dict = dict(self.service_dict) + env = resolve_environment(self.working_dir, self.service_dict) + if env: + service_dict['environment'] = env + service_dict.pop('env_file', None) + + if 'extends' in service_dict: + service_dict = self.resolve_extends(*self.validate_and_construct_extends()) if not self.already_seen: - validate_against_service_schema(self.service_dict, self.service_name) + validate_against_service_schema(service_dict, self.service_name) - return process_container_options(self.service_dict, working_dir=self.working_dir) - - def resolve_environment(self): - """ - Unpack any environment variables from an env_file, if set. - Interpolate environment values if set. - """ - if 'environment' not in self.service_dict and 'env_file' not in self.service_dict: - return - - env = {} - - if 'env_file' in self.service_dict: - for f in get_env_files(self.service_dict, working_dir=self.working_dir): - env.update(env_vars_from_file(f)) - del self.service_dict['env_file'] - - env.update(parse_environment(self.service_dict.get('environment'))) - env = dict(resolve_env_var(k, v) for k, v in six.iteritems(env)) - - self.service_dict['environment'] = env + return process_container_options(service_dict, working_dir=self.working_dir) def validate_and_construct_extends(self): extends = self.service_dict['extends'] if not isinstance(extends, dict): extends = {'service': extends} - validate_extends_file_path( - self.service_name, - extends, - self.filename - ) - self.extended_config_path = self.get_extended_config_path(extends) - self.extended_service_name = extends['service'] + validate_extends_file_path(self.service_name, extends, self.filename) + config_path = self.get_extended_config_path(extends) + service_name = extends['service'] - config = load_yaml(self.extended_config_path) + config = load_yaml(config_path) validate_top_level_object(config) full_extended_config = interpolate_environment_variables(config) validate_extended_service_exists( - self.extended_service_name, + service_name, full_extended_config, - self.extended_config_path + config_path ) validate_against_fields_schema(full_extended_config) - self.extended_config = full_extended_config[self.extended_service_name] + service_config = full_extended_config[service_name] + return config_path, service_config, service_name - def resolve_extends(self): - other_working_dir = os.path.dirname(self.extended_config_path) + def resolve_extends(self, extended_config_path, service_config, service_name): + other_working_dir = os.path.dirname(extended_config_path) other_already_seen = self.already_seen + [self.signature(self.service_name)] other_loader = ServiceLoader( - working_dir=other_working_dir, - filename=self.extended_config_path, - service_name=self.service_name, - service_dict=self.extended_config, + other_working_dir, + extended_config_path, + self.service_name, + service_config, already_seen=other_already_seen, ) - other_loader.detect_cycle(self.extended_service_name) + other_loader.detect_cycle(service_name) other_service_dict = other_loader.make_service_dict() validate_extended_service_dict( other_service_dict, - filename=self.extended_config_path, - service=self.extended_service_name, + extended_config_path, + service_name, ) return merge_service_dicts(other_service_dict, self.service_dict) @@ -331,6 +312,22 @@ class ServiceLoader(object): return self.filename, name +def resolve_environment(working_dir, service_dict): + """Unpack any environment variables from an env_file, if set. + Interpolate environment values if set. + """ + if 'environment' not in service_dict and 'env_file' not in service_dict: + return {} + + env = {} + if 'env_file' in service_dict: + for env_file in get_env_files(service_dict, working_dir=working_dir): + env.update(env_vars_from_file(env_file)) + + env.update(parse_environment(service_dict.get('environment'))) + return dict(resolve_env_var(k, v) for k, v in six.iteritems(env)) + + def validate_extended_service_dict(service_dict, filename, service): error_prefix = "Cannot extend service '%s' in %s:" % (service, filename) From c4f59e731d540780a767d105bcb8d7d164ba4cd5 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 4 Nov 2015 16:37:44 -0500 Subject: [PATCH 10/40] Make working_dir consistent in the config package. - make it a positional arg, since it's required - make it the first argument for all functions that require it - remove an unnecessary one-line function that was only called in one place Signed-off-by: Daniel Nephin --- compose/config/config.py | 33 ++++++++++++-------------------- tests/unit/config/config_test.py | 2 +- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 5bc534fe9..141fa89df 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -252,7 +252,7 @@ class ServiceLoader(object): if not self.already_seen: validate_against_service_schema(service_dict, self.service_name) - return process_container_options(service_dict, working_dir=self.working_dir) + return process_container_options(self.working_dir, service_dict) def validate_and_construct_extends(self): extends = self.service_dict['extends'] @@ -321,7 +321,7 @@ def resolve_environment(working_dir, service_dict): env = {} if 'env_file' in service_dict: - for env_file in get_env_files(service_dict, working_dir=working_dir): + for env_file in get_env_files(working_dir, service_dict): env.update(env_vars_from_file(env_file)) env.update(parse_environment(service_dict.get('environment'))) @@ -345,14 +345,14 @@ def validate_extended_service_dict(service_dict, filename, service): "%s services with 'net: container' cannot be extended" % error_prefix) -def process_container_options(service_dict, working_dir=None): - service_dict = service_dict.copy() +def process_container_options(working_dir, service_dict): + service_dict = dict(service_dict) if 'volumes' in service_dict and service_dict.get('volume_driver') is None: - service_dict['volumes'] = resolve_volume_paths(service_dict, working_dir=working_dir) + service_dict['volumes'] = resolve_volume_paths(working_dir, service_dict) if 'build' in service_dict: - service_dict['build'] = resolve_build_path(service_dict['build'], working_dir=working_dir) + service_dict['build'] = expand_path(working_dir, service_dict['build']) if 'labels' in service_dict: service_dict['labels'] = parse_labels(service_dict['labels']) @@ -428,7 +428,7 @@ def merge_environment(base, override): return env -def get_env_files(options, working_dir=None): +def get_env_files(working_dir, options): if 'env_file' not in options: return {} @@ -488,17 +488,14 @@ def env_vars_from_file(filename): return env -def resolve_volume_paths(service_dict, working_dir=None): - if working_dir is None: - raise Exception("No working_dir passed to resolve_volume_paths()") - +def resolve_volume_paths(working_dir, service_dict): return [ - resolve_volume_path(v, working_dir, service_dict['name']) - for v in service_dict['volumes'] + resolve_volume_path(working_dir, volume, service_dict['name']) + for volume in service_dict['volumes'] ] -def resolve_volume_path(volume, working_dir, service_name): +def resolve_volume_path(working_dir, volume, service_name): container_path, host_path = split_path_mapping(volume) if host_path is not None: @@ -510,12 +507,6 @@ def resolve_volume_path(volume, working_dir, service_name): return container_path -def resolve_build_path(build_path, working_dir=None): - if working_dir is None: - raise Exception("No working_dir passed to resolve_build_path") - return expand_path(working_dir, build_path) - - def validate_paths(service_dict): if 'build' in service_dict: build_path = service_dict['build'] @@ -582,7 +573,7 @@ def parse_labels(labels): return dict(split_label(e) for e in labels) if isinstance(labels, dict): - return labels + return dict(labels) def split_label(label): diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 69b235852..e0d2e870b 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -577,7 +577,7 @@ class VolumeConfigTest(unittest.TestCase): def test_volume_path_with_non_ascii_directory(self): volume = u'/Füü/data:/data' - container_path = config.resolve_volume_path(volume, ".", "test") + container_path = config.resolve_volume_path(".", volume, "test") self.assertEqual(container_path, volume) From e6755d1e7c548ebc5f011fd623c3bba53a3bf4b4 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 4 Nov 2015 16:46:19 -0500 Subject: [PATCH 11/40] Use VolumeSpec instead of re-parsing the volume string. Signed-off-by: Daniel Nephin --- compose/service.py | 19 +++++++++++-------- tests/unit/service_test.py | 4 ++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/compose/service.py b/compose/service.py index 19aa78380..a98cf49fe 100644 --- a/compose/service.py +++ b/compose/service.py @@ -899,14 +899,15 @@ def merge_volume_bindings(volumes_option, previous_container): """Return a list of volume bindings for a container. Container data volumes are replaced by those from the previous container. """ + volumes = [parse_volume_spec(volume) for volume in volumes_option or []] volume_bindings = dict( - build_volume_binding(parse_volume_spec(volume)) - for volume in volumes_option or [] - if ':' in volume) + build_volume_binding(volume) + for volume in volumes + if volume.external) if previous_container: volume_bindings.update( - get_container_data_volumes(previous_container, volumes_option)) + get_container_data_volumes(previous_container, volumes)) return list(volume_bindings.values()) @@ -917,12 +918,14 @@ def get_container_data_volumes(container, volumes_option): """ volumes = [] - volumes_option = volumes_option or [] container_volumes = container.get('Volumes') or {} - image_volumes = container.image_config['ContainerConfig'].get('Volumes') or {} + image_volumes = [ + parse_volume_spec(volume) + for volume in + container.image_config['ContainerConfig'].get('Volumes') or {} + ] - for volume in set(volumes_option + list(image_volumes)): - volume = parse_volume_spec(volume) + for volume in set(volumes_option + image_volumes): # No need to preserve host volumes if volume.external: continue diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index c77c6a364..1a28ddf1f 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -545,11 +545,11 @@ class ServiceVolumesTest(unittest.TestCase): self.assertEqual(binding, ('/inside', '/outside:/inside:rw')) def test_get_container_data_volumes(self): - options = [ + options = [parse_volume_spec(v) for v in [ '/host/volume:/host/volume:ro', '/new/volume', '/existing/volume', - ] + ]] self.mock_client.inspect_image.return_value = { 'ContainerConfig': { From 3f14df374fa5e47558de2d376f69194033817407 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 4 Nov 2015 15:54:59 -0500 Subject: [PATCH 12/40] Handle non-utf8 unicode without raising an error. Signed-off-by: Daniel Nephin --- compose/config/config.py | 2 +- compose/utils.py | 2 +- tests/unit/utils_test.py | 16 ++++++++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 141fa89df..434589d31 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -457,7 +457,7 @@ def parse_environment(environment): def split_env(env): if isinstance(env, six.binary_type): - env = env.decode('utf-8') + env = env.decode('utf-8', 'replace') if '=' in env: return env.split('=', 1) else: diff --git a/compose/utils.py b/compose/utils.py index 14cca61b0..2c6c4584d 100644 --- a/compose/utils.py +++ b/compose/utils.py @@ -95,7 +95,7 @@ def stream_as_text(stream): """ for data in stream: if not isinstance(data, six.text_type): - data = data.decode('utf-8') + data = data.decode('utf-8', 'replace') yield data diff --git a/tests/unit/utils_test.py b/tests/unit/utils_test.py index b272c7349..e3d0bc00b 100644 --- a/tests/unit/utils_test.py +++ b/tests/unit/utils_test.py @@ -1,3 +1,6 @@ +# encoding: utf-8 +from __future__ import unicode_literals + from .. import unittest from compose import utils @@ -14,3 +17,16 @@ class JsonSplitterTestCase(unittest.TestCase): utils.json_splitter(data), ({'foo': 'bar'}, '{"next": "obj"}') ) + + +class StreamAsTextTestCase(unittest.TestCase): + + def test_stream_with_non_utf_unicode_character(self): + stream = [b'\xed\xf3\xf3'] + output, = utils.stream_as_text(stream) + assert output == '���' + + def test_stream_with_utf_character(self): + stream = ['ěĝ'.encode('utf-8')] + output, = utils.stream_as_text(stream) + assert output == 'ěĝ' From ba61a6c5fbad38a6fc157f7c19003feea0be32e4 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 5 Nov 2015 17:50:32 -0500 Subject: [PATCH 13/40] Don't set the hostname to the service name with networking. Signed-off-by: Daniel Nephin --- compose/service.py | 3 --- tests/integration/cli_test.py | 1 - tests/unit/service_test.py | 10 ---------- 3 files changed, 14 deletions(-) diff --git a/compose/service.py b/compose/service.py index a98cf49fe..fc2677791 100644 --- a/compose/service.py +++ b/compose/service.py @@ -605,9 +605,6 @@ class Service(object): container_options['hostname'] = parts[0] container_options['domainname'] = parts[2] - if 'hostname' not in container_options and self.use_networking: - container_options['hostname'] = self.name - if 'ports' in container_options or 'expose' in self.options: ports = [] all_ports = container_options.get('ports', []) + self.options.get('expose', []) diff --git a/tests/integration/cli_test.py b/tests/integration/cli_test.py index e8f0df8cc..279c97f5f 100644 --- a/tests/integration/cli_test.py +++ b/tests/integration/cli_test.py @@ -226,7 +226,6 @@ class CLITestCase(DockerClientTestCase): containers = service.containers() self.assertEqual(len(containers), 1) self.assertIn(containers[0].id, network['Containers']) - self.assertEqual(containers[0].get('Config.Hostname'), service.name) web_container = self.project.get_service('web').containers()[0] self.assertFalse(web_container.get('HostConfig.Links')) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 1a28ddf1f..90bad87ac 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -213,16 +213,6 @@ class ServiceTest(unittest.TestCase): opts = service._get_container_create_options({'image': 'foo'}, 1) self.assertIsNone(opts.get('hostname')) - def test_hostname_defaults_to_service_name_when_using_networking(self): - service = Service( - 'foo', - image='foo', - use_networking=True, - client=self.mock_client, - ) - opts = service._get_container_create_options({'image': 'foo'}, 1) - self.assertEqual(opts['hostname'], 'foo') - def test_get_container_create_options_with_name_option(self): service = Service( 'foo', From 886134c1f3ca27f6bb849551a5e5a31e3efc6e07 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 6 Nov 2015 12:29:52 -0500 Subject: [PATCH 14/40] Recreate dependents when a dependency is created (not just when it's recreated). Signed-off-by: Daniel Nephin --- compose/project.py | 2 +- tests/integration/state_test.py | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/compose/project.py b/compose/project.py index 1e01eaf6d..f0478203b 100644 --- a/compose/project.py +++ b/compose/project.py @@ -322,7 +322,7 @@ class Project(object): name for name in service.get_dependency_names() if name in plans - and plans[name].action == 'recreate' + and plans[name].action in ('recreate', 'create') ] if updated_dependencies and strategy.allows_recreate: diff --git a/tests/integration/state_test.py b/tests/integration/state_test.py index 7830ba32c..1fecce87b 100644 --- a/tests/integration/state_test.py +++ b/tests/integration/state_test.py @@ -176,6 +176,19 @@ class ProjectWithDependenciesTest(ProjectTestCase): containers = self.run_up(next_cfg) self.assertEqual(len(containers), 2) + def test_service_recreated_when_dependency_created(self): + containers = self.run_up(self.cfg, service_names=['web'], start_deps=False) + self.assertEqual(len(containers), 1) + + containers = self.run_up(self.cfg) + self.assertEqual(len(containers), 3) + + web, = [c for c in containers if c.service == 'web'] + nginx, = [c for c in containers if c.service == 'nginx'] + + self.assertEqual(web.links(), ['composetest_db_1', 'db', 'db_1']) + self.assertEqual(nginx.links(), ['composetest_web_1', 'web', 'web_1']) + class ServiceStateTest(DockerClientTestCase): """Test cases for Service.convergence_plan.""" From 666c3cb1c76e6d69075337e523effce0d93b9e4d Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Tue, 10 Nov 2015 10:11:20 -0800 Subject: [PATCH 15/40] Use exit code 1 when encountering a ReadTimeout Signed-off-by: Joffrey F --- compose/cli/main.py | 1 + 1 file changed, 1 insertion(+) diff --git a/compose/cli/main.py b/compose/cli/main.py index 11aeac38c..95db45cec 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -80,6 +80,7 @@ def main(): "If you encounter this issue regularly because of slow network conditions, consider setting " "COMPOSE_HTTP_TIMEOUT to a higher value (current value: %s)." % HTTP_TIMEOUT ) + sys.exit(1) def setup_logging(): From 7f2f4eef48c2cec4f8cf8337c4a9076820cc9e3e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 2 Nov 2015 16:46:49 -0500 Subject: [PATCH 16/40] Update cli tests to use subprocess. Signed-off-by: Daniel Nephin --- tests/integration/cli_test.py | 447 ++++++++++++++++------------------ 1 file changed, 209 insertions(+), 238 deletions(-) diff --git a/tests/integration/cli_test.py b/tests/integration/cli_test.py index 279c97f5f..7b7537e21 100644 --- a/tests/integration/cli_test.py +++ b/tests/integration/cli_test.py @@ -2,30 +2,32 @@ from __future__ import absolute_import import os import shlex -import sys +import subprocess +from collections import namedtuple from operator import attrgetter -from six import StringIO +import pytest from .. import mock from .testcases import DockerClientTestCase from compose.cli.command import get_project from compose.cli.docker_client import docker_client -from compose.cli.errors import UserError -from compose.cli.main import TopLevelCommand -from compose.project import NoSuchService + + +ProcessResult = namedtuple('ProcessResult', 'stdout stderr') + + +BUILD_CACHE_TEXT = 'Using cache' +BUILD_PULL_TEXT = 'Status: Image is up to date for busybox:latest' class CLITestCase(DockerClientTestCase): + def setUp(self): super(CLITestCase, self).setUp() - self.old_sys_exit = sys.exit - sys.exit = lambda code=0: None - self.command = TopLevelCommand() - self.command.base_dir = 'tests/fixtures/simple-composefile' + self.base_dir = 'tests/fixtures/simple-composefile' def tearDown(self): - sys.exit = self.old_sys_exit self.project.kill() self.project.remove_stopped() for container in self.project.containers(stopped=True, one_off=True): @@ -34,129 +36,121 @@ class CLITestCase(DockerClientTestCase): @property def project(self): - # Hack: allow project to be overridden. This needs refactoring so that - # the project object is built exactly once, by the command object, and - # accessed by the test case object. - if hasattr(self, '_project'): - return self._project + # Hack: allow project to be overridden + if not hasattr(self, '_project'): + self._project = get_project(self.base_dir) + return self._project - return get_project(self.command.base_dir) + def dispatch(self, options, project_options=None, returncode=0): + project_options = project_options or [] + proc = subprocess.Popen( + ['docker-compose'] + project_options + options, + # Note: this might actually be a patched sys.stdout, so we have + # to specify it here, even though it's the default + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + cwd=self.base_dir) + print("Running process: %s" % proc.pid) + stdout, stderr = proc.communicate() + if proc.returncode != returncode: + print(stderr) + assert proc.returncode == returncode + return ProcessResult(stdout.decode('utf-8'), stderr.decode('utf-8')) def test_help(self): - old_base_dir = self.command.base_dir - self.command.base_dir = 'tests/fixtures/no-composefile' - with self.assertRaises(SystemExit) as exc_context: - self.command.dispatch(['help', 'up'], None) - self.assertIn('Usage: up [options] [SERVICE...]', str(exc_context.exception)) + old_base_dir = self.base_dir + self.base_dir = 'tests/fixtures/no-composefile' + result = self.dispatch(['help', 'up'], returncode=1) + assert 'Usage: up [options] [SERVICE...]' in result.stderr # self.project.kill() fails during teardown # unless there is a composefile. - self.command.base_dir = old_base_dir + self.base_dir = old_base_dir - # TODO: address the "Inappropriate ioctl for device" warnings in test output def test_ps(self): self.project.get_service('simple').create_container() - with mock.patch('sys.stdout', new_callable=StringIO) as mock_stdout: - self.command.dispatch(['ps'], None) - self.assertIn('simplecomposefile_simple_1', mock_stdout.getvalue()) + result = self.dispatch(['ps']) + assert 'simplecomposefile_simple_1' in result.stdout def test_ps_default_composefile(self): - self.command.base_dir = 'tests/fixtures/multiple-composefiles' - with mock.patch('sys.stdout', new_callable=StringIO) as mock_stdout: - self.command.dispatch(['up', '-d'], None) - self.command.dispatch(['ps'], None) + self.base_dir = 'tests/fixtures/multiple-composefiles' + self.dispatch(['up', '-d']) + result = self.dispatch(['ps']) - output = mock_stdout.getvalue() - self.assertIn('multiplecomposefiles_simple_1', output) - self.assertIn('multiplecomposefiles_another_1', output) - self.assertNotIn('multiplecomposefiles_yetanother_1', output) + self.assertIn('multiplecomposefiles_simple_1', result.stdout) + self.assertIn('multiplecomposefiles_another_1', result.stdout) + self.assertNotIn('multiplecomposefiles_yetanother_1', result.stdout) def test_ps_alternate_composefile(self): config_path = os.path.abspath( 'tests/fixtures/multiple-composefiles/compose2.yml') - self._project = get_project(self.command.base_dir, [config_path]) + self._project = get_project(self.base_dir, [config_path]) - self.command.base_dir = 'tests/fixtures/multiple-composefiles' - with mock.patch('sys.stdout', new_callable=StringIO) as mock_stdout: - self.command.dispatch(['-f', 'compose2.yml', 'up', '-d'], None) - self.command.dispatch(['-f', 'compose2.yml', 'ps'], None) + self.base_dir = 'tests/fixtures/multiple-composefiles' + self.dispatch(['-f', 'compose2.yml', 'up', '-d']) + result = self.dispatch(['-f', 'compose2.yml', 'ps']) - output = mock_stdout.getvalue() - self.assertNotIn('multiplecomposefiles_simple_1', output) - self.assertNotIn('multiplecomposefiles_another_1', output) - self.assertIn('multiplecomposefiles_yetanother_1', output) + self.assertNotIn('multiplecomposefiles_simple_1', result.stdout) + self.assertNotIn('multiplecomposefiles_another_1', result.stdout) + self.assertIn('multiplecomposefiles_yetanother_1', result.stdout) - @mock.patch('compose.service.log') - def test_pull(self, mock_logging): - self.command.dispatch(['pull'], None) - mock_logging.info.assert_any_call('Pulling simple (busybox:latest)...') - mock_logging.info.assert_any_call('Pulling another (busybox:latest)...') + def test_pull(self): + result = self.dispatch(['pull']) + assert sorted(result.stderr.split('\n'))[1:] == [ + 'Pulling another (busybox:latest)...', + 'Pulling simple (busybox:latest)...', + ] - @mock.patch('compose.service.log') - def test_pull_with_digest(self, mock_logging): - self.command.dispatch(['-f', 'digest.yml', 'pull'], None) - mock_logging.info.assert_any_call('Pulling simple (busybox:latest)...') - mock_logging.info.assert_any_call( - 'Pulling digest (busybox@' - 'sha256:38a203e1986cf79639cfb9b2e1d6e773de84002feea2d4eb006b52004ee8502d)...') + def test_pull_with_digest(self): + result = self.dispatch(['-f', 'digest.yml', 'pull']) - @mock.patch('compose.service.log') - def test_pull_with_ignore_pull_failures(self, mock_logging): - self.command.dispatch(['-f', 'ignore-pull-failures.yml', 'pull', '--ignore-pull-failures'], None) - mock_logging.info.assert_any_call('Pulling simple (busybox:latest)...') - mock_logging.info.assert_any_call('Pulling another (nonexisting-image:latest)...') - mock_logging.error.assert_any_call('Error: image library/nonexisting-image:latest not found') + assert 'Pulling simple (busybox:latest)...' in result.stderr + assert ('Pulling digest (busybox@' + 'sha256:38a203e1986cf79639cfb9b2e1d6e773de84002feea2d4eb006b520' + '04ee8502d)...') in result.stderr + + def test_pull_with_ignore_pull_failures(self): + result = self.dispatch([ + '-f', 'ignore-pull-failures.yml', + 'pull', '--ignore-pull-failures']) + + assert 'Pulling simple (busybox:latest)...' in result.stderr + assert 'Pulling another (nonexisting-image:latest)...' in result.stderr + assert 'Error: image library/nonexisting-image:latest not found' in result.stderr def test_build_plain(self): - self.command.base_dir = 'tests/fixtures/simple-dockerfile' - self.command.dispatch(['build', 'simple'], None) + self.base_dir = 'tests/fixtures/simple-dockerfile' + self.dispatch(['build', 'simple']) - cache_indicator = 'Using cache' - pull_indicator = 'Status: Image is up to date for busybox:latest' - - with mock.patch('sys.stdout', new_callable=StringIO) as mock_stdout: - self.command.dispatch(['build', 'simple'], None) - output = mock_stdout.getvalue() - self.assertIn(cache_indicator, output) - self.assertNotIn(pull_indicator, output) + result = self.dispatch(['build', 'simple']) + assert BUILD_CACHE_TEXT in result.stdout + assert BUILD_PULL_TEXT not in result.stdout def test_build_no_cache(self): - self.command.base_dir = 'tests/fixtures/simple-dockerfile' - self.command.dispatch(['build', 'simple'], None) + self.base_dir = 'tests/fixtures/simple-dockerfile' + self.dispatch(['build', 'simple']) - cache_indicator = 'Using cache' - pull_indicator = 'Status: Image is up to date for busybox:latest' - with mock.patch('sys.stdout', new_callable=StringIO) as mock_stdout: - self.command.dispatch(['build', '--no-cache', 'simple'], None) - output = mock_stdout.getvalue() - self.assertNotIn(cache_indicator, output) - self.assertNotIn(pull_indicator, output) + result = self.dispatch(['build', '--no-cache', 'simple']) + assert BUILD_CACHE_TEXT not in result.stdout + assert BUILD_PULL_TEXT not in result.stdout def test_build_pull(self): - self.command.base_dir = 'tests/fixtures/simple-dockerfile' - self.command.dispatch(['build', 'simple'], None) + self.base_dir = 'tests/fixtures/simple-dockerfile' + self.dispatch(['build', 'simple'], None) - cache_indicator = 'Using cache' - pull_indicator = 'Status: Image is up to date for busybox:latest' - with mock.patch('sys.stdout', new_callable=StringIO) as mock_stdout: - self.command.dispatch(['build', '--pull', 'simple'], None) - output = mock_stdout.getvalue() - self.assertIn(cache_indicator, output) - self.assertIn(pull_indicator, output) + result = self.dispatch(['build', '--pull', 'simple']) + assert BUILD_CACHE_TEXT in result.stdout + assert BUILD_PULL_TEXT in result.stdout def test_build_no_cache_pull(self): - self.command.base_dir = 'tests/fixtures/simple-dockerfile' - self.command.dispatch(['build', 'simple'], None) + self.base_dir = 'tests/fixtures/simple-dockerfile' + self.dispatch(['build', 'simple']) - cache_indicator = 'Using cache' - pull_indicator = 'Status: Image is up to date for busybox:latest' - with mock.patch('sys.stdout', new_callable=StringIO) as mock_stdout: - self.command.dispatch(['build', '--no-cache', '--pull', 'simple'], None) - output = mock_stdout.getvalue() - self.assertNotIn(cache_indicator, output) - self.assertIn(pull_indicator, output) + result = self.dispatch(['build', '--no-cache', '--pull', 'simple']) + assert BUILD_CACHE_TEXT not in result.stdout + assert BUILD_PULL_TEXT in result.stdout def test_up_detached(self): - self.command.dispatch(['up', '-d'], None) + self.dispatch(['up', '-d'], None) service = self.project.get_service('simple') another = self.project.get_service('another') self.assertEqual(len(service.containers()), 1) @@ -168,12 +162,14 @@ class CLITestCase(DockerClientTestCase): self.assertFalse(container.get('Config.AttachStdout')) self.assertFalse(container.get('Config.AttachStdin')) + # TODO: needs rework + @pytest.mark.skipif(True, reason="runs top") def test_up_attached(self): with mock.patch( 'compose.cli.main.attach_to_logs', autospec=True ) as mock_attach: - self.command.dispatch(['up'], None) + self.dispatch(['up'], None) _, args, kwargs = mock_attach.mock_calls[0] _project, log_printer, _names, _timeout = args @@ -189,8 +185,8 @@ class CLITestCase(DockerClientTestCase): def test_up_without_networking(self): self.require_api_version('1.21') - self.command.base_dir = 'tests/fixtures/links-composefile' - self.command.dispatch(['up', '-d'], None) + self.base_dir = 'tests/fixtures/links-composefile' + self.dispatch(['up', '-d'], None) client = docker_client(version='1.21') networks = client.networks(names=[self.project.name]) @@ -207,8 +203,8 @@ class CLITestCase(DockerClientTestCase): def test_up_with_networking(self): self.require_api_version('1.21') - self.command.base_dir = 'tests/fixtures/links-composefile' - self.command.dispatch(['--x-networking', 'up', '-d'], None) + self.base_dir = 'tests/fixtures/links-composefile' + self.dispatch(['--x-networking', 'up', '-d'], None) client = docker_client(version='1.21') services = self.project.get_services() @@ -231,8 +227,8 @@ class CLITestCase(DockerClientTestCase): self.assertFalse(web_container.get('HostConfig.Links')) def test_up_with_links(self): - self.command.base_dir = 'tests/fixtures/links-composefile' - self.command.dispatch(['up', '-d', 'web'], None) + self.base_dir = 'tests/fixtures/links-composefile' + self.dispatch(['up', '-d', 'web'], None) web = self.project.get_service('web') db = self.project.get_service('db') console = self.project.get_service('console') @@ -241,8 +237,8 @@ class CLITestCase(DockerClientTestCase): self.assertEqual(len(console.containers()), 0) def test_up_with_no_deps(self): - self.command.base_dir = 'tests/fixtures/links-composefile' - self.command.dispatch(['up', '-d', '--no-deps', 'web'], None) + self.base_dir = 'tests/fixtures/links-composefile' + self.dispatch(['up', '-d', '--no-deps', 'web'], None) web = self.project.get_service('web') db = self.project.get_service('db') console = self.project.get_service('console') @@ -251,13 +247,13 @@ class CLITestCase(DockerClientTestCase): self.assertEqual(len(console.containers()), 0) def test_up_with_force_recreate(self): - self.command.dispatch(['up', '-d'], None) + self.dispatch(['up', '-d'], None) service = self.project.get_service('simple') self.assertEqual(len(service.containers()), 1) old_ids = [c.id for c in service.containers()] - self.command.dispatch(['up', '-d', '--force-recreate'], None) + self.dispatch(['up', '-d', '--force-recreate'], None) self.assertEqual(len(service.containers()), 1) new_ids = [c.id for c in service.containers()] @@ -265,13 +261,13 @@ class CLITestCase(DockerClientTestCase): self.assertNotEqual(old_ids, new_ids) def test_up_with_no_recreate(self): - self.command.dispatch(['up', '-d'], None) + self.dispatch(['up', '-d'], None) service = self.project.get_service('simple') self.assertEqual(len(service.containers()), 1) old_ids = [c.id for c in service.containers()] - self.command.dispatch(['up', '-d', '--no-recreate'], None) + self.dispatch(['up', '-d', '--no-recreate'], None) self.assertEqual(len(service.containers()), 1) new_ids = [c.id for c in service.containers()] @@ -279,11 +275,12 @@ class CLITestCase(DockerClientTestCase): self.assertEqual(old_ids, new_ids) def test_up_with_force_recreate_and_no_recreate(self): - with self.assertRaises(UserError): - self.command.dispatch(['up', '-d', '--force-recreate', '--no-recreate'], None) + self.dispatch( + ['up', '-d', '--force-recreate', '--no-recreate'], + returncode=1) def test_up_with_timeout(self): - self.command.dispatch(['up', '-d', '-t', '1'], None) + self.dispatch(['up', '-d', '-t', '1'], None) service = self.project.get_service('simple') another = self.project.get_service('another') self.assertEqual(len(service.containers()), 1) @@ -295,10 +292,9 @@ class CLITestCase(DockerClientTestCase): self.assertFalse(config['AttachStdout']) self.assertFalse(config['AttachStdin']) - @mock.patch('dockerpty.start') - def test_run_service_without_links(self, mock_stdout): - self.command.base_dir = 'tests/fixtures/links-composefile' - self.command.dispatch(['run', 'console', '/bin/true'], None) + def test_run_service_without_links(self): + self.base_dir = 'tests/fixtures/links-composefile' + self.dispatch(['run', 'console', '/bin/true']) self.assertEqual(len(self.project.containers()), 0) # Ensure stdin/out was open @@ -308,44 +304,40 @@ class CLITestCase(DockerClientTestCase): self.assertTrue(config['AttachStdout']) self.assertTrue(config['AttachStdin']) - @mock.patch('dockerpty.start') - def test_run_service_with_links(self, _): - self.command.base_dir = 'tests/fixtures/links-composefile' - self.command.dispatch(['run', 'web', '/bin/true'], None) + def test_run_service_with_links(self): + self.base_dir = 'tests/fixtures/links-composefile' + self.dispatch(['run', 'web', '/bin/true'], None) db = self.project.get_service('db') console = self.project.get_service('console') self.assertEqual(len(db.containers()), 1) self.assertEqual(len(console.containers()), 0) - @mock.patch('dockerpty.start') - def test_run_with_no_deps(self, _): - self.command.base_dir = 'tests/fixtures/links-composefile' - self.command.dispatch(['run', '--no-deps', 'web', '/bin/true'], None) + def test_run_with_no_deps(self): + self.base_dir = 'tests/fixtures/links-composefile' + self.dispatch(['run', '--no-deps', 'web', '/bin/true']) db = self.project.get_service('db') self.assertEqual(len(db.containers()), 0) - @mock.patch('dockerpty.start') - def test_run_does_not_recreate_linked_containers(self, _): - self.command.base_dir = 'tests/fixtures/links-composefile' - self.command.dispatch(['up', '-d', 'db'], None) + def test_run_does_not_recreate_linked_containers(self): + self.base_dir = 'tests/fixtures/links-composefile' + self.dispatch(['up', '-d', 'db']) db = self.project.get_service('db') self.assertEqual(len(db.containers()), 1) old_ids = [c.id for c in db.containers()] - self.command.dispatch(['run', 'web', '/bin/true'], None) + self.dispatch(['run', 'web', '/bin/true'], None) self.assertEqual(len(db.containers()), 1) new_ids = [c.id for c in db.containers()] self.assertEqual(old_ids, new_ids) - @mock.patch('dockerpty.start') - def test_run_without_command(self, _): - self.command.base_dir = 'tests/fixtures/commands-composefile' + def test_run_without_command(self): + self.base_dir = 'tests/fixtures/commands-composefile' self.check_build('tests/fixtures/simple-dockerfile', tag='composetest_test') - self.command.dispatch(['run', 'implicit'], None) + self.dispatch(['run', 'implicit']) service = self.project.get_service('implicit') containers = service.containers(stopped=True, one_off=True) self.assertEqual( @@ -353,7 +345,7 @@ class CLITestCase(DockerClientTestCase): [u'/bin/sh -c echo "success"'], ) - self.command.dispatch(['run', 'explicit'], None) + self.dispatch(['run', 'explicit']) service = self.project.get_service('explicit') containers = service.containers(stopped=True, one_off=True) self.assertEqual( @@ -361,14 +353,10 @@ class CLITestCase(DockerClientTestCase): [u'/bin/true'], ) - @mock.patch('dockerpty.start') - def test_run_service_with_entrypoint_overridden(self, _): - self.command.base_dir = 'tests/fixtures/dockerfile_with_entrypoint' + def test_run_service_with_entrypoint_overridden(self): + self.base_dir = 'tests/fixtures/dockerfile_with_entrypoint' name = 'service' - self.command.dispatch( - ['run', '--entrypoint', '/bin/echo', name, 'helloworld'], - None - ) + self.dispatch(['run', '--entrypoint', '/bin/echo', name, 'helloworld']) service = self.project.get_service(name) container = service.containers(stopped=True, one_off=True)[0] self.assertEqual( @@ -376,37 +364,34 @@ class CLITestCase(DockerClientTestCase): [u'/bin/echo', u'helloworld'], ) - @mock.patch('dockerpty.start') - def test_run_service_with_user_overridden(self, _): - self.command.base_dir = 'tests/fixtures/user-composefile' + def test_run_service_with_user_overridden(self): + self.base_dir = 'tests/fixtures/user-composefile' name = 'service' user = 'sshd' - args = ['run', '--user={user}'.format(user=user), name] - self.command.dispatch(args, None) + self.dispatch(['run', '--user={user}'.format(user=user), name], returncode=1) service = self.project.get_service(name) container = service.containers(stopped=True, one_off=True)[0] self.assertEqual(user, container.get('Config.User')) - @mock.patch('dockerpty.start') - def test_run_service_with_user_overridden_short_form(self, _): - self.command.base_dir = 'tests/fixtures/user-composefile' + def test_run_service_with_user_overridden_short_form(self): + self.base_dir = 'tests/fixtures/user-composefile' name = 'service' user = 'sshd' - args = ['run', '-u', user, name] - self.command.dispatch(args, None) + self.dispatch(['run', '-u', user, name], returncode=1) service = self.project.get_service(name) container = service.containers(stopped=True, one_off=True)[0] self.assertEqual(user, container.get('Config.User')) - @mock.patch('dockerpty.start') - def test_run_service_with_environement_overridden(self, _): + def test_run_service_with_environement_overridden(self): name = 'service' - self.command.base_dir = 'tests/fixtures/environment-composefile' - self.command.dispatch( - ['run', '-e', 'foo=notbar', '-e', 'allo=moto=bobo', - '-e', 'alpha=beta', name], - None - ) + self.base_dir = 'tests/fixtures/environment-composefile' + self.dispatch([ + 'run', '-e', 'foo=notbar', + '-e', 'allo=moto=bobo', + '-e', 'alpha=beta', + name, + '/bin/true', + ]) service = self.project.get_service(name) container = service.containers(stopped=True, one_off=True)[0] # env overriden @@ -418,11 +403,10 @@ class CLITestCase(DockerClientTestCase): # make sure a value with a = don't crash out self.assertEqual('moto=bobo', container.environment['allo']) - @mock.patch('dockerpty.start') - def test_run_service_without_map_ports(self, _): + def test_run_service_without_map_ports(self): # create one off container - self.command.base_dir = 'tests/fixtures/ports-composefile' - self.command.dispatch(['run', '-d', 'simple'], None) + self.base_dir = 'tests/fixtures/ports-composefile' + self.dispatch(['run', '-d', 'simple']) container = self.project.get_service('simple').containers(one_off=True)[0] # get port information @@ -436,12 +420,10 @@ class CLITestCase(DockerClientTestCase): self.assertEqual(port_random, None) self.assertEqual(port_assigned, None) - @mock.patch('dockerpty.start') - def test_run_service_with_map_ports(self, _): - + def test_run_service_with_map_ports(self): # create one off container - self.command.base_dir = 'tests/fixtures/ports-composefile' - self.command.dispatch(['run', '-d', '--service-ports', 'simple'], None) + self.base_dir = 'tests/fixtures/ports-composefile' + self.dispatch(['run', '-d', '--service-ports', 'simple']) container = self.project.get_service('simple').containers(one_off=True)[0] # get port information @@ -459,12 +441,10 @@ class CLITestCase(DockerClientTestCase): self.assertEqual(port_range[0], "0.0.0.0:49153") self.assertEqual(port_range[1], "0.0.0.0:49154") - @mock.patch('dockerpty.start') - def test_run_service_with_explicitly_maped_ports(self, _): - + def test_run_service_with_explicitly_maped_ports(self): # create one off container - self.command.base_dir = 'tests/fixtures/ports-composefile' - self.command.dispatch(['run', '-d', '-p', '30000:3000', '--publish', '30001:3001', 'simple'], None) + self.base_dir = 'tests/fixtures/ports-composefile' + self.dispatch(['run', '-d', '-p', '30000:3000', '--publish', '30001:3001', 'simple']) container = self.project.get_service('simple').containers(one_off=True)[0] # get port information @@ -478,12 +458,10 @@ class CLITestCase(DockerClientTestCase): self.assertEqual(port_short, "0.0.0.0:30000") self.assertEqual(port_full, "0.0.0.0:30001") - @mock.patch('dockerpty.start') - def test_run_service_with_explicitly_maped_ip_ports(self, _): - + def test_run_service_with_explicitly_maped_ip_ports(self): # create one off container - self.command.base_dir = 'tests/fixtures/ports-composefile' - self.command.dispatch(['run', '-d', '-p', '127.0.0.1:30000:3000', '--publish', '127.0.0.1:30001:3001', 'simple'], None) + self.base_dir = 'tests/fixtures/ports-composefile' + self.dispatch(['run', '-d', '-p', '127.0.0.1:30000:3000', '--publish', '127.0.0.1:30001:3001', 'simple'], None) container = self.project.get_service('simple').containers(one_off=True)[0] # get port information @@ -497,22 +475,20 @@ class CLITestCase(DockerClientTestCase): self.assertEqual(port_short, "127.0.0.1:30000") self.assertEqual(port_full, "127.0.0.1:30001") - @mock.patch('dockerpty.start') - def test_run_with_custom_name(self, _): - self.command.base_dir = 'tests/fixtures/environment-composefile' + def test_run_with_custom_name(self): + self.base_dir = 'tests/fixtures/environment-composefile' name = 'the-container-name' - self.command.dispatch(['run', '--name', name, 'service'], None) + self.dispatch(['run', '--name', name, 'service', '/bin/true']) service = self.project.get_service('service') container, = service.containers(stopped=True, one_off=True) self.assertEqual(container.name, name) - @mock.patch('dockerpty.start') - def test_run_with_networking(self, _): + def test_run_with_networking(self): self.require_api_version('1.21') client = docker_client(version='1.21') - self.command.base_dir = 'tests/fixtures/simple-dockerfile' - self.command.dispatch(['--x-networking', 'run', 'simple', 'true'], None) + self.base_dir = 'tests/fixtures/simple-dockerfile' + self.dispatch(['--x-networking', 'run', 'simple', 'true'], None) service = self.project.get_service('simple') container, = service.containers(stopped=True, one_off=True) networks = client.networks(names=[self.project.name]) @@ -526,71 +502,70 @@ class CLITestCase(DockerClientTestCase): service.create_container() service.kill() self.assertEqual(len(service.containers(stopped=True)), 1) - self.command.dispatch(['rm', '--force'], None) + self.dispatch(['rm', '--force'], None) self.assertEqual(len(service.containers(stopped=True)), 0) service = self.project.get_service('simple') service.create_container() service.kill() self.assertEqual(len(service.containers(stopped=True)), 1) - self.command.dispatch(['rm', '-f'], None) + self.dispatch(['rm', '-f'], None) self.assertEqual(len(service.containers(stopped=True)), 0) def test_stop(self): - self.command.dispatch(['up', '-d'], None) + self.dispatch(['up', '-d'], None) service = self.project.get_service('simple') self.assertEqual(len(service.containers()), 1) self.assertTrue(service.containers()[0].is_running) - self.command.dispatch(['stop', '-t', '1'], None) + self.dispatch(['stop', '-t', '1'], None) self.assertEqual(len(service.containers(stopped=True)), 1) self.assertFalse(service.containers(stopped=True)[0].is_running) def test_pause_unpause(self): - self.command.dispatch(['up', '-d'], None) + self.dispatch(['up', '-d'], None) service = self.project.get_service('simple') self.assertFalse(service.containers()[0].is_paused) - self.command.dispatch(['pause'], None) + self.dispatch(['pause'], None) self.assertTrue(service.containers()[0].is_paused) - self.command.dispatch(['unpause'], None) + self.dispatch(['unpause'], None) self.assertFalse(service.containers()[0].is_paused) def test_logs_invalid_service_name(self): - with self.assertRaises(NoSuchService): - self.command.dispatch(['logs', 'madeupname'], None) + self.dispatch(['logs', 'madeupname'], returncode=1) def test_kill(self): - self.command.dispatch(['up', '-d'], None) + self.dispatch(['up', '-d'], None) service = self.project.get_service('simple') self.assertEqual(len(service.containers()), 1) self.assertTrue(service.containers()[0].is_running) - self.command.dispatch(['kill'], None) + self.dispatch(['kill'], None) self.assertEqual(len(service.containers(stopped=True)), 1) self.assertFalse(service.containers(stopped=True)[0].is_running) def test_kill_signal_sigstop(self): - self.command.dispatch(['up', '-d'], None) + self.dispatch(['up', '-d'], None) service = self.project.get_service('simple') self.assertEqual(len(service.containers()), 1) self.assertTrue(service.containers()[0].is_running) - self.command.dispatch(['kill', '-s', 'SIGSTOP'], None) + self.dispatch(['kill', '-s', 'SIGSTOP'], None) self.assertEqual(len(service.containers()), 1) # The container is still running. It has only been paused self.assertTrue(service.containers()[0].is_running) def test_kill_stopped_service(self): - self.command.dispatch(['up', '-d'], None) + self.dispatch(['up', '-d'], None) service = self.project.get_service('simple') - self.command.dispatch(['kill', '-s', 'SIGSTOP'], None) + self.dispatch(['kill', '-s', 'SIGSTOP'], None) self.assertTrue(service.containers()[0].is_running) - self.command.dispatch(['kill', '-s', 'SIGKILL'], None) + self.dispatch(['kill', '-s', 'SIGKILL'], None) self.assertEqual(len(service.containers(stopped=True)), 1) self.assertFalse(service.containers(stopped=True)[0].is_running) @@ -600,7 +575,7 @@ class CLITestCase(DockerClientTestCase): container = service.create_container() container.start() started_at = container.dictionary['State']['StartedAt'] - self.command.dispatch(['restart', '-t', '1'], None) + self.dispatch(['restart', '-t', '1'], None) container.inspect() self.assertNotEqual( container.dictionary['State']['FinishedAt'], @@ -614,53 +589,51 @@ class CLITestCase(DockerClientTestCase): def test_scale(self): project = self.project - self.command.scale(project, {'SERVICE=NUM': ['simple=1']}) + self.dispatch(['scale', 'simple=1']) self.assertEqual(len(project.get_service('simple').containers()), 1) - self.command.scale(project, {'SERVICE=NUM': ['simple=3', 'another=2']}) + self.dispatch(['scale', 'simple=3', 'another=2']) self.assertEqual(len(project.get_service('simple').containers()), 3) self.assertEqual(len(project.get_service('another').containers()), 2) - self.command.scale(project, {'SERVICE=NUM': ['simple=1', 'another=1']}) + self.dispatch(['scale', 'simple=1', 'another=1']) self.assertEqual(len(project.get_service('simple').containers()), 1) self.assertEqual(len(project.get_service('another').containers()), 1) - self.command.scale(project, {'SERVICE=NUM': ['simple=1', 'another=1']}) + self.dispatch(['scale', 'simple=1', 'another=1']) self.assertEqual(len(project.get_service('simple').containers()), 1) self.assertEqual(len(project.get_service('another').containers()), 1) - self.command.scale(project, {'SERVICE=NUM': ['simple=0', 'another=0']}) + self.dispatch(['scale', 'simple=0', 'another=0']) self.assertEqual(len(project.get_service('simple').containers()), 0) self.assertEqual(len(project.get_service('another').containers()), 0) def test_port(self): - self.command.base_dir = 'tests/fixtures/ports-composefile' - self.command.dispatch(['up', '-d'], None) + self.base_dir = 'tests/fixtures/ports-composefile' + self.dispatch(['up', '-d'], None) container = self.project.get_service('simple').get_container() - @mock.patch('sys.stdout', new_callable=StringIO) - def get_port(number, mock_stdout): - self.command.dispatch(['port', 'simple', str(number)], None) - return mock_stdout.getvalue().rstrip() + def get_port(number): + result = self.dispatch(['port', 'simple', str(number)]) + return result.stdout.rstrip() self.assertEqual(get_port(3000), container.get_local_port(3000)) self.assertEqual(get_port(3001), "0.0.0.0:49152") self.assertEqual(get_port(3002), "0.0.0.0:49153") def test_port_with_scale(self): - self.command.base_dir = 'tests/fixtures/ports-composefile-scale' - self.command.dispatch(['scale', 'simple=2'], None) + self.base_dir = 'tests/fixtures/ports-composefile-scale' + self.dispatch(['scale', 'simple=2'], None) containers = sorted( self.project.containers(service_names=['simple']), key=attrgetter('name')) - @mock.patch('sys.stdout', new_callable=StringIO) - def get_port(number, mock_stdout, index=None): + def get_port(number, index=None): if index is None: - self.command.dispatch(['port', 'simple', str(number)], None) + result = self.dispatch(['port', 'simple', str(number)]) else: - self.command.dispatch(['port', '--index=' + str(index), 'simple', str(number)], None) - return mock_stdout.getvalue().rstrip() + result = self.dispatch(['port', '--index=' + str(index), 'simple', str(number)]) + return result.stdout.rstrip() self.assertEqual(get_port(3000), containers[0].get_local_port(3000)) self.assertEqual(get_port(3000, index=1), containers[0].get_local_port(3000)) @@ -669,8 +642,8 @@ class CLITestCase(DockerClientTestCase): def test_env_file_relative_to_compose_file(self): config_path = os.path.abspath('tests/fixtures/env-file/docker-compose.yml') - self.command.dispatch(['-f', config_path, 'up', '-d'], None) - self._project = get_project(self.command.base_dir, [config_path]) + self.dispatch(['-f', config_path, 'up', '-d'], None) + self._project = get_project(self.base_dir, [config_path]) containers = self.project.containers(stopped=True) self.assertEqual(len(containers), 1) @@ -680,20 +653,18 @@ class CLITestCase(DockerClientTestCase): def test_home_and_env_var_in_volume_path(self): os.environ['VOLUME_NAME'] = 'my-volume' os.environ['HOME'] = '/tmp/home-dir' - expected_host_path = os.path.join(os.environ['HOME'], os.environ['VOLUME_NAME']) - self.command.base_dir = 'tests/fixtures/volume-path-interpolation' - self.command.dispatch(['up', '-d'], None) + self.base_dir = 'tests/fixtures/volume-path-interpolation' + self.dispatch(['up', '-d'], None) container = self.project.containers(stopped=True)[0] actual_host_path = container.get('Volumes')['/container-path'] components = actual_host_path.split('/') - self.assertTrue(components[-2:] == ['home-dir', 'my-volume'], - msg="Last two components differ: %s, %s" % (actual_host_path, expected_host_path)) + assert components[-2:] == ['home-dir', 'my-volume'] def test_up_with_default_override_file(self): - self.command.base_dir = 'tests/fixtures/override-files' - self.command.dispatch(['up', '-d'], None) + self.base_dir = 'tests/fixtures/override-files' + self.dispatch(['up', '-d'], None) containers = self.project.containers() self.assertEqual(len(containers), 2) @@ -703,15 +674,15 @@ class CLITestCase(DockerClientTestCase): self.assertEqual(db.human_readable_command, 'top') def test_up_with_multiple_files(self): - self.command.base_dir = 'tests/fixtures/override-files' + self.base_dir = 'tests/fixtures/override-files' config_paths = [ 'docker-compose.yml', 'docker-compose.override.yml', 'extra.yml', ] - self._project = get_project(self.command.base_dir, config_paths) - self.command.dispatch( + self._project = get_project(self.base_dir, config_paths) + self.dispatch( [ '-f', config_paths[0], '-f', config_paths[1], @@ -730,8 +701,8 @@ class CLITestCase(DockerClientTestCase): self.assertEqual(other.human_readable_command, 'top') def test_up_with_extends(self): - self.command.base_dir = 'tests/fixtures/extends' - self.command.dispatch(['up', '-d'], None) + self.base_dir = 'tests/fixtures/extends' + self.dispatch(['up', '-d'], None) self.assertEqual( set([s.name for s in self.project.services]), From 4105c3017c9a75b6044298daffe4a56d7409c2d5 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 2 Nov 2015 17:52:37 -0500 Subject: [PATCH 17/40] Move cli tests to a new testing package. These cli tests are now a different kind of that that run the compose binary. They are not the same as integration tests that test some internal interface. Signed-off-by: Daniel Nephin --- tests/acceptance/__init__.py | 0 tests/{integration => acceptance}/cli_test.py | 29 ++++--------------- .../fixtures/echo-services/docker-compose.yml | 6 ++++ 3 files changed, 12 insertions(+), 23 deletions(-) create mode 100644 tests/acceptance/__init__.py rename tests/{integration => acceptance}/cli_test.py (96%) create mode 100644 tests/fixtures/echo-services/docker-compose.yml diff --git a/tests/acceptance/__init__.py b/tests/acceptance/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/integration/cli_test.py b/tests/acceptance/cli_test.py similarity index 96% rename from tests/integration/cli_test.py rename to tests/acceptance/cli_test.py index 7b7537e21..fc68f9d85 100644 --- a/tests/integration/cli_test.py +++ b/tests/acceptance/cli_test.py @@ -6,12 +6,10 @@ import subprocess from collections import namedtuple from operator import attrgetter -import pytest - from .. import mock -from .testcases import DockerClientTestCase from compose.cli.command import get_project from compose.cli.docker_client import docker_client +from tests.integration.testcases import DockerClientTestCase ProcessResult = namedtuple('ProcessResult', 'stdout stderr') @@ -45,8 +43,6 @@ class CLITestCase(DockerClientTestCase): project_options = project_options or [] proc = subprocess.Popen( ['docker-compose'] + project_options + options, - # Note: this might actually be a patched sys.stdout, so we have - # to specify it here, even though it's the default stdout=subprocess.PIPE, stderr=subprocess.PIPE, cwd=self.base_dir) @@ -150,7 +146,7 @@ class CLITestCase(DockerClientTestCase): assert BUILD_PULL_TEXT in result.stdout def test_up_detached(self): - self.dispatch(['up', '-d'], None) + self.dispatch(['up', '-d']) service = self.project.get_service('simple') another = self.project.get_service('another') self.assertEqual(len(service.containers()), 1) @@ -162,25 +158,12 @@ class CLITestCase(DockerClientTestCase): self.assertFalse(container.get('Config.AttachStdout')) self.assertFalse(container.get('Config.AttachStdin')) - # TODO: needs rework - @pytest.mark.skipif(True, reason="runs top") def test_up_attached(self): - with mock.patch( - 'compose.cli.main.attach_to_logs', - autospec=True - ) as mock_attach: - self.dispatch(['up'], None) - _, args, kwargs = mock_attach.mock_calls[0] - _project, log_printer, _names, _timeout = args + self.base_dir = 'tests/fixtures/echo-services' + result = self.dispatch(['up', '--no-color']) - service = self.project.get_service('simple') - another = self.project.get_service('another') - self.assertEqual(len(service.containers()), 1) - self.assertEqual(len(another.containers()), 1) - self.assertEqual( - set(log_printer.containers), - set(self.project.containers()) - ) + assert 'simple_1 | simple' in result.stdout + assert 'another_1 | another' in result.stdout def test_up_without_networking(self): self.require_api_version('1.21') diff --git a/tests/fixtures/echo-services/docker-compose.yml b/tests/fixtures/echo-services/docker-compose.yml new file mode 100644 index 000000000..8014f3d91 --- /dev/null +++ b/tests/fixtures/echo-services/docker-compose.yml @@ -0,0 +1,6 @@ +simple: + image: busybox:latest + command: echo simple +another: + image: busybox:latest + command: echo another From 8fb44db92bd35c0ab9145401a0a560bc0391761f Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 20 Oct 2015 13:10:32 -0400 Subject: [PATCH 18/40] Cleanup some unit tests and whitespace. Remove some unnecessary newlines. Remove a unittest that was attempting to test behaviour that was removed a while ago, so isn't testing anything. Updated some unit tests to use mocks instead of a custom fake. Signed-off-by: Daniel Nephin --- compose/service.py | 8 ++---- tests/unit/service_test.py | 50 ++++++++++++++------------------------ 2 files changed, 20 insertions(+), 38 deletions(-) diff --git a/compose/service.py b/compose/service.py index fc2677791..4a7169e96 100644 --- a/compose/service.py +++ b/compose/service.py @@ -300,9 +300,7 @@ class Service(object): Create a container for this service. If the image doesn't exist, attempt to pull it. """ - self.ensure_image_exists( - do_build=do_build, - ) + self.ensure_image_exists(do_build=do_build) container_options = self._get_container_create_options( override_options, @@ -316,9 +314,7 @@ class Service(object): return Container.create(self.client, **container_options) - def ensure_image_exists(self, - do_build=True): - + def ensure_image_exists(self, do_build=True): try: self.image() return diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 90bad87ac..4c70beb70 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -339,44 +339,37 @@ class ServiceTest(unittest.TestCase): self.assertEqual(parse_repository_tag("user/repo@sha256:digest"), ("user/repo", "sha256:digest", "@")) self.assertEqual(parse_repository_tag("url:5000/repo@sha256:digest"), ("url:5000/repo", "sha256:digest", "@")) - @mock.patch('compose.service.Container', autospec=True) - def test_create_container_latest_is_used_when_no_tag_specified(self, mock_container): - service = Service('foo', client=self.mock_client, image='someimage') - images = [] - - def pull(repo, tag=None, **kwargs): - self.assertEqual('someimage', repo) - self.assertEqual('latest', tag) - images.append({'Id': 'abc123'}) - return [] - - service.image = lambda *args, **kwargs: mock_get_image(images) - self.mock_client.pull = pull - - service.create_container() - self.assertEqual(1, len(images)) - def test_create_container_with_build(self): service = Service('foo', client=self.mock_client, build='.') - - images = [] - service.image = lambda *args, **kwargs: mock_get_image(images) - service.build = lambda: images.append({'Id': 'abc123'}) + self.mock_client.inspect_image.side_effect = [ + NoSuchImageError, + {'Id': 'abc123'}, + ] + self.mock_client.build.return_value = [ + '{"stream": "Successfully built abcd"}', + ] service.create_container(do_build=True) - self.assertEqual(1, len(images)) + self.mock_client.build.assert_called_once_with( + tag='default_foo', + dockerfile=None, + stream=True, + path='.', + pull=False, + nocache=False, + rm=True, + ) def test_create_container_no_build(self): service = Service('foo', client=self.mock_client, build='.') - service.image = lambda: {'Id': 'abc123'} + self.mock_client.inspect_image.return_value = {'Id': 'abc123'} service.create_container(do_build=False) 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='.') - service.image = lambda *args, **kwargs: mock_get_image([]) - + self.mock_client.inspect_image.side_effect = NoSuchImageError with self.assertRaises(NeedsBuildError): service.create_container(do_build=False) @@ -484,13 +477,6 @@ class NetTestCase(unittest.TestCase): self.assertEqual(net.service_name, service_name) -def mock_get_image(images): - if images: - return images[0] - else: - raise NoSuchImageError() - - class ServiceVolumesTest(unittest.TestCase): def setUp(self): From 4c2eb17ccd04e5da00366de2650acf20becf0200 Mon Sep 17 00:00:00 2001 From: Adrian Budau Date: Tue, 27 Oct 2015 02:00:51 -0700 Subject: [PATCH 19/40] Added --force-rm to compose build. It's a flag passed to docker build that removes the intermediate containers left behind on fail builds. Signed-off-by: Adrian Budau --- compose/cli/main.py | 4 ++- compose/project.py | 4 +-- compose/service.py | 3 +- contrib/completion/bash/docker-compose | 2 +- contrib/completion/zsh/_docker-compose | 1 + docs/reference/build.md | 1 + tests/acceptance/cli_test.py | 28 +++++++++++++++++++ .../simple-failing-dockerfile/Dockerfile | 7 +++++ .../docker-compose.yml | 2 ++ tests/unit/service_test.py | 1 + 10 files changed, 48 insertions(+), 5 deletions(-) create mode 100644 tests/fixtures/simple-failing-dockerfile/Dockerfile create mode 100644 tests/fixtures/simple-failing-dockerfile/docker-compose.yml diff --git a/compose/cli/main.py b/compose/cli/main.py index 95db45cec..0360c8bef 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -181,12 +181,14 @@ class TopLevelCommand(DocoptCommand): Usage: build [options] [SERVICE...] Options: + --force-rm Always remove intermediate containers. --no-cache Do not use cache when building the image. --pull Always attempt to pull a newer version of the image. """ + force_rm = bool(options.get('--force-rm', False)) no_cache = bool(options.get('--no-cache', False)) pull = bool(options.get('--pull', False)) - project.build(service_names=options['SERVICE'], no_cache=no_cache, pull=pull) + project.build(service_names=options['SERVICE'], no_cache=no_cache, pull=pull, force_rm=force_rm) def help(self, project, options): """ diff --git a/compose/project.py b/compose/project.py index f0478203b..d2ba86b1b 100644 --- a/compose/project.py +++ b/compose/project.py @@ -278,10 +278,10 @@ class Project(object): for service in self.get_services(service_names): service.restart(**options) - def build(self, service_names=None, no_cache=False, pull=False): + def build(self, service_names=None, no_cache=False, pull=False, force_rm=False): for service in self.get_services(service_names): if service.can_be_built(): - service.build(no_cache, pull) + service.build(no_cache, pull, force_rm) else: log.info('%s uses an image, skipping' % service.name) diff --git a/compose/service.py b/compose/service.py index 4a7169e96..fcf438c09 100644 --- a/compose/service.py +++ b/compose/service.py @@ -701,7 +701,7 @@ class Service(object): cgroup_parent=cgroup_parent ) - def build(self, no_cache=False, pull=False): + def build(self, no_cache=False, pull=False, force_rm=False): log.info('Building %s' % self.name) path = self.options['build'] @@ -715,6 +715,7 @@ class Service(object): tag=self.image_name, stream=True, rm=True, + forcerm=force_rm, pull=pull, nocache=no_cache, dockerfile=self.options.get('dockerfile', None), diff --git a/contrib/completion/bash/docker-compose b/contrib/completion/bash/docker-compose index 0eed1f18b..3deff9a4b 100644 --- a/contrib/completion/bash/docker-compose +++ b/contrib/completion/bash/docker-compose @@ -87,7 +87,7 @@ __docker_compose_services_stopped() { _docker_compose_build() { case "$cur" in -*) - COMPREPLY=( $( compgen -W "--help --no-cache --pull" -- "$cur" ) ) + COMPREPLY=( $( compgen -W "--help --force-rm --no-cache --pull" -- "$cur" ) ) ;; *) __docker_compose_services_from_build diff --git a/contrib/completion/zsh/_docker-compose b/contrib/completion/zsh/_docker-compose index d79b25d16..08d5150d9 100644 --- a/contrib/completion/zsh/_docker-compose +++ b/contrib/completion/zsh/_docker-compose @@ -192,6 +192,7 @@ __docker-compose_subcommand() { (build) _arguments \ $opts_help \ + '--force-rm[Always remove intermediate containers.]' \ '--no-cache[Do not use cache when building the image]' \ '--pull[Always attempt to pull a newer version of the image.]' \ '*:services:__docker-compose_services_from_build' && ret=0 diff --git a/docs/reference/build.md b/docs/reference/build.md index c427199fe..84aefc253 100644 --- a/docs/reference/build.md +++ b/docs/reference/build.md @@ -15,6 +15,7 @@ parent = "smn_compose_cli" Usage: build [options] [SERVICE...] Options: +--force-rm Always remove intermediate containers. --no-cache Do not use cache when building the image. --pull Always attempt to pull a newer version of the image. ``` diff --git a/tests/acceptance/cli_test.py b/tests/acceptance/cli_test.py index fc68f9d85..88a43d7f0 100644 --- a/tests/acceptance/cli_test.py +++ b/tests/acceptance/cli_test.py @@ -9,6 +9,7 @@ from operator import attrgetter from .. import mock from compose.cli.command import get_project from compose.cli.docker_client import docker_client +from compose.container import Container from tests.integration.testcases import DockerClientTestCase @@ -145,6 +146,33 @@ class CLITestCase(DockerClientTestCase): assert BUILD_CACHE_TEXT not in result.stdout assert BUILD_PULL_TEXT in result.stdout + def test_build_failed(self): + self.base_dir = 'tests/fixtures/simple-failing-dockerfile' + self.dispatch(['build', 'simple'], returncode=1) + + labels = ["com.docker.compose.test_failing_image=true"] + containers = [ + Container.from_ps(self.project.client, c) + for c in self.project.client.containers( + all=True, + filters={"label": labels}) + ] + assert len(containers) == 1 + + def test_build_failed_forcerm(self): + self.base_dir = 'tests/fixtures/simple-failing-dockerfile' + self.dispatch(['build', '--force-rm', 'simple'], returncode=1) + + labels = ["com.docker.compose.test_failing_image=true"] + + containers = [ + Container.from_ps(self.project.client, c) + for c in self.project.client.containers( + all=True, + filters={"label": labels}) + ] + assert not containers + def test_up_detached(self): self.dispatch(['up', '-d']) service = self.project.get_service('simple') diff --git a/tests/fixtures/simple-failing-dockerfile/Dockerfile b/tests/fixtures/simple-failing-dockerfile/Dockerfile new file mode 100644 index 000000000..c2d06b167 --- /dev/null +++ b/tests/fixtures/simple-failing-dockerfile/Dockerfile @@ -0,0 +1,7 @@ +FROM busybox:latest +LABEL com.docker.compose.test_image=true +LABEL com.docker.compose.test_failing_image=true +# With the following label the container wil be cleaned up automatically +# Must be kept in sync with LABEL_PROJECT from compose/const.py +LABEL com.docker.compose.project=composetest +RUN exit 1 diff --git a/tests/fixtures/simple-failing-dockerfile/docker-compose.yml b/tests/fixtures/simple-failing-dockerfile/docker-compose.yml new file mode 100644 index 000000000..b0357541e --- /dev/null +++ b/tests/fixtures/simple-failing-dockerfile/docker-compose.yml @@ -0,0 +1,2 @@ +simple: + build: . diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 4c70beb70..af7007d39 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -356,6 +356,7 @@ class ServiceTest(unittest.TestCase): stream=True, path='.', pull=False, + forcerm=False, nocache=False, rm=True, ) From de08da278dcc9051d9b1040240cd9433f2266502 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 10 Nov 2015 13:11:59 -0500 Subject: [PATCH 20/40] Re-order flags in bash completion and remove unnecessary variables from build command. Signed-off-by: Daniel Nephin --- compose/cli/main.py | 9 +++++---- contrib/completion/bash/docker-compose | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 0360c8bef..08c1aee07 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -185,10 +185,11 @@ class TopLevelCommand(DocoptCommand): --no-cache Do not use cache when building the image. --pull Always attempt to pull a newer version of the image. """ - force_rm = bool(options.get('--force-rm', False)) - no_cache = bool(options.get('--no-cache', False)) - pull = bool(options.get('--pull', False)) - project.build(service_names=options['SERVICE'], no_cache=no_cache, pull=pull, force_rm=force_rm) + project.build( + service_names=options['SERVICE'], + no_cache=bool(options.get('--no-cache', False)), + pull=bool(options.get('--pull', False)), + force_rm=bool(options.get('--force-rm', False))) def help(self, project, options): """ diff --git a/contrib/completion/bash/docker-compose b/contrib/completion/bash/docker-compose index 3deff9a4b..72e159e08 100644 --- a/contrib/completion/bash/docker-compose +++ b/contrib/completion/bash/docker-compose @@ -87,7 +87,7 @@ __docker_compose_services_stopped() { _docker_compose_build() { case "$cur" in -*) - COMPREPLY=( $( compgen -W "--help --force-rm --no-cache --pull" -- "$cur" ) ) + COMPREPLY=( $( compgen -W "--force-rm --help --no-cache --pull" -- "$cur" ) ) ;; *) __docker_compose_services_from_build From 36176befb0ef4a16ef24b09234238363756689e5 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 5 Nov 2015 15:33:42 -0500 Subject: [PATCH 21/40] Fix #1549 - flush after each line of logs. Includes some refactoring of log_printer_test to support checking for flush(), and so that each test calls the unit-under-test directly, instead of through a helper function. Signed-off-by: Daniel Nephin --- compose/cli/log_printer.py | 1 + tests/unit/cli/log_printer_test.py | 82 +++++++++++++++--------------- 2 files changed, 42 insertions(+), 41 deletions(-) diff --git a/compose/cli/log_printer.py b/compose/cli/log_printer.py index 66920726c..864657a4c 100644 --- a/compose/cli/log_printer.py +++ b/compose/cli/log_printer.py @@ -26,6 +26,7 @@ class LogPrinter(object): generators = list(self._make_log_generators(self.monochrome, prefix_width)) for line in Multiplexer(generators).loop(): self.output.write(line) + self.output.flush() def _make_log_generators(self, monochrome, prefix_width): def no_color(text): diff --git a/tests/unit/cli/log_printer_test.py b/tests/unit/cli/log_printer_test.py index 575fcaf7b..5b04226cf 100644 --- a/tests/unit/cli/log_printer_test.py +++ b/tests/unit/cli/log_printer_test.py @@ -1,13 +1,13 @@ from __future__ import absolute_import from __future__ import unicode_literals -import mock +import pytest import six from compose.cli.log_printer import LogPrinter from compose.cli.log_printer import wait_on_exit from compose.container import Container -from tests import unittest +from tests import mock def build_mock_container(reader): @@ -22,40 +22,52 @@ def build_mock_container(reader): ) -class LogPrinterTest(unittest.TestCase): - def get_default_output(self, monochrome=False): - def reader(*args, **kwargs): - yield b"hello\nworld" - container = build_mock_container(reader) - output = run_log_printer([container], monochrome=monochrome) - return output +@pytest.fixture +def output_stream(): + output = six.StringIO() + output.flush = mock.Mock() + return output - def test_single_container(self): - output = self.get_default_output() - self.assertIn('hello', output) - self.assertIn('world', output) +@pytest.fixture +def mock_container(): + def reader(*args, **kwargs): + yield b"hello\nworld" + return build_mock_container(reader) - def test_monochrome(self): - output = self.get_default_output(monochrome=True) - self.assertNotIn('\033[', output) - def test_polychrome(self): - output = self.get_default_output() - self.assertIn('\033[', output) +class TestLogPrinter(object): - def test_unicode(self): + def test_single_container(self, output_stream, mock_container): + LogPrinter([mock_container], output=output_stream).run() + + output = output_stream.getvalue() + assert 'hello' in output + assert 'world' in output + # Call count is 2 lines + "container exited line" + assert output_stream.flush.call_count == 3 + + def test_monochrome(self, output_stream, mock_container): + LogPrinter([mock_container], output=output_stream, monochrome=True).run() + assert '\033[' not in output_stream.getvalue() + + def test_polychrome(self, output_stream, mock_container): + LogPrinter([mock_container], output=output_stream).run() + assert '\033[' in output_stream.getvalue() + + def test_unicode(self, output_stream): glyph = u'\u2022' def reader(*args, **kwargs): yield glyph.encode('utf-8') + b'\n' container = build_mock_container(reader) - output = run_log_printer([container]) + LogPrinter([container], output=output_stream).run() + output = output_stream.getvalue() if six.PY2: output = output.decode('utf-8') - self.assertIn(glyph, output) + assert glyph in output def test_wait_on_exit(self): exit_status = 3 @@ -65,24 +77,12 @@ class LogPrinterTest(unittest.TestCase): wait=mock.Mock(return_value=exit_status)) expected = '{} exited with code {}\n'.format(mock_container.name, exit_status) - self.assertEqual(expected, wait_on_exit(mock_container)) + assert expected == wait_on_exit(mock_container) - def test_generator_with_no_logs(self): - mock_container = mock.Mock( - spec=Container, - has_api_logs=False, - log_driver='none', - name_without_project='web_1', - wait=mock.Mock(return_value=0)) + def test_generator_with_no_logs(self, mock_container, output_stream): + mock_container.has_api_logs = False + mock_container.log_driver = 'none' + LogPrinter([mock_container], output=output_stream).run() - output = run_log_printer([mock_container]) - self.assertIn( - "WARNING: no logs are available with the 'none' log driver\n", - output - ) - - -def run_log_printer(containers, monochrome=False): - output = six.StringIO() - LogPrinter(containers, output=output, monochrome=monochrome).run() - return output.getvalue() + output = output_stream.getvalue() + assert "WARNING: no logs are available with the 'none' log driver\n" in output From 0a96f86f7444841017c4fcd7063d6d22a2488a0b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 6 Nov 2015 14:30:27 -0500 Subject: [PATCH 22/40] Cleanup workaround in testcase.py Signed-off-by: Daniel Nephin --- tests/integration/testcases.py | 35 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 23 deletions(-) diff --git a/tests/integration/testcases.py b/tests/integration/testcases.py index 686a2b69a..60e67b5bf 100644 --- a/tests/integration/testcases.py +++ b/tests/integration/testcases.py @@ -42,34 +42,23 @@ class DockerClientTestCase(unittest.TestCase): if 'command' not in kwargs: kwargs['command'] = ["top"] - links = kwargs.get('links', None) - volumes_from = kwargs.get('volumes_from', None) - net = kwargs.get('net', None) + workaround_options = {} + for option in ['links', 'volumes_from', 'net']: + if option in kwargs: + workaround_options[option] = kwargs.pop(option, None) - workaround_options = ['links', 'volumes_from', 'net'] - for key in workaround_options: - try: - del kwargs[key] - except KeyError: - pass - - options = ServiceLoader(working_dir='.', filename=None, service_name=name, service_dict=kwargs).make_service_dict() + options = ServiceLoader( + working_dir='.', + filename=None, + service_name=name, + service_dict=kwargs + ).make_service_dict() + options.update(workaround_options) labels = options.setdefault('labels', {}) labels['com.docker.compose.test-name'] = self.id() - if links: - options['links'] = links - if volumes_from: - options['volumes_from'] = volumes_from - if net: - options['net'] = net - - return Service( - project='composetest', - client=self.client, - **options - ) + return Service(project='composetest', client=self.client, **options) def check_build(self, *args, **kwargs): kwargs.setdefault('rm', True) From 73ebd7e560d6ec9ce3473ed77a91336bbf379af6 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 6 Nov 2015 13:56:25 -0500 Subject: [PATCH 23/40] Only create the default network if at least one service needs it. Signed-off-by: Daniel Nephin --- compose/project.py | 7 +++++-- tests/integration/project_test.py | 16 ++++++++++++++++ tests/unit/project_test.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/compose/project.py b/compose/project.py index d2ba86b1b..41af86261 100644 --- a/compose/project.py +++ b/compose/project.py @@ -300,7 +300,7 @@ class Project(object): plans = self._get_convergence_plans(services, strategy) - if self.use_networking: + if self.use_networking and self.uses_default_network(): self.ensure_network_exists() return [ @@ -383,7 +383,10 @@ class Project(object): def remove_network(self): network = self.get_network() if network: - self.client.remove_network(network['id']) + self.client.remove_network(network['Id']) + + def uses_default_network(self): + return any(service.net.mode == self.name for service in self.services) def _inject_deps(self, acc, service): dep_names = service.get_dependency_names() diff --git a/tests/integration/project_test.py b/tests/integration/project_test.py index 950523878..2ce319005 100644 --- a/tests/integration/project_test.py +++ b/tests/integration/project_test.py @@ -7,6 +7,7 @@ from compose.const import LABEL_PROJECT from compose.container import Container from compose.project import Project from compose.service import ConvergenceStrategy +from compose.service import Net from compose.service import VolumeFromSpec @@ -111,6 +112,7 @@ class ProjectTest(DockerClientTestCase): network_name = 'network_does_exist' project = Project(network_name, [], client) client.create_network(network_name) + self.addCleanup(client.remove_network, network_name) assert project.get_network()['Name'] == network_name def test_net_from_service(self): @@ -398,6 +400,20 @@ class ProjectTest(DockerClientTestCase): self.assertEqual(len(project.get_service('data').containers(stopped=True)), 1) self.assertEqual(len(project.get_service('console').containers()), 0) + def test_project_up_with_custom_network(self): + self.require_api_version('1.21') + client = docker_client(version='1.21') + network_name = 'composetest-custom' + + client.create_network(network_name) + self.addCleanup(client.remove_network, network_name) + + web = self.create_service('web', net=Net(network_name)) + project = Project('composetest', [web], client, use_networking=True) + project.up() + + assert project.get_network() is None + def test_unscale_after_restart(self): web = self.create_service('web') project = Project('composetest', [web], self.client) diff --git a/tests/unit/project_test.py b/tests/unit/project_test.py index fc189fbb1..b38f5c783 100644 --- a/tests/unit/project_test.py +++ b/tests/unit/project_test.py @@ -7,6 +7,8 @@ from .. import unittest from compose.const import LABEL_SERVICE from compose.container import Container from compose.project import Project +from compose.service import ContainerNet +from compose.service import Net from compose.service import Service @@ -263,6 +265,32 @@ class ProjectTest(unittest.TestCase): service = project.get_service('test') self.assertEqual(service.net.mode, 'container:' + container_name) + def test_uses_default_network_true(self): + web = Service('web', project='test', image="alpine", net=Net('test')) + db = Service('web', project='test', image="alpine", net=Net('other')) + project = Project('test', [web, db], None) + assert project.uses_default_network() + + def test_uses_default_network_custom_name(self): + web = Service('web', project='test', image="alpine", net=Net('other')) + project = Project('test', [web], None) + assert not project.uses_default_network() + + def test_uses_default_network_host(self): + web = Service('web', project='test', image="alpine", net=Net('host')) + project = Project('test', [web], None) + assert not project.uses_default_network() + + def test_uses_default_network_container(self): + container = mock.Mock(id='test') + web = Service( + 'web', + project='test', + image="alpine", + net=ContainerNet(container)) + project = Project('test', [web], None) + assert not project.uses_default_network() + def test_container_without_name(self): self.mock_client.containers.return_value = [ {'Image': 'busybox:latest', 'Id': '1', 'Name': '1'}, From 8444551373670035544edf7bc09fc870521884e6 Mon Sep 17 00:00:00 2001 From: Kevin Greene Date: Mon, 26 Oct 2015 17:39:50 -0400 Subject: [PATCH 24/40] Added ulimits functionality to docker compose Signed-off-by: Kevin Greene --- compose/config/config.py | 12 +++++++ compose/config/fields_schema.json | 19 ++++++++++ compose/service.py | 19 ++++++++++ docs/compose-file.md | 11 ++++++ tests/integration/service_test.py | 31 ++++++++++++++++ tests/unit/config/config_test.py | 60 +++++++++++++++++++++++++++++++ 6 files changed, 152 insertions(+) diff --git a/compose/config/config.py b/compose/config/config.py index 434589d31..7931608d2 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -345,6 +345,15 @@ def validate_extended_service_dict(service_dict, filename, service): "%s services with 'net: container' cannot be extended" % error_prefix) +def validate_ulimits(ulimit_config): + for limit_name, soft_hard_values in six.iteritems(ulimit_config): + if isinstance(soft_hard_values, dict): + if not soft_hard_values['soft'] <= soft_hard_values['hard']: + raise ConfigurationError( + "ulimit_config \"{}\" cannot contain a 'soft' value higher " + "than 'hard' value".format(ulimit_config)) + + def process_container_options(working_dir, service_dict): service_dict = dict(service_dict) @@ -357,6 +366,9 @@ def process_container_options(working_dir, service_dict): if 'labels' in service_dict: service_dict['labels'] = parse_labels(service_dict['labels']) + if 'ulimits' in service_dict: + validate_ulimits(service_dict['ulimits']) + return service_dict diff --git a/compose/config/fields_schema.json b/compose/config/fields_schema.json index e254e3539..f22b513ae 100644 --- a/compose/config/fields_schema.json +++ b/compose/config/fields_schema.json @@ -116,6 +116,25 @@ "security_opt": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, "stdin_open": {"type": "boolean"}, "tty": {"type": "boolean"}, + "ulimits": { + "type": "object", + "patternProperties": { + "^[a-z]+$": { + "oneOf": [ + {"type": "integer"}, + { + "type":"object", + "properties": { + "hard": {"type": "integer"}, + "soft": {"type": "integer"} + }, + "required": ["soft", "hard"], + "additionalProperties": false + } + ] + } + } + }, "user": {"type": "string"}, "volumes": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, "volume_driver": {"type": "string"}, diff --git a/compose/service.py b/compose/service.py index fcf438c09..f2fe66cdf 100644 --- a/compose/service.py +++ b/compose/service.py @@ -676,6 +676,7 @@ class Service(object): devices = options.get('devices', None) cgroup_parent = options.get('cgroup_parent', None) + ulimits = build_ulimits(options.get('ulimits', None)) return self.client.create_host_config( links=self._get_links(link_to_self=one_off), @@ -692,6 +693,7 @@ class Service(object): cap_drop=cap_drop, mem_limit=options.get('mem_limit'), memswap_limit=options.get('memswap_limit'), + ulimits=ulimits, log_config=log_config, extra_hosts=extra_hosts, read_only=read_only, @@ -1055,6 +1057,23 @@ def parse_restart_spec(restart_config): return {'Name': name, 'MaximumRetryCount': int(max_retry_count)} +# Ulimits + + +def build_ulimits(ulimit_config): + if not ulimit_config: + return None + ulimits = [] + for limit_name, soft_hard_values in six.iteritems(ulimit_config): + if isinstance(soft_hard_values, six.integer_types): + ulimits.append({'name': limit_name, 'soft': soft_hard_values, 'hard': soft_hard_values}) + elif isinstance(soft_hard_values, dict): + ulimit_dict = {'name': limit_name} + ulimit_dict.update(soft_hard_values) + ulimits.append(ulimit_dict) + + return ulimits + # Extra hosts diff --git a/docs/compose-file.md b/docs/compose-file.md index 034653efe..9370d9207 100644 --- a/docs/compose-file.md +++ b/docs/compose-file.md @@ -331,6 +331,17 @@ Override the default labeling scheme for each container. - label:user:USER - label:role:ROLE +### ulimits + +Override the default ulimits for a container. You can either use a number +to set the hard and soft limits, or specify them in a dictionary. + + ulimits: + nproc: 65535 + nofile: + soft: 20000 + hard: 40000 + ### volumes, volume\_driver Mount paths as volumes, optionally specifying a path on the host machine diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 804f5219a..2f3be89a3 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -22,6 +22,7 @@ from compose.const import LABEL_SERVICE from compose.const import LABEL_VERSION from compose.container import Container from compose.service import build_extra_hosts +from compose.service import build_ulimits from compose.service import ConfigError from compose.service import ConvergencePlan from compose.service import ConvergenceStrategy @@ -164,6 +165,36 @@ class ServiceTest(DockerClientTestCase): {'www.example.com': '192.168.0.17', 'api.example.com': '192.168.0.18'}) + def sort_dicts_by_name(self, dictionary_list): + return sorted(dictionary_list, key=lambda k: k['name']) + + def test_build_ulimits_with_invalid_options(self): + self.assertRaises(ConfigError, lambda: build_ulimits({'nofile': {'soft': 10000, 'hard': 10}})) + + def test_build_ulimits_with_integers(self): + self.assertEqual(build_ulimits( + {'nofile': {'soft': 10000, 'hard': 20000}}), + [{'name': 'nofile', 'soft': 10000, 'hard': 20000}]) + self.assertEqual(self.sort_dicts_by_name(build_ulimits( + {'nofile': {'soft': 10000, 'hard': 20000}, 'nproc': {'soft': 65535, 'hard': 65535}})), + self.sort_dicts_by_name([{'name': 'nofile', 'soft': 10000, 'hard': 20000}, + {'name': 'nproc', 'soft': 65535, 'hard': 65535}])) + + def test_build_ulimits_with_dicts(self): + self.assertEqual(build_ulimits( + {'nofile': 20000}), + [{'name': 'nofile', 'soft': 20000, 'hard': 20000}]) + self.assertEqual(self.sort_dicts_by_name(build_ulimits( + {'nofile': 20000, 'nproc': 65535})), + self.sort_dicts_by_name([{'name': 'nofile', 'soft': 20000, 'hard': 20000}, + {'name': 'nproc', 'soft': 65535, 'hard': 65535}])) + + def test_build_ulimits_with_integers_and_dicts(self): + self.assertEqual(self.sort_dicts_by_name(build_ulimits( + {'nproc': 65535, 'nofile': {'soft': 10000, 'hard': 20000}})), + self.sort_dicts_by_name([{'name': 'nofile', 'soft': 10000, 'hard': 20000}, + {'name': 'nproc', 'soft': 65535, 'hard': 65535}])) + def test_create_container_with_extra_hosts_list(self): extra_hosts = ['somehost:162.242.195.82', 'otherhost:50.31.209.229'] service = self.create_service('db', extra_hosts=extra_hosts) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index e0d2e870b..f27329ba0 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -349,6 +349,66 @@ class ConfigTest(unittest.TestCase): ) ) + def test_config_ulimits_invalid_keys_validation_error(self): + expected_error_msg = "Service 'web' configuration key 'ulimits' contains unsupported option: 'not_soft_or_hard'" + + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): + config.load( + build_config_details( + {'web': { + 'image': 'busybox', + 'ulimits': { + 'nofile': { + "not_soft_or_hard": 100, + "soft": 10000, + "hard": 20000, + } + } + }}, + 'working_dir', + 'filename.yml' + ) + ) + + def test_config_ulimits_required_keys_validation_error(self): + expected_error_msg = "Service 'web' configuration key 'ulimits' u?'hard' is a required property" + + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): + config.load( + build_config_details( + {'web': { + 'image': 'busybox', + 'ulimits': { + 'nofile': { + "soft": 10000, + } + } + }}, + 'working_dir', + 'filename.yml' + ) + ) + + def test_config_ulimits_soft_greater_than_hard_error(self): + expected_error_msg = "cannot contain a 'soft' value higher than 'hard' value" + + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): + config.load( + build_config_details( + {'web': { + 'image': 'busybox', + 'ulimits': { + 'nofile': { + "soft": 10000, + "hard": 1000 + } + } + }}, + 'working_dir', + 'filename.yml' + ) + ) + def test_valid_config_which_allows_two_type_definitions(self): expose_values = [["8000"], [8000]] for expose in expose_values: From 1208f92d9cca35f378d8d5c497710e5481e7d7b1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 10 Nov 2015 13:37:07 -0500 Subject: [PATCH 25/40] Update doc wording for ulimits. and move tests to the correct module Signed-off-by: Daniel Nephin --- docs/compose-file.md | 5 ++-- tests/integration/service_test.py | 31 ----------------------- tests/unit/service_test.py | 42 +++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 33 deletions(-) diff --git a/docs/compose-file.md b/docs/compose-file.md index 9370d9207..d5aeaa3f2 100644 --- a/docs/compose-file.md +++ b/docs/compose-file.md @@ -333,8 +333,9 @@ Override the default labeling scheme for each container. ### ulimits -Override the default ulimits for a container. You can either use a number -to set the hard and soft limits, or specify them in a dictionary. +Override the default ulimits for a container. You can either specify a single +limit as an integer or soft/hard limits as a mapping. + ulimits: nproc: 65535 diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 2f3be89a3..804f5219a 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -22,7 +22,6 @@ from compose.const import LABEL_SERVICE from compose.const import LABEL_VERSION from compose.container import Container from compose.service import build_extra_hosts -from compose.service import build_ulimits from compose.service import ConfigError from compose.service import ConvergencePlan from compose.service import ConvergenceStrategy @@ -165,36 +164,6 @@ class ServiceTest(DockerClientTestCase): {'www.example.com': '192.168.0.17', 'api.example.com': '192.168.0.18'}) - def sort_dicts_by_name(self, dictionary_list): - return sorted(dictionary_list, key=lambda k: k['name']) - - def test_build_ulimits_with_invalid_options(self): - self.assertRaises(ConfigError, lambda: build_ulimits({'nofile': {'soft': 10000, 'hard': 10}})) - - def test_build_ulimits_with_integers(self): - self.assertEqual(build_ulimits( - {'nofile': {'soft': 10000, 'hard': 20000}}), - [{'name': 'nofile', 'soft': 10000, 'hard': 20000}]) - self.assertEqual(self.sort_dicts_by_name(build_ulimits( - {'nofile': {'soft': 10000, 'hard': 20000}, 'nproc': {'soft': 65535, 'hard': 65535}})), - self.sort_dicts_by_name([{'name': 'nofile', 'soft': 10000, 'hard': 20000}, - {'name': 'nproc', 'soft': 65535, 'hard': 65535}])) - - def test_build_ulimits_with_dicts(self): - self.assertEqual(build_ulimits( - {'nofile': 20000}), - [{'name': 'nofile', 'soft': 20000, 'hard': 20000}]) - self.assertEqual(self.sort_dicts_by_name(build_ulimits( - {'nofile': 20000, 'nproc': 65535})), - self.sort_dicts_by_name([{'name': 'nofile', 'soft': 20000, 'hard': 20000}, - {'name': 'nproc', 'soft': 65535, 'hard': 65535}])) - - def test_build_ulimits_with_integers_and_dicts(self): - self.assertEqual(self.sort_dicts_by_name(build_ulimits( - {'nproc': 65535, 'nofile': {'soft': 10000, 'hard': 20000}})), - self.sort_dicts_by_name([{'name': 'nofile', 'soft': 10000, 'hard': 20000}, - {'name': 'nproc', 'soft': 65535, 'hard': 65535}])) - def test_create_container_with_extra_hosts_list(self): extra_hosts = ['somehost:162.242.195.82', 'otherhost:50.31.209.229'] service = self.create_service('db', extra_hosts=extra_hosts) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index af7007d39..f086b1075 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -12,6 +12,7 @@ from compose.const import LABEL_ONE_OFF from compose.const import LABEL_PROJECT from compose.const import LABEL_SERVICE from compose.container import Container +from compose.service import build_ulimits from compose.service import build_volume_binding from compose.service import ConfigError from compose.service import ContainerNet @@ -435,6 +436,47 @@ class ServiceTest(unittest.TestCase): self.assertEqual(service._get_links(link_to_self=True), []) +def sort_by_name(dictionary_list): + return sorted(dictionary_list, key=lambda k: k['name']) + + +class BuildUlimitsTestCase(unittest.TestCase): + + def test_build_ulimits_with_dict(self): + ulimits = build_ulimits( + { + 'nofile': {'soft': 10000, 'hard': 20000}, + 'nproc': {'soft': 65535, 'hard': 65535} + } + ) + expected = [ + {'name': 'nofile', 'soft': 10000, 'hard': 20000}, + {'name': 'nproc', 'soft': 65535, 'hard': 65535} + ] + assert sort_by_name(ulimits) == sort_by_name(expected) + + def test_build_ulimits_with_ints(self): + ulimits = build_ulimits({'nofile': 20000, 'nproc': 65535}) + expected = [ + {'name': 'nofile', 'soft': 20000, 'hard': 20000}, + {'name': 'nproc', 'soft': 65535, 'hard': 65535} + ] + assert sort_by_name(ulimits) == sort_by_name(expected) + + def test_build_ulimits_with_integers_and_dicts(self): + ulimits = build_ulimits( + { + 'nproc': 65535, + 'nofile': {'soft': 10000, 'hard': 20000} + } + ) + expected = [ + {'name': 'nofile', 'soft': 10000, 'hard': 20000}, + {'name': 'nproc', 'soft': 65535, 'hard': 65535} + ] + assert sort_by_name(ulimits) == sort_by_name(expected) + + class NetTestCase(unittest.TestCase): def test_net(self): From 92d56fab475256a732ba4e98b993c5a5d626d0d5 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 4 Nov 2015 18:00:24 -0500 Subject: [PATCH 26/40] Add a warning when the host volume config is being ignored. Signed-off-by: Daniel Nephin --- compose/service.py | 27 +++++++++++++++++++++++---- tests/integration/service_test.py | 27 +++++++++++++++++++++++++++ tests/unit/service_test.py | 12 ++++++------ 3 files changed, 56 insertions(+), 10 deletions(-) diff --git a/compose/service.py b/compose/service.py index f2fe66cdf..cc4915992 100644 --- a/compose/service.py +++ b/compose/service.py @@ -902,8 +902,10 @@ def merge_volume_bindings(volumes_option, previous_container): if volume.external) if previous_container: + data_volumes = get_container_data_volumes(previous_container, volumes) + warn_on_masked_volume(volumes, data_volumes, previous_container.service) volume_bindings.update( - get_container_data_volumes(previous_container, volumes)) + build_volume_binding(volume) for volume in data_volumes) return list(volume_bindings.values()) @@ -913,7 +915,6 @@ def get_container_data_volumes(container, volumes_option): a mapping of volume bindings for those volumes. """ volumes = [] - container_volumes = container.get('Volumes') or {} image_volumes = [ parse_volume_spec(volume) @@ -933,9 +934,27 @@ def get_container_data_volumes(container, volumes_option): # Copy existing volume from old container volume = volume._replace(external=volume_path) - volumes.append(build_volume_binding(volume)) + volumes.append(volume) - return dict(volumes) + return volumes + + +def warn_on_masked_volume(volumes_option, container_volumes, service): + container_volumes = dict( + (volume.internal, volume.external) + for volume in container_volumes) + + for volume in volumes_option: + if container_volumes.get(volume.internal) != volume.external: + log.warn(( + "Service \"{service}\" is using volume \"{volume}\" from the " + "previous container. Host mapping \"{host_path}\" has no effect. " + "Remove the existing containers (with `docker-compose rm {service}`) " + "to use the host volume mapping." + ).format( + service=service, + volume=volume.internal, + host_path=volume.external)) def build_volume_binding(volume_spec): diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 804f5219a..88214e836 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -369,6 +369,33 @@ class ServiceTest(DockerClientTestCase): self.assertEqual(list(new_container.get('Volumes')), ['/data']) self.assertEqual(new_container.get('Volumes')['/data'], volume_path) + def test_execute_convergence_plan_when_image_volume_masks_config(self): + service = Service( + project='composetest', + name='db', + client=self.client, + build='tests/fixtures/dockerfile-with-volume', + ) + + old_container = create_and_start_container(service) + self.assertEqual(list(old_container.get('Volumes').keys()), ['/data']) + volume_path = old_container.get('Volumes')['/data'] + + service.options['volumes'] = ['/tmp:/data'] + + with mock.patch('compose.service.log') as mock_log: + new_container, = service.execute_convergence_plan( + ConvergencePlan('recreate', [old_container])) + + mock_log.warn.assert_called_once_with(mock.ANY) + _, args, kwargs = mock_log.warn.mock_calls[0] + self.assertIn( + "Service \"db\" is using volume \"/data\" from the previous container", + args[0]) + + self.assertEqual(list(new_container.get('Volumes')), ['/data']) + self.assertEqual(new_container.get('Volumes')['/data'], volume_path) + def test_start_container_passes_through_options(self): db = self.create_service('db') create_and_start_container(db, environment={'FOO': 'BAR'}) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index f086b1075..311d5c95e 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -587,13 +587,13 @@ class ServiceVolumesTest(unittest.TestCase): }, }, has_been_inspected=True) - expected = { - '/existing/volume': '/var/lib/docker/aaaaaaaa:/existing/volume:rw', - '/mnt/image/data': '/var/lib/docker/cccccccc:/mnt/image/data:rw', - } + expected = [ + parse_volume_spec('/var/lib/docker/aaaaaaaa:/existing/volume:rw'), + parse_volume_spec('/var/lib/docker/cccccccc:/mnt/image/data:rw'), + ] - binds = get_container_data_volumes(container, options) - self.assertEqual(binds, expected) + volumes = get_container_data_volumes(container, options) + self.assertEqual(sorted(volumes), sorted(expected)) def test_merge_volume_bindings(self): options = [ From 3313dcb1ce7306fc6936e547c86f6cf53af08a31 Mon Sep 17 00:00:00 2001 From: Yves Peter Date: Wed, 4 Nov 2015 23:40:57 +0100 Subject: [PATCH 27/40] Fixes #1490 progress_stream would print a lot of new lines on "docker-compose pull" if there's no tty. Signed-off-by: Yves Peter --- compose/progress_stream.py | 23 +++++++++++++--------- tests/unit/progress_stream_test.py | 31 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/compose/progress_stream.py b/compose/progress_stream.py index ac8e4b410..c729a6df0 100644 --- a/compose/progress_stream.py +++ b/compose/progress_stream.py @@ -14,8 +14,14 @@ def stream_output(output, stream): for event in utils.json_stream(output): all_events.append(event) + is_progress_event = 'progress' in event or 'progressDetail' in event - if 'progress' in event or 'progressDetail' in event: + if not is_progress_event: + print_output_event(event, stream, is_terminal) + stream.flush() + + # if it's a progress event and we have a terminal, then display the progress bars + elif is_terminal: image_id = event.get('id') if not image_id: continue @@ -27,17 +33,16 @@ def stream_output(output, stream): stream.write("\n") diff = 0 - if is_terminal: - # move cursor up `diff` rows - stream.write("%c[%dA" % (27, diff)) + # move cursor up `diff` rows + stream.write("%c[%dA" % (27, diff)) - print_output_event(event, stream, is_terminal) + print_output_event(event, stream, is_terminal) - if 'id' in event and is_terminal: - # move cursor back down - stream.write("%c[%dB" % (27, diff)) + if 'id' in event: + # move cursor back down + stream.write("%c[%dB" % (27, diff)) - stream.flush() + stream.flush() return all_events diff --git a/tests/unit/progress_stream_test.py b/tests/unit/progress_stream_test.py index d8f7ec836..b01be11a8 100644 --- a/tests/unit/progress_stream_test.py +++ b/tests/unit/progress_stream_test.py @@ -34,3 +34,34 @@ class ProgressStreamTestCase(unittest.TestCase): ] events = progress_stream.stream_output(output, StringIO()) self.assertEqual(len(events), 1) + + def test_stream_output_progress_event_tty(self): + events = [ + b'{"status": "Already exists", "progressDetail": {}, "id": "8d05e3af52b0"}' + ] + + class TTYStringIO(StringIO): + def isatty(self): + return True + + output = TTYStringIO() + events = progress_stream.stream_output(events, output) + self.assertTrue(len(output.getvalue()) > 0) + + def test_stream_output_progress_event_no_tty(self): + events = [ + b'{"status": "Already exists", "progressDetail": {}, "id": "8d05e3af52b0"}' + ] + output = StringIO() + + events = progress_stream.stream_output(events, output) + self.assertEqual(len(output.getvalue()), 0) + + def test_stream_output_no_progress_event_no_tty(self): + events = [ + b'{"status": "Pulling from library/xy", "id": "latest"}' + ] + output = StringIO() + + events = progress_stream.stream_output(events, output) + self.assertTrue(len(output.getvalue()) > 0) From ba90f55075a5e07ec23874707206260eb372734c Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Wed, 11 Nov 2015 11:21:24 -0800 Subject: [PATCH 28/40] Reorganize conditional branches to improve readability Signed-off-by: Joffrey F --- compose/progress_stream.py | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/compose/progress_stream.py b/compose/progress_stream.py index c729a6df0..a6c8e0a26 100644 --- a/compose/progress_stream.py +++ b/compose/progress_stream.py @@ -19,30 +19,33 @@ def stream_output(output, stream): if not is_progress_event: print_output_event(event, stream, is_terminal) stream.flush() + continue + + if not is_terminal: + continue # if it's a progress event and we have a terminal, then display the progress bars - elif is_terminal: - image_id = event.get('id') - if not image_id: - continue + image_id = event.get('id') + if not image_id: + continue - if image_id in lines: - diff = len(lines) - lines[image_id] - else: - lines[image_id] = len(lines) - stream.write("\n") - diff = 0 + if image_id in lines: + diff = len(lines) - lines[image_id] + else: + lines[image_id] = len(lines) + stream.write("\n") + diff = 0 - # move cursor up `diff` rows - stream.write("%c[%dA" % (27, diff)) + # move cursor up `diff` rows + stream.write("%c[%dA" % (27, diff)) - print_output_event(event, stream, is_terminal) + print_output_event(event, stream, is_terminal) - if 'id' in event: - # move cursor back down - stream.write("%c[%dB" % (27, diff)) + if 'id' in event: + # move cursor back down + stream.write("%c[%dB" % (27, diff)) - stream.flush() + stream.flush() return all_events From 83581c3a0ff393af4529734f079afcf26e5c6191 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 6 Nov 2015 16:38:38 -0500 Subject: [PATCH 29/40] Validate additional files before merging them. Consolidates all the top level config handling into `process_config_file` which is now used for both files and merge sources. Signed-off-by: Daniel Nephin --- compose/cli/main.py | 2 +- compose/config/__init__.py | 1 - compose/config/config.py | 56 +++++++++++++++++--------------- compose/config/validation.py | 10 +----- tests/unit/config/config_test.py | 13 ++++++++ 5 files changed, 45 insertions(+), 37 deletions(-) diff --git a/compose/cli/main.py b/compose/cli/main.py index 08c1aee07..806926d84 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -13,12 +13,12 @@ from requests.exceptions import ReadTimeout from .. import __version__ from .. import legacy +from ..config import ConfigurationError from ..config import parse_environment from ..const import DEFAULT_TIMEOUT from ..const import HTTP_TIMEOUT from ..const import IS_WINDOWS_PLATFORM from ..progress_stream import StreamOutputError -from ..project import ConfigurationError from ..project import NoSuchService from ..service import BuildError from ..service import ConvergenceStrategy diff --git a/compose/config/__init__.py b/compose/config/__init__.py index de6f10c94..ec607e087 100644 --- a/compose/config/__init__.py +++ b/compose/config/__init__.py @@ -1,5 +1,4 @@ # flake8: noqa -from .config import ConfigDetails from .config import ConfigurationError from .config import DOCKER_CONFIG_KEYS from .config import find diff --git a/compose/config/config.py b/compose/config/config.py index 7931608d2..feef03877 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -13,7 +13,6 @@ from .errors import ConfigurationError from .interpolation import interpolate_environment_variables from .validation import validate_against_fields_schema from .validation import validate_against_service_schema -from .validation import validate_extended_service_exists from .validation import validate_extends_file_path from .validation import validate_top_level_object @@ -99,6 +98,10 @@ class ConfigFile(namedtuple('_ConfigFile', 'filename config')): :type config: :class:`dict` """ + @classmethod + def from_filename(cls, filename): + return cls(filename, load_yaml(filename)) + def find(base_dir, filenames): if filenames == ['-']: @@ -114,7 +117,7 @@ def find(base_dir, filenames): log.debug("Using configuration files: {}".format(",".join(filenames))) return ConfigDetails( os.path.dirname(filenames[0]), - [ConfigFile(f, load_yaml(f)) for f in filenames]) + [ConfigFile.from_filename(f) for f in filenames]) def get_default_config_files(base_dir): @@ -183,12 +186,10 @@ def load(config_details): validate_paths(service_dict) return service_dict - def load_file(filename, config): - processed_config = interpolate_environment_variables(config) - validate_against_fields_schema(processed_config) + def build_services(filename, config): return [ build_service(filename, name, service_config) - for name, service_config in processed_config.items() + for name, service_config in config.items() ] def merge_services(base, override): @@ -200,16 +201,27 @@ def load(config_details): for name in all_service_names } - config_file = config_details.config_files[0] - validate_top_level_object(config_file.config) + config_file = process_config_file(config_details.config_files[0]) for next_file in config_details.config_files[1:]: - validate_top_level_object(next_file.config) + next_file = process_config_file(next_file) - config_file = ConfigFile( - config_file.filename, - merge_services(config_file.config, next_file.config)) + config = merge_services(config_file.config, next_file.config) + config_file = config_file._replace(config=config) - return load_file(config_file.filename, config_file.config) + return build_services(config_file.filename, config_file.config) + + +def process_config_file(config_file, service_name=None): + validate_top_level_object(config_file.config) + processed_config = interpolate_environment_variables(config_file.config) + validate_against_fields_schema(processed_config) + + if service_name and service_name not in processed_config: + raise ConfigurationError( + "Cannot extend service '{}' in {}: Service not found".format( + service_name, config_file.filename)) + + return config_file._replace(config=processed_config) class ServiceLoader(object): @@ -259,22 +271,13 @@ class ServiceLoader(object): if not isinstance(extends, dict): extends = {'service': extends} - validate_extends_file_path(self.service_name, extends, self.filename) config_path = self.get_extended_config_path(extends) service_name = extends['service'] - config = load_yaml(config_path) - validate_top_level_object(config) - full_extended_config = interpolate_environment_variables(config) - - validate_extended_service_exists( - service_name, - full_extended_config, - config_path - ) - validate_against_fields_schema(full_extended_config) - - service_config = full_extended_config[service_name] + extended_file = process_config_file( + ConfigFile.from_filename(config_path), + service_name=service_name) + service_config = extended_file.config[service_name] return config_path, service_config, service_name def resolve_extends(self, extended_config_path, service_config, service_name): @@ -304,6 +307,7 @@ class ServiceLoader(object): need to obtain a full path too or we are extending from a service defined in our own file. """ + validate_extends_file_path(self.service_name, extends_options, self.filename) if 'file' in extends_options: return expand_path(self.working_dir, extends_options['file']) return self.filename diff --git a/compose/config/validation.py b/compose/config/validation.py index 542081d52..3bd404109 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -96,14 +96,6 @@ def validate_extends_file_path(service_name, extends_options, filename): ) -def validate_extended_service_exists(extended_service_name, full_extended_config, extended_config_path): - if extended_service_name not in full_extended_config: - msg = ( - "Cannot extend service '%s' in %s: Service not found" - ) % (extended_service_name, extended_config_path) - raise ConfigurationError(msg) - - def get_unsupported_config_msg(service_name, error_key): msg = "Unsupported config option for '{}' service: '{}'".format(service_name, error_key) if error_key in DOCKER_CONFIG_HINTS: @@ -264,7 +256,7 @@ def process_errors(errors, service_name=None): msg)) else: root_msgs.append( - "Service '{}' doesn\'t have any configuration options. " + "Service \"{}\" doesn't have any configuration options. " "All top level keys in your docker-compose.yml must map " "to a dictionary of configuration options.'".format(service_name)) elif error.validator == 'required': diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index f27329ba0..ab34f4dcb 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -195,6 +195,19 @@ class ConfigTest(unittest.TestCase): ] self.assertEqual(service_sort(service_dicts), service_sort(expected)) + def test_load_with_multiple_files_and_invalid_override(self): + base_file = config.ConfigFile( + 'base.yaml', + {'web': {'image': 'example/web'}}) + override_file = config.ConfigFile( + 'override.yaml', + {'bogus': 'thing'}) + details = config.ConfigDetails('.', [base_file, override_file]) + + with pytest.raises(ConfigurationError) as exc: + config.load(details) + assert 'Service "bogus" doesn\'t have any configuration' in exc.exconly() + def test_config_valid_service_names(self): for valid_name in ['_', '-', '.__.', '_what-up.', 'what_.up----', 'whatup']: config.load( From 87d79d4d993bc06c16ae19aba71aadd88292abb3 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 6 Nov 2015 17:18:47 -0500 Subject: [PATCH 30/40] Rename ServiceLoader to ServiceExtendsResolver ServiceLoader has evolved to be not really all that related to "loading" a service. It's responsibility is more to do with handling the `extends` field, which is only part of loading. The class and its primary method (make_service_dict()) were renamed to better reflect their responsibility. As part of that change process_container_options() was removed from make_service_dict() and renamed to process_service(). It contains logic for handling the non-extends options. This change allows us to remove the hacks from testcase.py and only call the functions we need to format a service dict correctly for integration tests. Signed-off-by: Daniel Nephin --- compose/config/config.py | 27 ++++++++++++--------------- tests/integration/service_test.py | 25 ++++++++++++++++++++++--- tests/integration/testcases.py | 21 +++++++-------------- tests/unit/config/config_test.py | 7 ++++--- 4 files changed, 45 insertions(+), 35 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index feef03877..7846ea7b4 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -177,12 +177,12 @@ def load(config_details): """ def build_service(filename, service_name, service_dict): - loader = ServiceLoader( + resolver = ServiceExtendsResolver( config_details.working_dir, filename, service_name, service_dict) - service_dict = loader.make_service_dict() + service_dict = process_service(config_details.working_dir, resolver.run()) validate_paths(service_dict) return service_dict @@ -224,7 +224,7 @@ def process_config_file(config_file, service_name=None): return config_file._replace(config=processed_config) -class ServiceLoader(object): +class ServiceExtendsResolver(object): def __init__( self, working_dir, @@ -234,7 +234,7 @@ class ServiceLoader(object): already_seen=None ): if working_dir is None: - raise ValueError("No working_dir passed to ServiceLoader()") + raise ValueError("No working_dir passed to ServiceExtendsResolver()") self.working_dir = os.path.abspath(working_dir) @@ -251,7 +251,7 @@ class ServiceLoader(object): if self.signature(name) in self.already_seen: raise CircularReference(self.already_seen + [self.signature(name)]) - def make_service_dict(self): + def run(self): service_dict = dict(self.service_dict) env = resolve_environment(self.working_dir, self.service_dict) if env: @@ -264,7 +264,7 @@ class ServiceLoader(object): if not self.already_seen: validate_against_service_schema(service_dict, self.service_name) - return process_container_options(self.working_dir, service_dict) + return service_dict def validate_and_construct_extends(self): extends = self.service_dict['extends'] @@ -281,19 +281,16 @@ class ServiceLoader(object): return config_path, service_config, service_name def resolve_extends(self, extended_config_path, service_config, service_name): - other_working_dir = os.path.dirname(extended_config_path) - other_already_seen = self.already_seen + [self.signature(self.service_name)] - - other_loader = ServiceLoader( - other_working_dir, + resolver = ServiceExtendsResolver( + os.path.dirname(extended_config_path), extended_config_path, self.service_name, service_config, - already_seen=other_already_seen, + already_seen=self.already_seen + [self.signature(self.service_name)], ) - other_loader.detect_cycle(service_name) - other_service_dict = other_loader.make_service_dict() + resolver.detect_cycle(service_name) + other_service_dict = process_service(resolver.working_dir, resolver.run()) validate_extended_service_dict( other_service_dict, extended_config_path, @@ -358,7 +355,7 @@ def validate_ulimits(ulimit_config): "than 'hard' value".format(ulimit_config)) -def process_container_options(working_dir, service_dict): +def process_service(working_dir, service_dict): service_dict = dict(service_dict) if 'volumes' in service_dict and service_dict.get('volume_driver') is None: diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 88214e836..aaa4f01ec 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -842,7 +842,13 @@ class ServiceTest(DockerClientTestCase): environment=['ONE=1', 'TWO=2', 'THREE=3'], env_file=['tests/fixtures/env/one.env', 'tests/fixtures/env/two.env']) env = create_and_start_container(service).environment - for k, v in {'ONE': '1', 'TWO': '2', 'THREE': '3', 'FOO': 'baz', 'DOO': 'dah'}.items(): + for k, v in { + 'ONE': '1', + 'TWO': '2', + 'THREE': '3', + 'FOO': 'baz', + 'DOO': 'dah' + }.items(): self.assertEqual(env[k], v) @mock.patch.dict(os.environ) @@ -850,9 +856,22 @@ class ServiceTest(DockerClientTestCase): os.environ['FILE_DEF'] = 'E1' os.environ['FILE_DEF_EMPTY'] = 'E2' os.environ['ENV_DEF'] = 'E3' - service = self.create_service('web', environment={'FILE_DEF': 'F1', 'FILE_DEF_EMPTY': '', 'ENV_DEF': None, 'NO_DEF': None}) + service = self.create_service( + 'web', + environment={ + 'FILE_DEF': 'F1', + 'FILE_DEF_EMPTY': '', + 'ENV_DEF': None, + 'NO_DEF': None + } + ) env = create_and_start_container(service).environment - for k, v in {'FILE_DEF': 'F1', 'FILE_DEF_EMPTY': '', 'ENV_DEF': 'E3', 'NO_DEF': ''}.items(): + for k, v in { + 'FILE_DEF': 'F1', + 'FILE_DEF_EMPTY': '', + 'ENV_DEF': 'E3', + 'NO_DEF': '' + }.items(): self.assertEqual(env[k], v) def test_with_high_enough_api_version_we_get_default_network_mode(self): diff --git a/tests/integration/testcases.py b/tests/integration/testcases.py index 60e67b5bf..5ee6a4212 100644 --- a/tests/integration/testcases.py +++ b/tests/integration/testcases.py @@ -7,7 +7,8 @@ from pytest import skip from .. import unittest from compose.cli.docker_client import docker_client -from compose.config.config import ServiceLoader +from compose.config.config import process_service +from compose.config.config import resolve_environment from compose.const import LABEL_PROJECT from compose.progress_stream import stream_output from compose.service import Service @@ -42,23 +43,15 @@ class DockerClientTestCase(unittest.TestCase): if 'command' not in kwargs: kwargs['command'] = ["top"] - workaround_options = {} - for option in ['links', 'volumes_from', 'net']: - if option in kwargs: - workaround_options[option] = kwargs.pop(option, None) - - options = ServiceLoader( - working_dir='.', - filename=None, - service_name=name, - service_dict=kwargs - ).make_service_dict() - options.update(workaround_options) + # TODO: remove this once #2299 is fixed + kwargs['name'] = name + options = process_service('.', kwargs) + options['environment'] = resolve_environment('.', kwargs) labels = options.setdefault('labels', {}) labels['com.docker.compose.test-name'] = self.id() - return Service(project='composetest', client=self.client, **options) + return Service(client=self.client, project='composetest', **options) def check_build(self, *args, **kwargs): kwargs.setdefault('rm', True) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index ab34f4dcb..2835a4317 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -18,13 +18,14 @@ from tests import unittest def make_service_dict(name, service_dict, working_dir, filename=None): """ - Test helper function to construct a ServiceLoader + Test helper function to construct a ServiceExtendsResolver """ - return config.ServiceLoader( + resolver = config.ServiceExtendsResolver( working_dir=working_dir, filename=filename, service_name=name, - service_dict=service_dict).make_service_dict() + service_dict=service_dict) + return config.process_service(working_dir, resolver.run()) def service_sort(services): From 3a43110f062a00b7e1256d7594f0dd345447c6fc Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 9 Nov 2015 14:24:44 -0500 Subject: [PATCH 31/40] Fix a bug in ExtendsResolver where the service name of the extended service was wrong. This bug can be seen by the change to the test case. When the extended service uses a different name, the error was reported incorrectly. By fixing this bug we can simplify self.signature and self.detect_cycles to always use self.service_name. Signed-off-by: Daniel Nephin --- compose/config/config.py | 20 +++++++++++--------- tests/fixtures/extends/circle-1.yml | 2 +- tests/fixtures/extends/circle-2.yml | 2 +- tests/unit/config/config_test.py | 23 ++++++++++++----------- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 7846ea7b4..1ddb2abe0 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -247,11 +247,17 @@ class ServiceExtendsResolver(object): self.service_name = service_name self.service_dict['name'] = service_name - def detect_cycle(self, name): - if self.signature(name) in self.already_seen: - raise CircularReference(self.already_seen + [self.signature(name)]) + @property + def signature(self): + return self.filename, self.service_name + + def detect_cycle(self): + if self.signature in self.already_seen: + raise CircularReference(self.already_seen + [self.signature]) def run(self): + self.detect_cycle() + service_dict = dict(self.service_dict) env = resolve_environment(self.working_dir, self.service_dict) if env: @@ -284,12 +290,11 @@ class ServiceExtendsResolver(object): resolver = ServiceExtendsResolver( os.path.dirname(extended_config_path), extended_config_path, - self.service_name, + service_name, service_config, - already_seen=self.already_seen + [self.signature(self.service_name)], + already_seen=self.already_seen + [self.signature], ) - resolver.detect_cycle(service_name) other_service_dict = process_service(resolver.working_dir, resolver.run()) validate_extended_service_dict( other_service_dict, @@ -309,9 +314,6 @@ class ServiceExtendsResolver(object): return expand_path(self.working_dir, extends_options['file']) return self.filename - def signature(self, name): - return self.filename, name - def resolve_environment(working_dir, service_dict): """Unpack any environment variables from an env_file, if set. diff --git a/tests/fixtures/extends/circle-1.yml b/tests/fixtures/extends/circle-1.yml index a034e9619..d88ea61d0 100644 --- a/tests/fixtures/extends/circle-1.yml +++ b/tests/fixtures/extends/circle-1.yml @@ -5,7 +5,7 @@ bar: web: extends: file: circle-2.yml - service: web + service: other baz: image: busybox quux: diff --git a/tests/fixtures/extends/circle-2.yml b/tests/fixtures/extends/circle-2.yml index fa6ddefcc..de05bc8da 100644 --- a/tests/fixtures/extends/circle-2.yml +++ b/tests/fixtures/extends/circle-2.yml @@ -2,7 +2,7 @@ foo: image: busybox bar: image: busybox -web: +other: extends: file: circle-1.yml service: web diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 2835a4317..717831681 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -1080,18 +1080,19 @@ class ExtendsTest(unittest.TestCase): ])) def test_circular(self): - try: + with pytest.raises(config.CircularReference) as exc: load_from_filename('tests/fixtures/extends/circle-1.yml') - raise Exception("Expected config.CircularReference to be raised") - except config.CircularReference as e: - self.assertEqual( - [(os.path.basename(filename), service_name) for (filename, service_name) in e.trail], - [ - ('circle-1.yml', 'web'), - ('circle-2.yml', 'web'), - ('circle-1.yml', 'web'), - ], - ) + + path = [ + (os.path.basename(filename), service_name) + for (filename, service_name) in exc.value.trail + ] + expected = [ + ('circle-1.yml', 'web'), + ('circle-2.yml', 'other'), + ('circle-1.yml', 'web'), + ] + self.assertEqual(path, expected) def test_extends_validation_empty_dictionary(self): with self.assertRaisesRegexp(ConfigurationError, 'service'): From 7fc577c31de5f316de543576609c675b177291a7 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 6 Nov 2015 11:57:27 -0500 Subject: [PATCH 32/40] Remove name from config schema. Refactors config validation of a service to use a ServiceConfig data object. Instead of passing around a bunch of related scalars, we can use the ServiceConfig object as a parameter to most of the service validation functions. This allows for a fix to the config schema, where the name is a field in the schema, but not actually in the configuration. My passing the name around as part of the ServiceConfig object, we don't need to add it to the config options. Fixes #2299 validate_against_service_schema() is moved from a conditional branch in ServiceExtendsResolver() to happen as one of the last steps after all configuration is merged. This schema only contains constraints which only need to be true at the very end of merging. Signed-off-by: Daniel Nephin --- compose/config/config.py | 101 +++++++++++++++-------------- compose/config/fields_schema.json | 1 - compose/config/service_schema.json | 6 -- compose/config/validation.py | 2 +- tests/integration/testcases.py | 9 ++- tests/unit/config/config_test.py | 10 +-- 6 files changed, 62 insertions(+), 67 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 1ddb2abe0..21788551d 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -103,6 +103,20 @@ class ConfigFile(namedtuple('_ConfigFile', 'filename config')): return cls(filename, load_yaml(filename)) +class ServiceConfig(namedtuple('_ServiceConfig', 'working_dir filename name config')): + + @classmethod + def with_abs_paths(cls, working_dir, filename, name, config): + if not working_dir: + raise ValueError("No working_dir for ServiceConfig.") + + return cls( + os.path.abspath(working_dir), + os.path.abspath(filename) if filename else filename, + name, + config) + + def find(base_dir, filenames): if filenames == ['-']: return ConfigDetails( @@ -177,19 +191,22 @@ def load(config_details): """ def build_service(filename, service_name, service_dict): - resolver = ServiceExtendsResolver( + service_config = ServiceConfig.with_abs_paths( config_details.working_dir, filename, service_name, service_dict) - service_dict = process_service(config_details.working_dir, resolver.run()) + resolver = ServiceExtendsResolver(service_config) + service_dict = process_service(resolver.run()) + validate_against_service_schema(service_dict, service_config.name) validate_paths(service_dict) + service_dict['name'] = service_config.name return service_dict - def build_services(filename, config): + def build_services(config_file): return [ - build_service(filename, name, service_config) - for name, service_config in config.items() + build_service(config_file.filename, name, service_dict) + for name, service_dict in config_file.config.items() ] def merge_services(base, override): @@ -208,7 +225,7 @@ def load(config_details): config = merge_services(config_file.config, next_file.config) config_file = config_file._replace(config=config) - return build_services(config_file.filename, config_file.config) + return build_services(config_file) def process_config_file(config_file, service_name=None): @@ -225,31 +242,14 @@ def process_config_file(config_file, service_name=None): class ServiceExtendsResolver(object): - def __init__( - self, - working_dir, - filename, - service_name, - service_dict, - already_seen=None - ): - if working_dir is None: - raise ValueError("No working_dir passed to ServiceExtendsResolver()") - - self.working_dir = os.path.abspath(working_dir) - - if filename: - self.filename = os.path.abspath(filename) - else: - self.filename = filename + def __init__(self, service_config, already_seen=None): + self.service_config = service_config + self.working_dir = service_config.working_dir self.already_seen = already_seen or [] - self.service_dict = service_dict.copy() - self.service_name = service_name - self.service_dict['name'] = service_name @property def signature(self): - return self.filename, self.service_name + return self.service_config.filename, self.service_config.name def detect_cycle(self): if self.signature in self.already_seen: @@ -258,8 +258,8 @@ class ServiceExtendsResolver(object): def run(self): self.detect_cycle() - service_dict = dict(self.service_dict) - env = resolve_environment(self.working_dir, self.service_dict) + service_dict = dict(self.service_config.config) + env = resolve_environment(self.working_dir, self.service_config.config) if env: service_dict['environment'] = env service_dict.pop('env_file', None) @@ -267,13 +267,10 @@ class ServiceExtendsResolver(object): if 'extends' in service_dict: service_dict = self.resolve_extends(*self.validate_and_construct_extends()) - if not self.already_seen: - validate_against_service_schema(service_dict, self.service_name) - - return service_dict + return self.service_config._replace(config=service_dict) def validate_and_construct_extends(self): - extends = self.service_dict['extends'] + extends = self.service_config.config['extends'] if not isinstance(extends, dict): extends = {'service': extends} @@ -286,33 +283,38 @@ class ServiceExtendsResolver(object): service_config = extended_file.config[service_name] return config_path, service_config, service_name - def resolve_extends(self, extended_config_path, service_config, service_name): + def resolve_extends(self, extended_config_path, service_dict, service_name): resolver = ServiceExtendsResolver( - os.path.dirname(extended_config_path), - extended_config_path, - service_name, - service_config, - already_seen=self.already_seen + [self.signature], - ) + ServiceConfig.with_abs_paths( + os.path.dirname(extended_config_path), + extended_config_path, + service_name, + service_dict), + already_seen=self.already_seen + [self.signature]) - other_service_dict = process_service(resolver.working_dir, resolver.run()) + service_config = resolver.run() + other_service_dict = process_service(service_config) validate_extended_service_dict( other_service_dict, extended_config_path, service_name, ) - return merge_service_dicts(other_service_dict, self.service_dict) + return merge_service_dicts(other_service_dict, self.service_config.config) def get_extended_config_path(self, extends_options): """Service we are extending either has a value for 'file' set, which we need to obtain a full path too or we are extending from a service defined in our own file. """ - validate_extends_file_path(self.service_name, extends_options, self.filename) + filename = self.service_config.filename + validate_extends_file_path( + self.service_config.name, + extends_options, + filename) if 'file' in extends_options: return expand_path(self.working_dir, extends_options['file']) - return self.filename + return filename def resolve_environment(working_dir, service_dict): @@ -357,8 +359,9 @@ def validate_ulimits(ulimit_config): "than 'hard' value".format(ulimit_config)) -def process_service(working_dir, service_dict): - service_dict = dict(service_dict) +def process_service(service_config): + working_dir = service_config.working_dir + service_dict = dict(service_config.config) if 'volumes' in service_dict and service_dict.get('volume_driver') is None: service_dict['volumes'] = resolve_volume_paths(working_dir, service_dict) @@ -505,12 +508,12 @@ def env_vars_from_file(filename): def resolve_volume_paths(working_dir, service_dict): return [ - resolve_volume_path(working_dir, volume, service_dict['name']) + resolve_volume_path(working_dir, volume) for volume in service_dict['volumes'] ] -def resolve_volume_path(working_dir, volume, service_name): +def resolve_volume_path(working_dir, volume): container_path, host_path = split_path_mapping(volume) if host_path is not None: diff --git a/compose/config/fields_schema.json b/compose/config/fields_schema.json index f22b513ae..7723f2fbc 100644 --- a/compose/config/fields_schema.json +++ b/compose/config/fields_schema.json @@ -89,7 +89,6 @@ "mac_address": {"type": "string"}, "mem_limit": {"type": ["number", "string"]}, "memswap_limit": {"type": ["number", "string"]}, - "name": {"type": "string"}, "net": {"type": "string"}, "pid": {"type": ["string", "null"]}, diff --git a/compose/config/service_schema.json b/compose/config/service_schema.json index 5cb5d6d07..221c5d8d7 100644 --- a/compose/config/service_schema.json +++ b/compose/config/service_schema.json @@ -3,12 +3,6 @@ "type": "object", - "properties": { - "name": {"type": "string"} - }, - - "required": ["name"], - "allOf": [ {"$ref": "fields_schema.json#/definitions/service"}, {"$ref": "#/definitions/service_constraints"} diff --git a/compose/config/validation.py b/compose/config/validation.py index 3bd404109..d3bcb35c4 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -195,7 +195,7 @@ def process_errors(errors, service_name=None): for error in errors: # handle root level errors - if len(error.path) == 0 and not error.instance.get('name'): + if len(error.path) == 0 and not service_name: if error.validator == 'type': msg = "Top level object needs to be a dictionary. Check your .yml file that you have defined a service at the top level." root_msgs.append(msg) diff --git a/tests/integration/testcases.py b/tests/integration/testcases.py index 5ee6a4212..d63f05916 100644 --- a/tests/integration/testcases.py +++ b/tests/integration/testcases.py @@ -9,6 +9,7 @@ from .. import unittest from compose.cli.docker_client import docker_client from compose.config.config import process_service from compose.config.config import resolve_environment +from compose.config.config import ServiceConfig from compose.const import LABEL_PROJECT from compose.progress_stream import stream_output from compose.service import Service @@ -43,15 +44,13 @@ class DockerClientTestCase(unittest.TestCase): if 'command' not in kwargs: kwargs['command'] = ["top"] - # TODO: remove this once #2299 is fixed - kwargs['name'] = name - - options = process_service('.', kwargs) + service_config = ServiceConfig('.', None, name, kwargs) + options = process_service(service_config) options['environment'] = resolve_environment('.', kwargs) labels = options.setdefault('labels', {}) labels['com.docker.compose.test-name'] = self.id() - return Service(client=self.client, project='composetest', **options) + return Service(name, client=self.client, project='composetest', **options) def check_build(self, *args, **kwargs): kwargs.setdefault('rm', True) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 717831681..022ec7c7d 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -20,12 +20,12 @@ def make_service_dict(name, service_dict, working_dir, filename=None): """ Test helper function to construct a ServiceExtendsResolver """ - resolver = config.ServiceExtendsResolver( + resolver = config.ServiceExtendsResolver(config.ServiceConfig( working_dir=working_dir, filename=filename, - service_name=name, - service_dict=service_dict) - return config.process_service(working_dir, resolver.run()) + name=name, + config=service_dict)) + return config.process_service(resolver.run()) def service_sort(services): @@ -651,7 +651,7 @@ class VolumeConfigTest(unittest.TestCase): def test_volume_path_with_non_ascii_directory(self): volume = u'/Füü/data:/data' - container_path = config.resolve_volume_path(".", volume, "test") + container_path = config.resolve_volume_path(".", volume) self.assertEqual(container_path, volume) From 0ab76bb8bccf507dc17b2b016d5ecabc1fcb5dc6 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 9 Nov 2015 15:39:55 -0500 Subject: [PATCH 33/40] Add a test for invalid field 'name', and fix an existing test for invalid service names. Signed-off-by: Daniel Nephin --- tests/unit/config/config_test.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 022ec7c7d..add7a5a48 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -77,8 +77,8 @@ class ConfigTest(unittest.TestCase): ) def test_config_invalid_service_names(self): - with self.assertRaises(ConfigurationError): - for invalid_name in ['?not?allowed', ' ', '', '!', '/', '\xe2']: + for invalid_name in ['?not?allowed', ' ', '', '!', '/', '\xe2']: + with pytest.raises(ConfigurationError): config.load( build_config_details( {invalid_name: {'image': 'busybox'}}, @@ -87,6 +87,16 @@ class ConfigTest(unittest.TestCase): ) ) + def test_load_with_invalid_field_name(self): + config_details = build_config_details( + {'web': {'image': 'busybox', 'name': 'bogus'}}, + 'working_dir', + 'filename.yml') + with pytest.raises(ConfigurationError) as exc: + config.load(config_details) + error_msg = "Unsupported config option for 'web' service: 'name'" + assert error_msg in exc.exconly() + def test_config_integer_service_name_raise_validation_error(self): expected_error_msg = "Service name: 1 needs to be a string, eg '1'" with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): From 63c3e6f58c124d757705711dfc56b50130136615 Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Wed, 11 Nov 2015 12:45:02 -0800 Subject: [PATCH 34/40] Allow dashes in environment variable names See http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html Environment variable names used by the utilities in the Shell and Utilities volume of POSIX.1-2008 consist solely of uppercase letters, digits, and the ( '_' ) from the characters defined in Portable Character Set and do not begin with a digit. Other characters may be permitted by an implementation; applications shall tolerate the presence of such names. Signed-off-by: Joffrey F --- compose/config/fields_schema.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compose/config/fields_schema.json b/compose/config/fields_schema.json index 7723f2fbc..3d592bbc7 100644 --- a/compose/config/fields_schema.json +++ b/compose/config/fields_schema.json @@ -40,7 +40,7 @@ { "type": "object", "patternProperties": { - "^[^-]+$": { + ".+": { "type": ["string", "number", "boolean", "null"], "format": "environment" } From d52c969f947750efb85e10b7a598f3c7b92d3996 Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Wed, 11 Nov 2015 12:52:30 -0800 Subject: [PATCH 35/40] Add test for environment variable dashes support Signed-off-by: Joffrey F --- tests/unit/config/config_test.py | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index add7a5a48..324061365 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -480,20 +480,18 @@ class ConfigTest(unittest.TestCase): self.assertTrue(mock_logging.warn.called) self.assertTrue(expected_warning_msg in mock_logging.warn.call_args[0][0]) - def test_config_invalid_environment_dict_key_raises_validation_error(self): - expected_error_msg = "Service 'web' configuration key 'environment' contains unsupported option: '---'" - - with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): - config.load( - build_config_details( - {'web': { - 'image': 'busybox', - 'environment': {'---': 'nope'} - }}, - 'working_dir', - 'filename.yml' - ) + def test_config_valid_environment_dict_key_contains_dashes(self): + services = config.load( + build_config_details( + {'web': { + 'image': 'busybox', + 'environment': {'SPRING_JPA_HIBERNATE_DDL-AUTO': 'none'} + }}, + 'working_dir', + 'filename.yml' ) + ) + self.assertEqual(services[0]['environment']['SPRING_JPA_HIBERNATE_DDL-AUTO'], 'none') def test_load_yaml_with_yaml_error(self): tmpdir = py.test.ensuretemp('invalid_yaml_test') From 285e52cc7c17764b54a683e849787ee20c476349 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 12 Nov 2015 12:40:36 -0500 Subject: [PATCH 36/40] Add ids to config schemas Also enforce a max complexity for functions and add some new tests for config. Signed-off-by: Daniel Nephin --- compose/config/fields_schema.json | 6 ++++-- compose/config/service_schema.json | 11 ++++------- compose/config/validation.py | 5 ++++- tests/unit/config/config_test.py | 24 ++++++++++++++++-------- tox.ini | 2 ++ 5 files changed, 30 insertions(+), 18 deletions(-) diff --git a/compose/config/fields_schema.json b/compose/config/fields_schema.json index 3d592bbc7..ca3b3a502 100644 --- a/compose/config/fields_schema.json +++ b/compose/config/fields_schema.json @@ -2,15 +2,18 @@ "$schema": "http://json-schema.org/draft-04/schema#", "type": "object", + "id": "fields_schema.json", "patternProperties": { "^[a-zA-Z0-9._-]+$": { "$ref": "#/definitions/service" } }, + "additionalProperties": false, "definitions": { "service": { + "id": "#/definitions/service", "type": "object", "properties": { @@ -167,6 +170,5 @@ ] } - }, - "additionalProperties": false + } } diff --git a/compose/config/service_schema.json b/compose/config/service_schema.json index 221c5d8d7..05774efdd 100644 --- a/compose/config/service_schema.json +++ b/compose/config/service_schema.json @@ -1,15 +1,17 @@ { "$schema": "http://json-schema.org/draft-04/schema#", + "id": "service_schema.json", "type": "object", "allOf": [ {"$ref": "fields_schema.json#/definitions/service"}, - {"$ref": "#/definitions/service_constraints"} + {"$ref": "#/definitions/constraints"} ], "definitions": { - "service_constraints": { + "constraints": { + "id": "#/definitions/constraints", "anyOf": [ { "required": ["build"], @@ -21,13 +23,8 @@ {"required": ["build"]}, {"required": ["dockerfile"]} ]} - }, - { - "required": ["extends"], - "not": {"required": ["build", "image"]} } ] } } - } diff --git a/compose/config/validation.py b/compose/config/validation.py index d3bcb35c4..962d41e2f 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -307,7 +307,10 @@ def _validate_against_schema(config, schema_filename, format_checker=[], service schema = json.load(schema_fh) resolver = RefResolver(resolver_full_path, schema) - validation_output = Draft4Validator(schema, resolver=resolver, format_checker=FormatChecker(format_checker)) + validation_output = Draft4Validator( + schema, + resolver=resolver, + format_checker=FormatChecker(format_checker)) errors = [error for error in sorted(validation_output.iter_errors(config), key=str)] if errors: diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 324061365..0e2fb7d72 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -78,14 +78,12 @@ class ConfigTest(unittest.TestCase): def test_config_invalid_service_names(self): for invalid_name in ['?not?allowed', ' ', '', '!', '/', '\xe2']: - with pytest.raises(ConfigurationError): - config.load( - build_config_details( - {invalid_name: {'image': 'busybox'}}, - 'working_dir', - 'filename.yml' - ) - ) + with pytest.raises(ConfigurationError) as exc: + config.load(build_config_details( + {invalid_name: {'image': 'busybox'}}, + 'working_dir', + 'filename.yml')) + assert 'Invalid service name \'%s\'' % invalid_name in exc.exconly() def test_load_with_invalid_field_name(self): config_details = build_config_details( @@ -97,6 +95,16 @@ class ConfigTest(unittest.TestCase): error_msg = "Unsupported config option for 'web' service: 'name'" assert error_msg in exc.exconly() + def test_load_invalid_service_definition(self): + config_details = build_config_details( + {'web': 'wrong'}, + 'working_dir', + 'filename.yml') + with pytest.raises(ConfigurationError) as exc: + config.load(config_details) + error_msg = "Service \"web\" doesn\'t have any configuration options" + assert error_msg in exc.exconly() + def test_config_integer_service_name_raise_validation_error(self): expected_error_msg = "Service name: 1 needs to be a string, eg '1'" with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): diff --git a/tox.ini b/tox.ini index f05c5ed26..d1098a55a 100644 --- a/tox.ini +++ b/tox.ini @@ -43,4 +43,6 @@ directory = coverage-html [flake8] # Allow really long lines for now max-line-length = 140 +# Set this high for now +max-complexity = 20 exclude = compose/packages From 34166ef5a4a64cfd0cd13acb607fbd1b36412232 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 12 Nov 2015 12:43:29 -0500 Subject: [PATCH 37/40] Refactor process_errors into smaller functions So that it passed new max-complexity requirement Signed-off-by: Daniel Nephin --- compose/config/validation.py | 316 +++++++++++++++---------------- tests/unit/config/config_test.py | 2 +- 2 files changed, 149 insertions(+), 169 deletions(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index 962d41e2f..2928238c3 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -109,189 +109,169 @@ def anglicize_validator(validator): return 'a ' + validator -def process_errors(errors, service_name=None): +def handle_error_for_schema_with_id(error, service_name): + schema_id = error.schema['id'] + + if schema_id == 'fields_schema.json' and error.validator == 'additionalProperties': + return "Invalid service name '{}' - only {} characters are allowed".format( + # The service_name is the key to the json object + list(error.instance)[0], + VALID_NAME_CHARS) + + if schema_id == '#/definitions/constraints': + if 'image' in error.instance and 'build' in error.instance: + return ( + "Service '{}' has both an image and build path specified. " + "A service can either be built to image or use an existing " + "image, not both.".format(service_name)) + if 'image' not in error.instance and 'build' not in error.instance: + return ( + "Service '{}' has neither an image nor a build path " + "specified. Exactly one must be provided.".format(service_name)) + if 'image' in error.instance and 'dockerfile' in error.instance: + return ( + "Service '{}' has both an image and alternate Dockerfile. " + "A service can either be built to image or use an existing " + "image, not both.".format(service_name)) + + if schema_id == '#/definitions/service': + if error.validator == 'additionalProperties': + invalid_config_key = parse_key_from_error_msg(error) + return get_unsupported_config_msg(service_name, invalid_config_key) + + +def handle_generic_service_error(error, service_name): + config_key = " ".join("'%s'" % k for k in error.path) + msg_format = None + error_msg = error.message + + if error.validator == 'oneOf': + msg_format = "Service '{}' configuration key {} {}" + error_msg = _parse_oneof_validator(error) + + elif error.validator == 'type': + msg_format = ("Service '{}' configuration key {} contains an invalid " + "type, it should be {}") + error_msg = _parse_valid_types_from_validator(error.validator_value) + + # TODO: no test case for this branch, there are no config options + # which exercise this branch + elif error.validator == 'required': + msg_format = "Service '{}' configuration key '{}' is invalid, {}" + + elif error.validator == 'dependencies': + msg_format = "Service '{}' configuration key '{}' is invalid: {}" + config_key = list(error.validator_value.keys())[0] + required_keys = ",".join(error.validator_value[config_key]) + error_msg = "when defining '{}' you must set '{}' as well".format( + config_key, + required_keys) + + elif error.path: + msg_format = "Service '{}' configuration key {} value {}" + + if msg_format: + return msg_format.format(service_name, config_key, error_msg) + + return error.message + + +def parse_key_from_error_msg(error): + return error.message.split("'")[1] + + +def _parse_valid_types_from_validator(validator): + """A validator value can be either an array of valid types or a string of + a valid type. Parse the valid types and prefix with the correct article. """ - jsonschema gives us an error tree full of information to explain what has + if not isinstance(validator, list): + return anglicize_validator(validator) + + if len(validator) == 1: + return anglicize_validator(validator[0]) + + return "{}, or {}".format( + ", ".join([anglicize_validator(validator[0])] + validator[1:-1]), + anglicize_validator(validator[-1])) + + +def _parse_oneof_validator(error): + """oneOf has multiple schemas, so we need to reason about which schema, sub + schema or constraint the validation is failing on. + Inspecting the context value of a ValidationError gives us information about + which sub schema failed and which kind of error it is. + """ + types = [] + for context in error.context: + + if context.validator == 'required': + return context.message + + if context.validator == 'additionalProperties': + invalid_config_key = parse_key_from_error_msg(context) + return "contains unsupported option: '{}'".format(invalid_config_key) + + if context.path: + invalid_config_key = " ".join( + "'{}' ".format(fragment) for fragment in context.path + if isinstance(fragment, six.string_types) + ) + return "{}contains {}, which is an invalid type, it should be {}".format( + invalid_config_key, + context.instance, + _parse_valid_types_from_validator(context.validator_value)) + + if context.validator == 'uniqueItems': + return "contains non unique items, please remove duplicates from {}".format( + context.instance) + + if context.validator == 'type': + types.append(context.validator_value) + + valid_types = _parse_valid_types_from_validator(types) + return "contains an invalid type, it should be {}".format(valid_types) + + +def process_errors(errors, service_name=None): + """jsonschema gives us an error tree full of information to explain what has gone wrong. Process each error and pull out relevant information and re-write helpful error messages that are relevant. """ - def _parse_key_from_error_msg(error): - return error.message.split("'")[1] + def format_error_message(error, service_name): + if not service_name and error.path: + # field_schema errors will have service name on the path + service_name = error.path.popleft() - def _clean_error_message(message): - return message.replace("u'", "'") + if 'id' in error.schema: + error_msg = handle_error_for_schema_with_id(error, service_name) + if error_msg: + return error_msg - def _parse_valid_types_from_validator(validator): - """ - A validator value can be either an array of valid types or a string of - a valid type. Parse the valid types and prefix with the correct article. - """ - if isinstance(validator, list): - if len(validator) >= 2: - first_type = anglicize_validator(validator[0]) - last_type = anglicize_validator(validator[-1]) - types_from_validator = ", ".join([first_type] + validator[1:-1]) + return handle_generic_service_error(error, service_name) - msg = "{} or {}".format( - types_from_validator, - last_type - ) - else: - msg = "{}".format(anglicize_validator(validator[0])) - else: - msg = "{}".format(anglicize_validator(validator)) - - return msg - - def _parse_oneof_validator(error): - """ - oneOf has multiple schemas, so we need to reason about which schema, sub - schema or constraint the validation is failing on. - Inspecting the context value of a ValidationError gives us information about - which sub schema failed and which kind of error it is. - """ - required = [context for context in error.context if context.validator == 'required'] - if required: - return required[0].message - - additionalProperties = [context for context in error.context if context.validator == 'additionalProperties'] - if additionalProperties: - invalid_config_key = _parse_key_from_error_msg(additionalProperties[0]) - return "contains unsupported option: '{}'".format(invalid_config_key) - - constraint = [context for context in error.context if len(context.path) > 0] - if constraint: - valid_types = _parse_valid_types_from_validator(constraint[0].validator_value) - invalid_config_key = "".join( - "'{}' ".format(fragment) for fragment in constraint[0].path - if isinstance(fragment, six.string_types) - ) - msg = "{}contains {}, which is an invalid type, it should be {}".format( - invalid_config_key, - constraint[0].instance, - valid_types - ) - return msg - - uniqueness = [context for context in error.context if context.validator == 'uniqueItems'] - if uniqueness: - msg = "contains non unique items, please remove duplicates from {}".format( - uniqueness[0].instance - ) - return msg - - types = [context.validator_value for context in error.context if context.validator == 'type'] - valid_types = _parse_valid_types_from_validator(types) - - msg = "contains an invalid type, it should be {}".format(valid_types) - - return msg - - root_msgs = [] - invalid_keys = [] - required = [] - type_errors = [] - other_errors = [] - - for error in errors: - # handle root level errors - if len(error.path) == 0 and not service_name: - if error.validator == 'type': - msg = "Top level object needs to be a dictionary. Check your .yml file that you have defined a service at the top level." - root_msgs.append(msg) - elif error.validator == 'additionalProperties': - invalid_service_name = _parse_key_from_error_msg(error) - msg = "Invalid service name '{}' - only {} characters are allowed".format(invalid_service_name, VALID_NAME_CHARS) - root_msgs.append(msg) - else: - root_msgs.append(_clean_error_message(error.message)) - - else: - if not service_name: - # field_schema errors will have service name on the path - service_name = error.path[0] - error.path.popleft() - else: - # service_schema errors have the service name passed in, as that - # is not available on error.path or necessarily error.instance - service_name = service_name - - if error.validator == 'additionalProperties': - invalid_config_key = _parse_key_from_error_msg(error) - invalid_keys.append(get_unsupported_config_msg(service_name, invalid_config_key)) - elif error.validator == 'anyOf': - if 'image' in error.instance and 'build' in error.instance: - required.append( - "Service '{}' has both an image and build path specified. " - "A service can either be built to image or use an existing " - "image, not both.".format(service_name)) - elif 'image' not in error.instance and 'build' not in error.instance: - required.append( - "Service '{}' has neither an image nor a build path " - "specified. Exactly one must be provided.".format(service_name)) - elif 'image' in error.instance and 'dockerfile' in error.instance: - required.append( - "Service '{}' has both an image and alternate Dockerfile. " - "A service can either be built to image or use an existing " - "image, not both.".format(service_name)) - else: - required.append(_clean_error_message(error.message)) - elif error.validator == 'oneOf': - config_key = error.path[0] - msg = _parse_oneof_validator(error) - - type_errors.append("Service '{}' configuration key '{}' {}".format( - service_name, config_key, msg) - ) - elif error.validator == 'type': - msg = _parse_valid_types_from_validator(error.validator_value) - - if len(error.path) > 0: - config_key = " ".join(["'%s'" % k for k in error.path]) - type_errors.append( - "Service '{}' configuration key {} contains an invalid " - "type, it should be {}".format( - service_name, - config_key, - msg)) - else: - root_msgs.append( - "Service \"{}\" doesn't have any configuration options. " - "All top level keys in your docker-compose.yml must map " - "to a dictionary of configuration options.'".format(service_name)) - elif error.validator == 'required': - config_key = error.path[0] - required.append( - "Service '{}' option '{}' is invalid, {}".format( - service_name, - config_key, - _clean_error_message(error.message))) - elif error.validator == 'dependencies': - dependency_key = list(error.validator_value.keys())[0] - required_keys = ",".join(error.validator_value[dependency_key]) - required.append("Invalid '{}' configuration for '{}' service: when defining '{}' you must set '{}' as well".format( - dependency_key, service_name, dependency_key, required_keys)) - else: - config_key = " ".join(["'%s'" % k for k in error.path]) - err_msg = "Service '{}' configuration key {} value {}".format(service_name, config_key, error.message) - other_errors.append(err_msg) - - return "\n".join(root_msgs + invalid_keys + required + type_errors + other_errors) + return '\n'.join(format_error_message(error, service_name) for error in errors) def validate_against_fields_schema(config): - schema_filename = "fields_schema.json" - format_checkers = ["ports", "environment"] - return _validate_against_schema(config, schema_filename, format_checkers) + return _validate_against_schema( + config, + "fields_schema.json", + ["ports", "environment"]) def validate_against_service_schema(config, service_name): - schema_filename = "service_schema.json" - format_checkers = ["ports"] - return _validate_against_schema(config, schema_filename, format_checkers, service_name) + return _validate_against_schema( + config, + "service_schema.json", + ["ports"], + service_name) -def _validate_against_schema(config, schema_filename, format_checker=[], service_name=None): +def _validate_against_schema( + config, + schema_filename, + format_checker=(), + service_name=None): config_source_dir = os.path.dirname(os.path.abspath(__file__)) if sys.platform == "win32": diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 0e2fb7d72..ada5e9cae 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -865,7 +865,7 @@ class MemoryOptionsTest(unittest.TestCase): a mem_limit """ expected_error_msg = ( - "Invalid 'memswap_limit' configuration for 'foo' service: when " + "Service 'foo' configuration key 'memswap_limit' is invalid: when " "defining 'memswap_limit' you must set 'mem_limit' as well" ) with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): From 96e9b470597594beaade8e1851cbb2b3f5c3b37c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 9 Nov 2015 20:01:20 -0500 Subject: [PATCH 38/40] Inclide the filename in validation errors. Signed-off-by: Daniel Nephin --- compose/config/config.py | 4 +- compose/config/interpolation.py | 7 --- compose/config/validation.py | 60 ++++++++++++------ tests/unit/config/config_test.py | 104 +++++++++++++++---------------- 4 files changed, 95 insertions(+), 80 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 21788551d..2c1fdeb9c 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -229,9 +229,9 @@ def load(config_details): def process_config_file(config_file, service_name=None): - validate_top_level_object(config_file.config) + validate_top_level_object(config_file) processed_config = interpolate_environment_variables(config_file.config) - validate_against_fields_schema(processed_config) + validate_against_fields_schema(processed_config, config_file.filename) if service_name and service_name not in processed_config: raise ConfigurationError( diff --git a/compose/config/interpolation.py b/compose/config/interpolation.py index f8e1da610..ba7e35c1e 100644 --- a/compose/config/interpolation.py +++ b/compose/config/interpolation.py @@ -18,13 +18,6 @@ def interpolate_environment_variables(config): def process_service(service_name, service_dict, mapping): - if not isinstance(service_dict, dict): - raise ConfigurationError( - 'Service "%s" doesn\'t have any configuration options. ' - 'All top level keys in your docker-compose.yml must map ' - 'to a dictionary of configuration options.' % service_name - ) - return dict( (key, interpolate_value(service_name, key, val, mapping)) for (key, val) in service_dict.items() diff --git a/compose/config/validation.py b/compose/config/validation.py index 2928238c3..38866b0f4 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -66,21 +66,38 @@ def format_boolean_in_environment(instance): return True -def validate_service_names(config): - for service_name in config.keys(): +def validate_top_level_service_objects(config_file): + """Perform some high level validation of the service name and value. + + This validation must happen before interpolation, which must happen + before the rest of validation, which is why it's separate from the + rest of the service validation. + """ + for service_name, service_dict in config_file.config.items(): if not isinstance(service_name, six.string_types): raise ConfigurationError( - "Service name: {} needs to be a string, eg '{}'".format( + "In file '{}' service name: {} needs to be a string, eg '{}'".format( + config_file.filename, service_name, service_name)) + if not isinstance(service_dict, dict): + raise ConfigurationError( + "In file '{}' service '{}' doesn\'t have any configuration options. " + "All top level keys in your docker-compose.yml must map " + "to a dictionary of configuration options.".format( + config_file.filename, + service_name)) -def validate_top_level_object(config): - if not isinstance(config, dict): + +def validate_top_level_object(config_file): + if not isinstance(config_file.config, dict): raise ConfigurationError( - "Top level object needs to be a dictionary. Check your .yml file " - "that you have defined a service at the top level.") - validate_service_names(config) + "Top level object in '{}' needs to be an object not '{}'. Check " + "that you have defined a service at the top level.".format( + config_file.filename, + type(config_file.config))) + validate_top_level_service_objects(config_file) def validate_extends_file_path(service_name, extends_options, filename): @@ -252,26 +269,28 @@ def process_errors(errors, service_name=None): return '\n'.join(format_error_message(error, service_name) for error in errors) -def validate_against_fields_schema(config): - return _validate_against_schema( +def validate_against_fields_schema(config, filename): + _validate_against_schema( config, "fields_schema.json", - ["ports", "environment"]) + format_checker=["ports", "environment"], + filename=filename) def validate_against_service_schema(config, service_name): - return _validate_against_schema( + _validate_against_schema( config, "service_schema.json", - ["ports"], - service_name) + format_checker=["ports"], + service_name=service_name) def _validate_against_schema( config, schema_filename, format_checker=(), - service_name=None): + service_name=None, + filename=None): config_source_dir = os.path.dirname(os.path.abspath(__file__)) if sys.platform == "win32": @@ -293,6 +312,11 @@ def _validate_against_schema( format_checker=FormatChecker(format_checker)) errors = [error for error in sorted(validation_output.iter_errors(config), key=str)] - if errors: - error_msg = process_errors(errors, service_name) - raise ConfigurationError("Validation failed, reason(s):\n{}".format(error_msg)) + if not errors: + return + + error_msg = process_errors(errors, service_name) + file_msg = " in file '{}'".format(filename) if filename else '' + raise ConfigurationError("Validation failed{}, reason(s):\n{}".format( + file_msg, + error_msg)) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index ada5e9cae..3038af80d 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -94,6 +94,7 @@ class ConfigTest(unittest.TestCase): config.load(config_details) error_msg = "Unsupported config option for 'web' service: 'name'" assert error_msg in exc.exconly() + assert "Validation failed in file 'filename.yml'" in exc.exconly() def test_load_invalid_service_definition(self): config_details = build_config_details( @@ -102,11 +103,12 @@ class ConfigTest(unittest.TestCase): 'filename.yml') with pytest.raises(ConfigurationError) as exc: config.load(config_details) - error_msg = "Service \"web\" doesn\'t have any configuration options" + error_msg = "service 'web' doesn't have any configuration options" assert error_msg in exc.exconly() def test_config_integer_service_name_raise_validation_error(self): - expected_error_msg = "Service name: 1 needs to be a string, eg '1'" + expected_error_msg = ("In file 'filename.yml' service name: 1 needs to " + "be a string, eg '1'") with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( build_config_details( @@ -156,25 +158,26 @@ class ConfigTest(unittest.TestCase): def test_load_with_multiple_files_and_empty_override(self): base_file = config.ConfigFile( - 'base.yaml', + 'base.yml', {'web': {'image': 'example/web'}}) - override_file = config.ConfigFile('override.yaml', None) + override_file = config.ConfigFile('override.yml', None) details = config.ConfigDetails('.', [base_file, override_file]) with pytest.raises(ConfigurationError) as exc: config.load(details) - assert 'Top level object needs to be a dictionary' in exc.exconly() + error_msg = "Top level object in 'override.yml' needs to be an object" + assert error_msg in exc.exconly() def test_load_with_multiple_files_and_empty_base(self): - base_file = config.ConfigFile('base.yaml', None) + base_file = config.ConfigFile('base.yml', None) override_file = config.ConfigFile( - 'override.yaml', + 'override.yml', {'web': {'image': 'example/web'}}) details = config.ConfigDetails('.', [base_file, override_file]) with pytest.raises(ConfigurationError) as exc: config.load(details) - assert 'Top level object needs to be a dictionary' in exc.exconly() + assert "Top level object in 'base.yml' needs to be an object" in exc.exconly() def test_load_with_multiple_files_and_extends_in_override_file(self): base_file = config.ConfigFile( @@ -225,17 +228,17 @@ class ConfigTest(unittest.TestCase): with pytest.raises(ConfigurationError) as exc: config.load(details) - assert 'Service "bogus" doesn\'t have any configuration' in exc.exconly() + assert "service 'bogus' doesn't have any configuration" in exc.exconly() + assert "In file 'override.yaml'" in exc.exconly() def test_config_valid_service_names(self): for valid_name in ['_', '-', '.__.', '_what-up.', 'what_.up----', 'whatup']: - config.load( + services = config.load( build_config_details( {valid_name: {'image': 'busybox'}}, 'tests/fixtures/extends', - 'common.yml' - ) - ) + 'common.yml')) + assert services[0]['name'] == valid_name def test_config_invalid_ports_format_validation(self): expected_error_msg = "Service 'web' configuration key 'ports' contains an invalid type" @@ -300,7 +303,8 @@ class ConfigTest(unittest.TestCase): ) def test_invalid_config_not_a_dictionary(self): - expected_error_msg = "Top level object needs to be a dictionary." + expected_error_msg = ("Top level object in 'filename.yml' needs to be " + "an object.") with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( build_config_details( @@ -382,12 +386,13 @@ class ConfigTest(unittest.TestCase): ) def test_config_ulimits_invalid_keys_validation_error(self): - expected_error_msg = "Service 'web' configuration key 'ulimits' contains unsupported option: 'not_soft_or_hard'" + expected = ("Service 'web' configuration key 'ulimits' 'nofile' contains " + "unsupported option: 'not_soft_or_hard'") - with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): - config.load( - build_config_details( - {'web': { + with pytest.raises(ConfigurationError) as exc: + config.load(build_config_details( + { + 'web': { 'image': 'busybox', 'ulimits': { 'nofile': { @@ -396,50 +401,43 @@ class ConfigTest(unittest.TestCase): "hard": 20000, } } - }}, - 'working_dir', - 'filename.yml' - ) - ) + } + }, + 'working_dir', + 'filename.yml')) + assert expected in exc.exconly() def test_config_ulimits_required_keys_validation_error(self): - expected_error_msg = "Service 'web' configuration key 'ulimits' u?'hard' is a required property" - with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): - config.load( - build_config_details( - {'web': { + with pytest.raises(ConfigurationError) as exc: + config.load(build_config_details( + { + 'web': { 'image': 'busybox', - 'ulimits': { - 'nofile': { - "soft": 10000, - } - } - }}, - 'working_dir', - 'filename.yml' - ) - ) + 'ulimits': {'nofile': {"soft": 10000}} + } + }, + 'working_dir', + 'filename.yml')) + assert "Service 'web' configuration key 'ulimits' 'nofile'" in exc.exconly() + assert "'hard' is a required property" in exc.exconly() def test_config_ulimits_soft_greater_than_hard_error(self): - expected_error_msg = "cannot contain a 'soft' value higher than 'hard' value" + expected = "cannot contain a 'soft' value higher than 'hard' value" - with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): - config.load( - build_config_details( - {'web': { + with pytest.raises(ConfigurationError) as exc: + config.load(build_config_details( + { + 'web': { 'image': 'busybox', 'ulimits': { - 'nofile': { - "soft": 10000, - "hard": 1000 - } + 'nofile': {"soft": 10000, "hard": 1000} } - }}, - 'working_dir', - 'filename.yml' - ) - ) + } + }, + 'working_dir', + 'filename.yml')) + assert expected in exc.exconly() def test_valid_config_which_allows_two_type_definitions(self): expose_values = [["8000"], [8000]] From 82086a4e92aada343cf656e7c5c1e9f88d13533e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 12 Nov 2015 13:26:13 -0500 Subject: [PATCH 39/40] Remove name field from the list of ALLOWED_KEYS Signed-off-by: Daniel Nephin --- compose/config/config.py | 1 - 1 file changed, 1 deletion(-) diff --git a/compose/config/config.py b/compose/config/config.py index 2c1fdeb9c..201266208 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -65,7 +65,6 @@ ALLOWED_KEYS = DOCKER_CONFIG_KEYS + [ 'dockerfile', 'expose', 'external_links', - 'name', ] From 4628e93fb2ee8e09f9410016574be58a5c2973ed Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 12 Nov 2015 13:23:04 -0500 Subject: [PATCH 40/40] Bump 1.5.1 Signed-off-by: Daniel Nephin --- CHANGELOG.md | 53 +++++++++++++++++++++++++++++++++++++++++++++ compose/__init__.py | 2 +- docs/install.md | 6 ++--- script/run.sh | 2 +- 4 files changed, 58 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a123c2a44..50aabcb8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,59 @@ Change log ========== +1.5.1 (2015-11-12) +------------------ + +- Add the `--force-rm` option to `build`. + +- Add the `ulimit` option for services in the Compose file. + +- Fixed a bug where `up` would error with "service needs to be built" if + a service changed from using `image` to using `build`. + +- Fixed a bug that would cause incorrect output of parallel operations + on some terminals. + +- Fixed a bug that prevented a container from being recreated when the + mode of a `volumes_from` was changed. + +- Fixed a regression in 1.5.0 where non-utf-8 unicode characters would cause + `up` or `logs` to crash. + +- Fixed a regression in 1.5.0 where Compose would use a success exit status + code when a command fails due to an HTTP timeout communicating with the + docker daemon. + +- Fixed a regression in 1.5.0 where `name` was being accepted as a valid + service option which would override the actual name of the service. + +- When using `--x-networking` Compose no longer sets the hostname to the + container name. + +- When using `--x-networking` Compose will only create the default network + if at least one container is using the network. + +- When printings logs during `up` or `logs`, flush the output buffer after + each line to prevent buffering issues from hideing logs. + +- Recreate a container if one of it's dependencies is being created. + Previously a container was only recreated if it's dependencies already + existed, but were being recreated as well. + +- Add a warning when a `volume` in the Compose file is being ignored + and masked by a container volume from a previous container. + +- Improve the output of `pull` when run without a tty. + +- When using multiple Compose files, validate each before attempting to merge + them together. Previously invalid files would result in not helpful errors. + +- Allow dashes in keys in the `environment` service option. + +- Improve validation error messages by including the filename as part of the + error message. + + 1.5.0 (2015-11-03) ------------------ diff --git a/compose/__init__.py b/compose/__init__.py index 2b8d5e72b..5f2b332af 100644 --- a/compose/__init__.py +++ b/compose/__init__.py @@ -1,3 +1,3 @@ from __future__ import unicode_literals -__version__ = '1.5.0' +__version__ = '1.5.1' diff --git a/docs/install.md b/docs/install.md index c5304409c..f8c9db638 100644 --- a/docs/install.md +++ b/docs/install.md @@ -39,7 +39,7 @@ which the release page specifies, in your terminal. The following is an example command illustrating the format: - curl -L https://github.com/docker/compose/releases/download/VERSION_NUM/docker-compose-`uname -s`-`uname -m` > /usr/local/bin/docker-compose + curl -L https://github.com/docker/compose/releases/download/1.5.1/docker-compose-`uname -s`-`uname -m` > /usr/local/bin/docker-compose If you have problems installing with `curl`, see [Alternative Install Options](#alternative-install-options). @@ -54,7 +54,7 @@ which the release page specifies, in your terminal. 7. Test the installation. $ docker-compose --version - docker-compose version: 1.5.0 + docker-compose version: 1.5.1 ## Alternative install options @@ -76,7 +76,7 @@ to get started. Compose can also be run inside a container, from a small bash script wrapper. To install compose as a container run: - $ curl -L https://github.com/docker/compose/releases/download/1.5.0/run.sh > /usr/local/bin/docker-compose + $ curl -L https://github.com/docker/compose/releases/download/1.5.1/run.sh > /usr/local/bin/docker-compose $ chmod +x /usr/local/bin/docker-compose ## Master builds diff --git a/script/run.sh b/script/run.sh index cf46c143c..a9b954774 100755 --- a/script/run.sh +++ b/script/run.sh @@ -15,7 +15,7 @@ set -e -VERSION="1.5.0" +VERSION="1.5.1" IMAGE="docker/compose:$VERSION"