From da36ee7cbcaf2051fc0829f273c01517bd7d9bc2 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 27 Jul 2015 15:15:07 +0100 Subject: [PATCH 01/17] Perform schema validation Define a schema that we can pass to jsonschema to validate against the config a user has supplied. This will help catch a wide variety of common errors that occur. If the config does not pass schema validation then it raises an exception and prints out human readable reasons. Signed-off-by: Mazz Mosley --- compose/config/config.py | 43 ++++--- compose/schema.json | 79 ++++++++++++ compose/service.py | 6 - requirements.txt | 1 + setup.py | 1 + .../fixtures/extends/specify-file-as-self.yml | 1 + tests/unit/config_test.py | 118 +++++++++++------- tests/unit/service_test.py | 1 - 8 files changed, 175 insertions(+), 75 deletions(-) create mode 100644 compose/schema.json diff --git a/compose/config/config.py b/compose/config/config.py index 4d3f5faefad..1e793d9f645 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -3,6 +3,8 @@ import sys import yaml from collections import namedtuple +import json +import jsonschema import six @@ -131,13 +133,31 @@ def get_config_path(base_dir): return os.path.join(path, winner) +def validate_against_schema(config): + config_source_dir = os.path.dirname(os.path.abspath(__file__)) + schema_file = os.path.join(config_source_dir, "schema.json") + + with open(schema_file, "r") as schema_fh: + schema = json.load(schema_fh) + + validation_output = jsonschema.Draft4Validator(schema) + + errors = [error.message for error in sorted(validation_output.iter_errors(config), key=str)] + if errors: + raise ConfigurationError("Validation failed, reason(s): {}".format("\n".join(errors))) + + def load(config_details): - dictionary, working_dir, filename = config_details - dictionary = interpolate_environment_variables(dictionary) + config, working_dir, filename = config_details + config = interpolate_environment_variables(config) service_dicts = [] - for service_name, service_dict in list(dictionary.items()): + validate_against_schema(config) + + for service_name, service_dict in list(config.items()): + 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) loader = ServiceLoader(working_dir=working_dir, filename=filename) service_dict = loader.make_service_dict(service_name, service_dict) validate_paths(service_dict) @@ -210,25 +230,11 @@ def signature(self, name): def validate_extends_options(self, service_name, extends_options): error_prefix = "Invalid 'extends' configuration for %s:" % service_name - if not isinstance(extends_options, dict): - raise ConfigurationError("%s must be a dictionary" % error_prefix) - - if 'service' not in extends_options: - raise ConfigurationError( - "%s you need to specify a service, e.g. 'service: web'" % error_prefix - ) - if 'file' not in extends_options and self.filename is None: raise ConfigurationError( "%s you need to specify a 'file', e.g. 'file: something.yml'" % error_prefix ) - for k, _ in extends_options.items(): - if k not in ['file', 'service']: - raise ConfigurationError( - "%s unsupported configuration option '%s'" % (error_prefix, k) - ) - return extends_options @@ -256,9 +262,6 @@ def process_container_options(service_dict, working_dir=None): service_dict = service_dict.copy() - if 'memswap_limit' in service_dict and 'mem_limit' not in service_dict: - raise ConfigurationError("Invalid 'memswap_limit' configuration for %s service: when defining 'memswap_limit' you must set 'mem_limit' as well" % service_dict['name']) - if 'volumes' in service_dict and service_dict.get('volume_driver') is None: service_dict['volumes'] = resolve_volume_paths(service_dict['volumes'], working_dir=working_dir) diff --git a/compose/schema.json b/compose/schema.json new file mode 100644 index 00000000000..7c7e2d096c7 --- /dev/null +++ b/compose/schema.json @@ -0,0 +1,79 @@ +{ + "$schema": "http://json-schema.org/draft-04/schema#", + + "type": "object", + + "patternProperties": { + "^[a-zA-Z0-9._-]+$": { + "$ref": "#/definitions/service" + } + }, + + "definitions": { + "service": { + "type": "object", + + "properties": { + "build": {"type": "string"}, + "env_file": {"$ref": "#/definitions/string_or_list"}, + "environment": { + "oneOf": [ + {"type": "object"}, + {"type": "array", "items": {"type": "string"}, "uniqueItems": true} + ] + }, + "image": {"type": "string"}, + "mem_limit": {"type": "number"}, + "memswap_limit": {"type": "number"}, + + "extends": { + "type": "object", + + "properties": { + "service": {"type": "string"}, + "file": {"type": "string"} + }, + "required": ["service"], + "additionalProperties": false + } + + }, + + "anyOf": [ + { + "required": ["build"], + "not": {"required": ["image"]} + }, + { + "required": ["image"], + "not": {"required": ["build"]} + }, + { + "required": ["extends"], + "not": {"required": ["build", "image"]} + } + ], + + "dependencies": { + "memswap_limit": ["mem_limit"] + } + + }, + + "string_or_list": { + "oneOf": [ + {"type": "string"}, + {"$ref": "#/definitions/list_of_strings"} + ] + }, + + "list_of_strings": { + "type": "array", + "items": {"type": "string"}, + "uniqueItems": true + } + + }, + + "additionalProperties": false +} diff --git a/compose/service.py b/compose/service.py index 2e0490a5086..c72365cf99c 100644 --- a/compose/service.py +++ b/compose/service.py @@ -82,14 +82,8 @@ class NoSuchImageError(Exception): class Service(object): def __init__(self, name, client=None, project='default', links=None, external_links=None, volumes_from=None, net=None, **options): - if not re.match('^%s+$' % VALID_NAME_CHARS, name): - raise ConfigError('Invalid service name "%s" - only %s are allowed' % (name, VALID_NAME_CHARS)) if not re.match('^%s+$' % VALID_NAME_CHARS, project): raise ConfigError('Invalid project name "%s" - only %s are allowed' % (project, VALID_NAME_CHARS)) - if 'image' in options and 'build' in options: - raise ConfigError('Service %s has both an image and build path specified. A service can either be built to image or use an existing image, not both.' % name) - if 'image' not in options and 'build' not in options: - raise ConfigError('Service %s has neither an image nor a build path specified. Exactly one must be provided.' % name) self.name = name self.client = client diff --git a/requirements.txt b/requirements.txt index f9cec8372c7..64168768615 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,5 @@ PyYAML==3.10 +jsonschema==2.5.1 docker-py==1.3.1 dockerpty==0.3.4 docopt==0.6.1 diff --git a/setup.py b/setup.py index 9bca4752de4..1f9c981d1b0 100644 --- a/setup.py +++ b/setup.py @@ -33,6 +33,7 @@ def find_version(*file_paths): 'docker-py >= 1.3.1, < 1.4', 'dockerpty >= 0.3.4, < 0.4', 'six >= 1.3.0, < 2', + 'jsonschema >= 2.5.1, < 3', ] diff --git a/tests/fixtures/extends/specify-file-as-self.yml b/tests/fixtures/extends/specify-file-as-self.yml index 7e249976235..c24f10bc92b 100644 --- a/tests/fixtures/extends/specify-file-as-self.yml +++ b/tests/fixtures/extends/specify-file-as-self.yml @@ -12,5 +12,6 @@ web: environment: - "BAZ=3" otherweb: + image: busybox environment: - "YEP=1" diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index 0046202030c..3ed394a92a6 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -20,10 +20,10 @@ def test_load(self): config.ConfigDetails( { 'foo': {'image': 'busybox'}, - 'bar': {'environment': ['FOO=1']}, + 'bar': {'image': 'busybox', 'environment': ['FOO=1']}, }, - 'working_dir', - 'filename.yml' + 'tests/fixtures/extends', + 'common.yml' ) ) @@ -32,13 +32,14 @@ def test_load(self): sorted([ { 'name': 'bar', + 'image': 'busybox', 'environment': {'FOO': '1'}, }, { 'name': 'foo', 'image': 'busybox', } - ]) + ], key=lambda d: d['name']) ) def test_load_throws_error_when_not_dict(self): @@ -327,23 +328,26 @@ def test_validation_fails_with_just_memswap_limit(self): When you set a 'memswap_limit' it is invalid config unless you also set a mem_limit """ - with self.assertRaises(config.ConfigurationError): - make_service_dict( - 'foo', { - 'memswap_limit': 2000000, - }, - 'tests/' + with self.assertRaisesRegexp(config.ConfigurationError, "u'mem_limit' is a dependency of u'memswap_limit'"): + config.load( + config.ConfigDetails( + { + 'foo': {'image': 'busybox', 'memswap_limit': 2000000}, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) ) def test_validation_with_correct_memswap_values(self): - service_dict = make_service_dict( - 'foo', { - 'mem_limit': 1000000, - 'memswap_limit': 2000000, - }, - 'tests/' + service_dict = config.load( + config.ConfigDetails( + {'foo': {'image': 'busybox', 'mem_limit': 1000000, 'memswap_limit': 2000000}}, + 'tests/fixtures/extends', + 'common.yml' + ) ) - self.assertEqual(service_dict['memswap_limit'], 2000000) + self.assertEqual(service_dict[0]['memswap_limit'], 2000000) class EnvTest(unittest.TestCase): @@ -528,6 +532,7 @@ def test_self_referencing_file(self): { 'environment': {'YEP': '1'}, + 'image': 'busybox', 'name': 'otherweb' }, { @@ -553,36 +558,47 @@ def test_circular(self): ) def test_extends_validation_empty_dictionary(self): - dictionary = {'extends': None} - - def load_config(): - return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends') - - self.assertRaisesRegexp(config.ConfigurationError, 'dictionary', load_config) - - dictionary['extends'] = {} - self.assertRaises(config.ConfigurationError, load_config) + with self.assertRaisesRegexp(config.ConfigurationError, 'service'): + config.load( + config.ConfigDetails( + { + 'web': {'image': 'busybox', 'extends': {}}, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) def test_extends_validation_missing_service_key(self): - dictionary = {'extends': {'file': 'common.yml'}} - - def load_config(): - return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends') - - self.assertRaisesRegexp(config.ConfigurationError, 'service', load_config) + with self.assertRaisesRegexp(config.ConfigurationError, "u'service' is a required property"): + config.load( + config.ConfigDetails( + { + 'web': {'image': 'busybox', 'extends': {'file': 'common.yml'}}, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) def test_extends_validation_invalid_key(self): - dictionary = { - 'extends': - { - 'service': 'web', 'file': 'common.yml', 'what': 'is this' - } - } - - def load_config(): - return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends') - - self.assertRaisesRegexp(config.ConfigurationError, 'what', load_config) + with self.assertRaisesRegexp(config.ConfigurationError, "'rogue_key' was unexpected"): + config.load( + config.ConfigDetails( + { + 'web': { + 'image': 'busybox', + 'extends': { + 'file': 'common.yml', + 'service': 'web', + 'rogue_key': 'is not allowed' + } + }, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) def test_extends_validation_no_file_key_no_filename_set(self): dictionary = {'extends': {'service': 'web'}} @@ -593,12 +609,18 @@ def load_config(): self.assertRaisesRegexp(config.ConfigurationError, 'file', load_config) def test_extends_validation_valid_config(self): - dictionary = {'extends': {'service': 'web', 'file': 'common.yml'}} - - def load_config(): - return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends') + service = config.load( + config.ConfigDetails( + { + 'web': {'image': 'busybox', 'extends': {'service': 'web', 'file': 'common.yml'}}, + }, + 'tests/fixtures/extends', + 'common.yml' + ) + ) - self.assertIsInstance(load_config(), dict) + self.assertEquals(len(service), 1) + self.assertIsInstance(service[0], dict) def test_extends_file_defaults_to_self(self): """ diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index bc6b9e485e4..aa348466a40 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -49,7 +49,6 @@ def test_name_validations(self): Service('.__.', image='foo') def test_project_validation(self): - self.assertRaises(ConfigError, lambda: Service('bar')) self.assertRaises(ConfigError, lambda: Service(name='foo', project='>', image='foo')) Service(name='foo', project='bar.bar__', image='foo') From 76e6029f2132cfd531d069e57bb2e33060e84eb5 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Wed, 29 Jul 2015 16:09:33 +0100 Subject: [PATCH 02/17] Replace service tests with config tests We validate the config against our schema before a service is created so checking whether a service name is valid at time of instantiation of the Service class is not needed. Signed-off-by: Mazz Mosley --- tests/unit/config_test.py | 21 +++++++++++++++++++++ tests/unit/service_test.py | 19 ------------------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index 3ed394a92a6..f06cbab637e 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -59,6 +59,27 @@ def test_config_validation(self): ) make_service_dict('foo', {'ports': ['8000']}, 'tests/') + def test_config_invalid_service_names(self): + with self.assertRaises(config.ConfigurationError): + for invalid_name in ['?not?allowed', ' ', '', '!', '/', '\xe2']: + config.load( + config.ConfigDetails( + {invalid_name: {'image': 'busybox'}}, + 'working_dir', + 'filename.yml' + ) + ) + + def test_config_valid_service_names(self): + for valid_name in ['_', '-', '.__.', '_what-up.', 'what_.up----', 'whatup']: + config.load( + config.ConfigDetails( + {valid_name: {'image': 'busybox'}}, + 'tests/fixtures/extends', + 'common.yml' + ) + ) + class InterpolationTest(unittest.TestCase): @mock.patch.dict(os.environ) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index aa348466a40..a99197e63cf 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -29,25 +29,6 @@ class ServiceTest(unittest.TestCase): def setUp(self): self.mock_client = mock.create_autospec(docker.Client) - def test_name_validations(self): - self.assertRaises(ConfigError, lambda: Service(name='', image='foo')) - - self.assertRaises(ConfigError, lambda: Service(name=' ', image='foo')) - self.assertRaises(ConfigError, lambda: Service(name='/', image='foo')) - self.assertRaises(ConfigError, lambda: Service(name='!', image='foo')) - self.assertRaises(ConfigError, lambda: Service(name='\xe2', image='foo')) - - Service('a', image='foo') - Service('foo', image='foo') - Service('foo-bar', image='foo') - Service('foo.bar', image='foo') - Service('foo_bar', image='foo') - Service('_', image='foo') - Service('___', image='foo') - Service('-', image='foo') - Service('--', image='foo') - Service('.__.', image='foo') - def test_project_validation(self): self.assertRaises(ConfigError, lambda: Service(name='foo', project='>', image='foo')) From 6c7c5985465d63a70e579ed3e253ca8d0f5d4b06 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Wed, 29 Jul 2015 16:37:11 +0100 Subject: [PATCH 03/17] Format validation of ports Signed-off-by: Mazz Mosley --- compose/config/config.py | 26 ++++++++++++++++++++++++-- compose/schema.json | 11 +++++++++++ tests/unit/config_test.py | 22 ++++++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 1e793d9f645..6cffa2fe880 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -4,7 +4,7 @@ import yaml from collections import namedtuple import json -import jsonschema +from jsonschema import Draft4Validator, FormatChecker, ValidationError import six @@ -133,6 +133,28 @@ def get_config_path(base_dir): return os.path.join(path, winner) +@FormatChecker.cls_checks(format="ports", raises=ValidationError("Ports is incorrectly formatted.")) +def format_ports(instance): + def _is_valid(port): + if ':' in port or '/' in port: + return True + try: + int(port) + return True + except ValueError: + return False + return False + + if isinstance(instance, list): + for port in instance: + if not _is_valid(port): + return False + return True + elif isinstance(instance, str): + return _is_valid(instance) + return False + + def validate_against_schema(config): config_source_dir = os.path.dirname(os.path.abspath(__file__)) schema_file = os.path.join(config_source_dir, "schema.json") @@ -140,7 +162,7 @@ def validate_against_schema(config): with open(schema_file, "r") as schema_fh: schema = json.load(schema_fh) - validation_output = jsonschema.Draft4Validator(schema) + validation_output = Draft4Validator(schema, format_checker=FormatChecker(["ports"])) errors = [error.message for error in sorted(validation_output.iter_errors(config), key=str)] if errors: diff --git a/compose/schema.json b/compose/schema.json index 7c7e2d096c7..bf43ca36b06 100644 --- a/compose/schema.json +++ b/compose/schema.json @@ -14,6 +14,17 @@ "type": "object", "properties": { + "ports": { + "oneOf": [ + {"type": "string", "format": "ports"}, + { + "type": "array", + "items": {"type": "string"}, + "uniqueItems": true, + "format": "ports" + } + ] + }, "build": {"type": "string"}, "env_file": {"$ref": "#/definitions/string_or_list"}, "environment": { diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index f06cbab637e..f7e949d3cb7 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -80,6 +80,28 @@ def test_config_valid_service_names(self): ) ) + def test_config_invalid_ports_format_validation(self): + with self.assertRaises(config.ConfigurationError): + for invalid_ports in [{"1": "8000"}, "whatport"]: + config.load( + config.ConfigDetails( + {'web': {'image': 'busybox', 'ports': invalid_ports}}, + 'working_dir', + 'filename.yml' + ) + ) + + def test_config_valid_ports_format_validation(self): + valid_ports = [["8000", "9000"], "625", "8000:8050", ["8000/8050"]] + for ports in valid_ports: + config.load( + config.ConfigDetails( + {'web': {'image': 'busybox', 'ports': ports}}, + 'working_dir', + 'filename.yml' + ) + ) + class InterpolationTest(unittest.TestCase): @mock.patch.dict(os.environ) From 98c7a7da6110e72540810e888eec9d42c8172f9d Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Thu, 30 Jul 2015 10:53:41 +0100 Subject: [PATCH 04/17] Order properties alphabetically Improves readability. Signed-off-by: Mazz Mosley --- compose/schema.json | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/compose/schema.json b/compose/schema.json index bf43ca36b06..3e719fc4248 100644 --- a/compose/schema.json +++ b/compose/schema.json @@ -14,28 +14,15 @@ "type": "object", "properties": { - "ports": { - "oneOf": [ - {"type": "string", "format": "ports"}, - { - "type": "array", - "items": {"type": "string"}, - "uniqueItems": true, - "format": "ports" - } - ] - }, "build": {"type": "string"}, "env_file": {"$ref": "#/definitions/string_or_list"}, + "environment": { "oneOf": [ {"type": "object"}, {"type": "array", "items": {"type": "string"}, "uniqueItems": true} ] }, - "image": {"type": "string"}, - "mem_limit": {"type": "number"}, - "memswap_limit": {"type": "number"}, "extends": { "type": "object", @@ -46,6 +33,22 @@ }, "required": ["service"], "additionalProperties": false + }, + + "image": {"type": "string"}, + "mem_limit": {"type": "number"}, + "memswap_limit": {"type": "number"}, + + "ports": { + "oneOf": [ + {"type": "string", "format": "ports"}, + { + "type": "array", + "items": {"type": "string"}, + "uniqueItems": true, + "format": "ports" + } + ] } }, From 8d6694085d8e9a80223b6473e1f9a1939b3ef936 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Thu, 30 Jul 2015 17:11:28 +0100 Subject: [PATCH 05/17] Include remaining valid config properties Signed-off-by: Mazz Mosley --- compose/config/config.py | 5 ---- compose/schema.json | 59 +++++++++++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 6cffa2fe880..27f845b75ba 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -525,11 +525,6 @@ def parse_labels(labels): if isinstance(labels, dict): return labels - raise ConfigurationError( - "labels \"%s\" must be a list or mapping" % - labels - ) - def split_label(label): if '=' in label: diff --git a/compose/schema.json b/compose/schema.json index 3e719fc4248..258f44ccac0 100644 --- a/compose/schema.json +++ b/compose/schema.json @@ -15,6 +15,19 @@ "properties": { "build": {"type": "string"}, + "cap_add": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + "cap_drop": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + "command": {"$ref": "#/definitions/string_or_list"}, + "container_name": {"type": "string"}, + "cpu_shares": {"type": "string"}, + "cpuset": {"type": "string"}, + "detach": {"type": "boolean"}, + "devices": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + "dns": {"$ref": "#/definitions/string_or_list"}, + "dns_search": {"$ref": "#/definitions/string_or_list"}, + "dockerfile": {"type": "string"}, + "domainname": {"type": "string"}, + "entrypoint": {"type": "string"}, "env_file": {"$ref": "#/definitions/string_or_list"}, "environment": { @@ -24,6 +37,8 @@ ] }, + "expose": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + "extends": { "type": "object", @@ -35,9 +50,29 @@ "additionalProperties": false }, + "extra_hosts": {"$ref": "#/definitions/list_or_dict"}, + "external_links": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + "hostname": {"type": "string"}, "image": {"type": "string"}, + "labels": {"$ref": "#/definitions/list_or_dict"}, + "links": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + "log_driver": {"type": "string"}, + + "log_opt": { + "type": "object", + + "properties": { + "address": {"type": "string"} + }, + "required": ["address"] + }, + + "mac_address": {"type": "string"}, "mem_limit": {"type": "number"}, "memswap_limit": {"type": "number"}, + "name": {"type": "string"}, + "net": {"type": "string"}, + "pid": {"type": "string"}, "ports": { "oneOf": [ @@ -49,8 +84,18 @@ "format": "ports" } ] - } + }, + "privileged": {"type": "string"}, + "read_only": {"type": "boolean"}, + "restart": {"type": "string"}, + "security_opt": {"type": "string"}, + "stdin_open": {"type": "string"}, + "tty": {"type": "string"}, + "user": {"type": "string"}, + "volumes": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + "volumes_from": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + "working_dir": {"type": "string"} }, "anyOf": [ @@ -70,8 +115,8 @@ "dependencies": { "memswap_limit": ["mem_limit"] - } - + }, + "additionalProperties": false }, "string_or_list": { @@ -85,9 +130,15 @@ "type": "array", "items": {"type": "string"}, "uniqueItems": true + }, + + "list_or_dict": { + "oneOf": [ + {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + {"type": "object"} + ] } }, - "additionalProperties": false } From d8aee782c876e1e6aa1d31ebaaf4fe566018fc26 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 4 Aug 2015 17:43:33 +0100 Subject: [PATCH 06/17] Error handling jsonschema provides a rich error tree of info, by parsing each error we can pull out relevant info and re-write the error messages. This covers current error handling behaviour. This includes new error handling behaviour for types and formatting of the ports field. Signed-off-by: Mazz Mosley --- compose/config/config.py | 78 ++++++++++++++++++++++++++++++++++++++- compose/service.py | 4 +- tests/unit/config_test.py | 6 ++- 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 27f845b75ba..f2a89699d95 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -18,6 +18,8 @@ ) +VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]' + DOCKER_CONFIG_KEYS = [ 'cap_add', 'cap_drop', @@ -155,6 +157,77 @@ def _is_valid(port): return False +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: + msg += " (did you mean '{}'?)".format(DOCKER_CONFIG_HINTS[error_key]) + return msg + + +def process_errors(errors): + """ + 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] + + root_msgs = [] + invalid_keys = [] + required = [] + type_errors = [] + + for error in errors: + # handle root level errors + if len(error.path) == 0: + 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(error.message) + + else: + # handle service level errors + service_name = error.path[0] + + 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)) + else: + required.append(error.message) + elif error.validator == 'type': + msg = "a" + if error.validator_value == "array": + msg = "an" + + try: + config_key = error.path[1] + type_errors.append("Service '{}' has an invalid value for '{}', it should be {} {}".format(service_name, config_key, msg, error.validator_value)) + except IndexError: + config_key = error.path[0] + 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(config_key)) + elif error.validator == 'required': + config_key = error.path[1] + required.append("Service '{}' option '{}' is invalid, {}".format(service_name, config_key, error.message)) + elif error.validator == 'dependencies': + dependency_key = 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)) + + return "\n".join(root_msgs + invalid_keys + required + type_errors) + + def validate_against_schema(config): config_source_dir = os.path.dirname(os.path.abspath(__file__)) schema_file = os.path.join(config_source_dir, "schema.json") @@ -164,9 +237,10 @@ def validate_against_schema(config): validation_output = Draft4Validator(schema, format_checker=FormatChecker(["ports"])) - errors = [error.message for error in sorted(validation_output.iter_errors(config), key=str)] + errors = [error for error in sorted(validation_output.iter_errors(config), key=str)] if errors: - raise ConfigurationError("Validation failed, reason(s): {}".format("\n".join(errors))) + error_msg = process_errors(errors) + raise ConfigurationError("Validation failed, reason(s):\n{}".format(error_msg)) def load(config_details): diff --git a/compose/service.py b/compose/service.py index c72365cf99c..103840c3be2 100644 --- a/compose/service.py +++ b/compose/service.py @@ -12,7 +12,7 @@ from docker.utils import create_host_config, LogConfig from . import __version__ -from .config import DOCKER_CONFIG_KEYS, merge_environment +from .config import DOCKER_CONFIG_KEYS, merge_environment, VALID_NAME_CHARS from .const import ( DEFAULT_TIMEOUT, LABEL_CONTAINER_NUMBER, @@ -49,8 +49,6 @@ 'security_opt', ] -VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]' - class BuildError(Exception): def __init__(self, service, reason): diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index f7e949d3cb7..c0ccead8ab3 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -371,7 +371,8 @@ def test_validation_fails_with_just_memswap_limit(self): When you set a 'memswap_limit' it is invalid config unless you also set a mem_limit """ - with self.assertRaisesRegexp(config.ConfigurationError, "u'mem_limit' is a dependency of u'memswap_limit'"): + expected_error_msg = "Invalid 'memswap_limit' configuration for 'foo' service: when defining 'memswap_limit' you must set 'mem_limit' as well" + with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( { @@ -625,7 +626,8 @@ def test_extends_validation_missing_service_key(self): ) def test_extends_validation_invalid_key(self): - with self.assertRaisesRegexp(config.ConfigurationError, "'rogue_key' was unexpected"): + expected_error_msg = "Unsupported config option for 'web' service: 'rogue_key'" + with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( { From ea3608e1f4c5894ebbdc21fddeab4746deda05d8 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Wed, 5 Aug 2015 12:33:28 +0100 Subject: [PATCH 07/17] Improve test coverage for validation Signed-off-by: Mazz Mosley --- tests/unit/config_test.py | 50 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index c0ccead8ab3..15657f878f9 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -102,6 +102,56 @@ def test_config_valid_ports_format_validation(self): ) ) + def test_config_hint(self): + expected_error_msg = "(did you mean 'privileged'?)" + with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + config.load( + config.ConfigDetails( + { + 'foo': {'image': 'busybox', 'privilige': 'something'}, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) + + def test_invalid_config_build_and_image_specified(self): + expected_error_msg = "Service 'foo' has both an image and build path specified." + with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + config.load( + config.ConfigDetails( + { + 'foo': {'image': 'busybox', 'build': '.'}, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) + + def test_invalid_config_type_should_be_an_array(self): + expected_error_msg = "Service 'foo' has an invalid value for 'links', it should be an array" + with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + config.load( + config.ConfigDetails( + { + 'foo': {'image': 'busybox', 'links': 'an_link'}, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) + + def test_invalid_config_not_a_dictionary(self): + expected_error_msg = "Top level object needs to be a dictionary." + with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + config.load( + config.ConfigDetails( + ['foo', 'lol'], + 'tests/fixtures/extends', + 'filename.yml' + ) + ) + class InterpolationTest(unittest.TestCase): @mock.patch.dict(os.environ) From 0557b5dce6cbebe7bc24f415f4138d487524319b Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Wed, 5 Aug 2015 15:28:05 +0100 Subject: [PATCH 08/17] Remove dead code These functions weren't being called by anything. Signed-off-by: Mazz Mosley --- compose/config/config.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index f2a89699d95..31e5e9166cd 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -252,8 +252,6 @@ def load(config_details): validate_against_schema(config) for service_name, service_dict in list(config.items()): - 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) loader = ServiceLoader(working_dir=working_dir, filename=filename) service_dict = loader.make_service_dict(service_name, service_dict) validate_paths(service_dict) @@ -427,18 +425,6 @@ def merge_environment(base, override): return env -def parse_links(links): - return dict(parse_link(l) for l in links) - - -def parse_link(link): - if ':' in link: - source, alias = link.split(':', 1) - return (alias, source) - else: - return (link, link) - - def get_env_files(options, working_dir=None): if 'env_file' not in options: return {} From 2e428f94ca3e0333a5b8b6469cb6fd528041cbe7 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Thu, 6 Aug 2015 16:56:45 +0100 Subject: [PATCH 09/17] Refactor validation out Move validation out into its own file without causing circular import errors. Fix some of the tests to import from the right place. Also fix tests that were not using valid test data, as the validation schema is now firing telling you that you couldn't "just" have this dict without a build/image config key. Signed-off-by: Mazz Mosley --- compose/config/config.py | 145 ++----------------------------- compose/{ => config}/schema.json | 0 compose/config/validation.py | 134 ++++++++++++++++++++++++++++ compose/service.py | 3 +- tests/unit/config_test.py | 50 +++++------ 5 files changed, 165 insertions(+), 167 deletions(-) rename compose/{ => config}/schema.json (100%) create mode 100644 compose/config/validation.py diff --git a/compose/config/config.py b/compose/config/config.py index 31e5e9166cd..c1cfdb73d60 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -3,9 +3,6 @@ import sys import yaml from collections import namedtuple -import json -from jsonschema import Draft4Validator, FormatChecker, ValidationError - import six from compose.cli.utils import find_candidates_in_parent_dirs @@ -16,10 +13,9 @@ CircularReference, ComposeFileNotFound, ) +from .validation import validate_against_schema -VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]' - DOCKER_CONFIG_KEYS = [ 'cap_add', 'cap_drop', @@ -69,22 +65,6 @@ 'name', ] -DOCKER_CONFIG_HINTS = { - 'cpu_share': 'cpu_shares', - 'add_host': 'extra_hosts', - 'hosts': 'extra_hosts', - 'extra_host': 'extra_hosts', - 'device': 'devices', - 'link': 'links', - 'memory_swap': 'memswap_limit', - 'port': 'ports', - 'privilege': 'privileged', - 'priviliged': 'privileged', - 'privilige': 'privileged', - 'volume': 'volumes', - 'workdir': 'working_dir', -} - SUPPORTED_FILENAMES = [ 'docker-compose.yml', @@ -135,122 +115,18 @@ def get_config_path(base_dir): return os.path.join(path, winner) -@FormatChecker.cls_checks(format="ports", raises=ValidationError("Ports is incorrectly formatted.")) -def format_ports(instance): - def _is_valid(port): - if ':' in port or '/' in port: - return True - try: - int(port) - return True - except ValueError: - return False - return False - - if isinstance(instance, list): - for port in instance: - if not _is_valid(port): - return False - return True - elif isinstance(instance, str): - return _is_valid(instance) - return False - - -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: - msg += " (did you mean '{}'?)".format(DOCKER_CONFIG_HINTS[error_key]) - return msg - - -def process_errors(errors): - """ - 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] - - root_msgs = [] - invalid_keys = [] - required = [] - type_errors = [] - - for error in errors: - # handle root level errors - if len(error.path) == 0: - 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(error.message) - - else: - # handle service level errors - service_name = error.path[0] - - 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)) - else: - required.append(error.message) - elif error.validator == 'type': - msg = "a" - if error.validator_value == "array": - msg = "an" - - try: - config_key = error.path[1] - type_errors.append("Service '{}' has an invalid value for '{}', it should be {} {}".format(service_name, config_key, msg, error.validator_value)) - except IndexError: - config_key = error.path[0] - 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(config_key)) - elif error.validator == 'required': - config_key = error.path[1] - required.append("Service '{}' option '{}' is invalid, {}".format(service_name, config_key, error.message)) - elif error.validator == 'dependencies': - dependency_key = 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)) - - return "\n".join(root_msgs + invalid_keys + required + type_errors) - - -def validate_against_schema(config): - config_source_dir = os.path.dirname(os.path.abspath(__file__)) - schema_file = os.path.join(config_source_dir, "schema.json") - - with open(schema_file, "r") as schema_fh: - schema = json.load(schema_fh) - - validation_output = Draft4Validator(schema, format_checker=FormatChecker(["ports"])) - - errors = [error for error in sorted(validation_output.iter_errors(config), key=str)] - if errors: - error_msg = process_errors(errors) - raise ConfigurationError("Validation failed, reason(s):\n{}".format(error_msg)) - - def load(config_details): config, working_dir, filename = config_details + if not isinstance(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." + ) + config = interpolate_environment_variables(config) + validate_against_schema(config) service_dicts = [] - validate_against_schema(config) - for service_name, service_dict in list(config.items()): loader = ServiceLoader(working_dir=working_dir, filename=filename) service_dict = loader.make_service_dict(service_name, service_dict) @@ -347,13 +223,6 @@ def validate_extended_service_dict(service_dict, filename, service): def process_container_options(service_dict, working_dir=None): - for k in service_dict: - if k not in ALLOWED_KEYS: - msg = "Unsupported config option for %s service: '%s'" % (service_dict['name'], k) - if k in DOCKER_CONFIG_HINTS: - msg += " (did you mean '%s'?)" % DOCKER_CONFIG_HINTS[k] - raise ConfigurationError(msg) - service_dict = service_dict.copy() if 'volumes' in service_dict and service_dict.get('volume_driver') is None: diff --git a/compose/schema.json b/compose/config/schema.json similarity index 100% rename from compose/schema.json rename to compose/config/schema.json diff --git a/compose/config/validation.py b/compose/config/validation.py new file mode 100644 index 00000000000..ba5803decb4 --- /dev/null +++ b/compose/config/validation.py @@ -0,0 +1,134 @@ +import os + +import json +from jsonschema import Draft4Validator, FormatChecker, ValidationError + +from .errors import ConfigurationError + + +DOCKER_CONFIG_HINTS = { + 'cpu_share': 'cpu_shares', + 'add_host': 'extra_hosts', + 'hosts': 'extra_hosts', + 'extra_host': 'extra_hosts', + 'device': 'devices', + 'link': 'links', + 'memory_swap': 'memswap_limit', + 'port': 'ports', + 'privilege': 'privileged', + 'priviliged': 'privileged', + 'privilige': 'privileged', + 'volume': 'volumes', + 'workdir': 'working_dir', +} + + +VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]' + + +@FormatChecker.cls_checks(format="ports", raises=ValidationError("Ports is incorrectly formatted.")) +def format_ports(instance): + def _is_valid(port): + if ':' in port or '/' in port: + return True + try: + int(port) + return True + except ValueError: + return False + return False + + if isinstance(instance, list): + for port in instance: + if not _is_valid(port): + return False + return True + elif isinstance(instance, str): + return _is_valid(instance) + return False + + +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: + msg += " (did you mean '{}'?)".format(DOCKER_CONFIG_HINTS[error_key]) + return msg + + +def process_errors(errors): + """ + 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] + + root_msgs = [] + invalid_keys = [] + required = [] + type_errors = [] + + for error in errors: + # handle root level errors + if len(error.path) == 0: + 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(error.message) + + else: + # handle service level errors + service_name = error.path[0] + + 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)) + else: + required.append(error.message) + elif error.validator == 'type': + msg = "a" + if error.validator_value == "array": + msg = "an" + + try: + config_key = error.path[1] + type_errors.append("Service '{}' has an invalid value for '{}', it should be {} {}".format(service_name, config_key, msg, error.validator_value)) + except IndexError: + config_key = error.path[0] + 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(config_key)) + elif error.validator == 'required': + config_key = error.path[1] + required.append("Service '{}' option '{}' is invalid, {}".format(service_name, config_key, error.message)) + elif error.validator == 'dependencies': + dependency_key = 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)) + + return "\n".join(root_msgs + invalid_keys + required + type_errors) + + +def validate_against_schema(config): + config_source_dir = os.path.dirname(os.path.abspath(__file__)) + schema_file = os.path.join(config_source_dir, "schema.json") + + with open(schema_file, "r") as schema_fh: + schema = json.load(schema_fh) + + validation_output = Draft4Validator(schema, format_checker=FormatChecker(["ports"])) + + errors = [error for error in sorted(validation_output.iter_errors(config), key=str)] + if errors: + error_msg = process_errors(errors) + raise ConfigurationError("Validation failed, reason(s):\n{}".format(error_msg)) diff --git a/compose/service.py b/compose/service.py index 103840c3be2..9b5e5928b93 100644 --- a/compose/service.py +++ b/compose/service.py @@ -12,7 +12,7 @@ from docker.utils import create_host_config, LogConfig from . import __version__ -from .config import DOCKER_CONFIG_KEYS, merge_environment, VALID_NAME_CHARS +from .config import DOCKER_CONFIG_KEYS, merge_environment from .const import ( DEFAULT_TIMEOUT, LABEL_CONTAINER_NUMBER, @@ -26,6 +26,7 @@ from .legacy import check_for_legacy_containers from .progress_stream import stream_output, StreamOutputError from .utils import json_hash, parallel_execute +from .config.validation import VALID_NAME_CHARS log = logging.getLogger(__name__) diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index 15657f878f9..9f690f111ea 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -5,6 +5,7 @@ from .. import unittest from compose.config import config +from compose.config.errors import ConfigurationError def make_service_dict(name, service_dict, working_dir): @@ -43,7 +44,7 @@ def test_load(self): ) def test_load_throws_error_when_not_dict(self): - with self.assertRaises(config.ConfigurationError): + with self.assertRaises(ConfigurationError): config.load( config.ConfigDetails( {'web': 'busybox:latest'}, @@ -52,15 +53,8 @@ def test_load_throws_error_when_not_dict(self): ) ) - def test_config_validation(self): - self.assertRaises( - config.ConfigurationError, - lambda: make_service_dict('foo', {'port': ['8000']}, 'tests/') - ) - make_service_dict('foo', {'ports': ['8000']}, 'tests/') - def test_config_invalid_service_names(self): - with self.assertRaises(config.ConfigurationError): + with self.assertRaises(ConfigurationError): for invalid_name in ['?not?allowed', ' ', '', '!', '/', '\xe2']: config.load( config.ConfigDetails( @@ -81,7 +75,7 @@ def test_config_valid_service_names(self): ) def test_config_invalid_ports_format_validation(self): - with self.assertRaises(config.ConfigurationError): + with self.assertRaises(ConfigurationError): for invalid_ports in [{"1": "8000"}, "whatport"]: config.load( config.ConfigDetails( @@ -104,7 +98,7 @@ def test_config_valid_ports_format_validation(self): def test_config_hint(self): expected_error_msg = "(did you mean 'privileged'?)" - with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( { @@ -117,7 +111,7 @@ def test_config_hint(self): def test_invalid_config_build_and_image_specified(self): expected_error_msg = "Service 'foo' has both an image and build path specified." - with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( { @@ -130,7 +124,7 @@ def test_invalid_config_build_and_image_specified(self): def test_invalid_config_type_should_be_an_array(self): expected_error_msg = "Service 'foo' has an invalid value for 'links', it should be an array" - with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( { @@ -143,7 +137,7 @@ def test_invalid_config_type_should_be_an_array(self): def test_invalid_config_not_a_dictionary(self): expected_error_msg = "Top level object needs to be a dictionary." - with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( ['foo', 'lol'], @@ -198,7 +192,7 @@ def test_volume_binding_with_environment_variable(self): os.environ['VOLUME_PATH'] = '/host/path' d = config.load( config.ConfigDetails( - config={'foo': {'volumes': ['${VOLUME_PATH}:/container/path']}}, + config={'foo': {'build': '.', 'volumes': ['${VOLUME_PATH}:/container/path']}}, working_dir='.', filename=None, ) @@ -422,7 +416,7 @@ def test_validation_fails_with_just_memswap_limit(self): a mem_limit """ expected_error_msg = "Invalid 'memswap_limit' configuration for 'foo' service: when defining 'memswap_limit' you must set 'mem_limit' as well" - with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( { @@ -465,7 +459,7 @@ def test_parse_environment_as_dict(self): self.assertEqual(config.parse_environment(environment), environment) def test_parse_environment_invalid(self): - with self.assertRaises(config.ConfigurationError): + with self.assertRaises(ConfigurationError): config.parse_environment('a=b') def test_parse_environment_empty(self): @@ -519,7 +513,7 @@ def test_env_from_multiple_files(self): def test_env_nonexistent_file(self): options = {'env_file': 'nonexistent.env'} self.assertRaises( - config.ConfigurationError, + ConfigurationError, lambda: make_service_dict('foo', options, 'tests/fixtures/env'), ) @@ -545,7 +539,7 @@ def test_resolve_path(self): service_dict = config.load( config.ConfigDetails( - config={'foo': {'volumes': ['$HOSTENV:$CONTAINERENV']}}, + config={'foo': {'build': '.', 'volumes': ['$HOSTENV:$CONTAINERENV']}}, working_dir="tests/fixtures/env", filename=None, ) @@ -554,7 +548,7 @@ def test_resolve_path(self): service_dict = config.load( config.ConfigDetails( - config={'foo': {'volumes': ['/opt${HOSTENV}:/opt${CONTAINERENV}']}}, + config={'foo': {'build': '.', 'volumes': ['/opt${HOSTENV}:/opt${CONTAINERENV}']}}, working_dir="tests/fixtures/env", filename=None, ) @@ -652,7 +646,7 @@ def test_circular(self): ) def test_extends_validation_empty_dictionary(self): - with self.assertRaisesRegexp(config.ConfigurationError, 'service'): + with self.assertRaisesRegexp(ConfigurationError, 'service'): config.load( config.ConfigDetails( { @@ -664,7 +658,7 @@ def test_extends_validation_empty_dictionary(self): ) def test_extends_validation_missing_service_key(self): - with self.assertRaisesRegexp(config.ConfigurationError, "u'service' is a required property"): + with self.assertRaisesRegexp(ConfigurationError, "u'service' is a required property"): config.load( config.ConfigDetails( { @@ -677,7 +671,7 @@ def test_extends_validation_missing_service_key(self): def test_extends_validation_invalid_key(self): expected_error_msg = "Unsupported config option for 'web' service: 'rogue_key'" - with self.assertRaisesRegexp(config.ConfigurationError, expected_error_msg): + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( { @@ -701,7 +695,7 @@ def test_extends_validation_no_file_key_no_filename_set(self): def load_config(): return make_service_dict('myweb', dictionary, working_dir='tests/fixtures/extends') - self.assertRaisesRegexp(config.ConfigurationError, 'file', load_config) + self.assertRaisesRegexp(ConfigurationError, 'file', load_config) def test_extends_validation_valid_config(self): service = config.load( @@ -750,19 +744,19 @@ def load_config(): } }, '.') - with self.assertRaisesRegexp(config.ConfigurationError, 'links'): + with self.assertRaisesRegexp(ConfigurationError, 'links'): other_config = {'web': {'links': ['db']}} with mock.patch.object(config, 'load_yaml', return_value=other_config): print load_config() - with self.assertRaisesRegexp(config.ConfigurationError, 'volumes_from'): + with self.assertRaisesRegexp(ConfigurationError, 'volumes_from'): other_config = {'web': {'volumes_from': ['db']}} with mock.patch.object(config, 'load_yaml', return_value=other_config): print load_config() - with self.assertRaisesRegexp(config.ConfigurationError, 'net'): + with self.assertRaisesRegexp(ConfigurationError, 'net'): other_config = {'web': {'net': 'container:db'}} with mock.patch.object(config, 'load_yaml', return_value=other_config): @@ -804,7 +798,7 @@ def setUp(self): self.abs_context_path = os.path.join(os.getcwd(), 'tests/fixtures/build-ctx') def test_nonexistent_path(self): - with self.assertRaises(config.ConfigurationError): + with self.assertRaises(ConfigurationError): config.load( config.ConfigDetails( { From df74b131ff8ca8f6055a1e16d4e9c7ff56462370 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Fri, 7 Aug 2015 15:26:26 +0100 Subject: [PATCH 10/17] Use split_port for ports format check Rather than implement the logic a second time, use docker-py split_port function to test if the ports is valid. Signed-off-by: Mazz Mosley --- compose/config/schema.json | 13 ++++--------- compose/config/validation.py | 24 ++++++------------------ tests/unit/config_test.py | 4 ++-- 3 files changed, 12 insertions(+), 29 deletions(-) diff --git a/compose/config/schema.json b/compose/config/schema.json index 258f44ccac0..74f5edbbffc 100644 --- a/compose/config/schema.json +++ b/compose/config/schema.json @@ -75,15 +75,10 @@ "pid": {"type": "string"}, "ports": { - "oneOf": [ - {"type": "string", "format": "ports"}, - { - "type": "array", - "items": {"type": "string"}, - "uniqueItems": true, - "format": "ports" - } - ] + "type": "array", + "items": {"type": "string"}, + "uniqueItems": true, + "format": "ports" }, "privileged": {"type": "string"}, diff --git a/compose/config/validation.py b/compose/config/validation.py index ba5803decb4..15e0754cf8d 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -1,5 +1,6 @@ import os +from docker.utils.ports import split_port import json from jsonschema import Draft4Validator, FormatChecker, ValidationError @@ -26,26 +27,13 @@ VALID_NAME_CHARS = '[a-zA-Z0-9\._\-]' -@FormatChecker.cls_checks(format="ports", raises=ValidationError("Ports is incorrectly formatted.")) +@FormatChecker.cls_checks(format="ports", raises=ValidationError("Invalid port formatting, it should be '[[remote_ip:]remote_port:]port[/protocol]'")) def format_ports(instance): - def _is_valid(port): - if ':' in port or '/' in port: - return True - try: - int(port) - return True - except ValueError: - return False + try: + split_port(instance) + except ValueError: return False - - if isinstance(instance, list): - for port in instance: - if not _is_valid(port): - return False - return True - elif isinstance(instance, str): - return _is_valid(instance) - return False + return True def get_unsupported_config_msg(service_name, error_key): diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index 9f690f111ea..4e982bb49a0 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -76,7 +76,7 @@ def test_config_valid_service_names(self): def test_config_invalid_ports_format_validation(self): with self.assertRaises(ConfigurationError): - for invalid_ports in [{"1": "8000"}, "whatport"]: + for invalid_ports in [{"1": "8000"}, "whatport", "625", "8000:8050"]: config.load( config.ConfigDetails( {'web': {'image': 'busybox', 'ports': invalid_ports}}, @@ -86,7 +86,7 @@ def test_config_invalid_ports_format_validation(self): ) def test_config_valid_ports_format_validation(self): - valid_ports = [["8000", "9000"], "625", "8000:8050", ["8000/8050"]] + valid_ports = [["8000", "9000"], ["8000/8050"], ["8000"]] for ports in valid_ports: config.load( config.ConfigDetails( From b4872de2135a41481f3cbfe97c75126b26c11929 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 10 Aug 2015 14:55:33 +0100 Subject: [PATCH 11/17] Allow integer value for ports While it was intended as a positive to be stricter in validation it would in fact break backwards compatibility, which we do not want to be doing. Consider re-visiting this later and include a deprecation warning if we want to be stricter. Signed-off-by: Mazz Mosley --- compose/config/schema.json | 20 ++++++++++++++++---- compose/config/validation.py | 7 +++++++ tests/unit/config_test.py | 7 ++++--- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/compose/config/schema.json b/compose/config/schema.json index 74f5edbbffc..24fd53d116b 100644 --- a/compose/config/schema.json +++ b/compose/config/schema.json @@ -75,10 +75,22 @@ "pid": {"type": "string"}, "ports": { - "type": "array", - "items": {"type": "string"}, - "uniqueItems": true, - "format": "ports" + "oneOf": [ + { + "type": "array", + "items": {"type": "string"}, + "uniqueItems": true, + "format": "ports" + }, + { + "type": "string", + "format": "ports" + }, + { + "type": "number", + "format": "ports" + } + ] }, "privileged": {"type": "string"}, diff --git a/compose/config/validation.py b/compose/config/validation.py index 15e0754cf8d..3f46632b850 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -84,6 +84,13 @@ def _parse_key_from_error_msg(error): required.append("Service '{}' has neither an image nor a build path specified. Exactly one must be provided.".format(service_name)) else: required.append(error.message) + elif error.validator == 'oneOf': + config_key = error.path[1] + valid_types = [context.validator_value for context in error.context] + valid_type_msg = " or ".join(valid_types) + type_errors.append("Service '{}' configuration key '{}' contains an invalid type, it should be either {}".format( + service_name, config_key, valid_type_msg) + ) elif error.validator == 'type': msg = "a" if error.validator_value == "array": diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index 4e982bb49a0..b4d2ce82f6a 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -75,8 +75,9 @@ def test_config_valid_service_names(self): ) def test_config_invalid_ports_format_validation(self): - with self.assertRaises(ConfigurationError): - for invalid_ports in [{"1": "8000"}, "whatport", "625", "8000:8050"]: + expected_error_msg = "Service 'web' configuration key 'ports' contains an invalid type" + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): + for invalid_ports in [{"1": "8000"}, False, 0]: config.load( config.ConfigDetails( {'web': {'image': 'busybox', 'ports': invalid_ports}}, @@ -86,7 +87,7 @@ def test_config_invalid_ports_format_validation(self): ) def test_config_valid_ports_format_validation(self): - valid_ports = [["8000", "9000"], ["8000/8050"], ["8000"]] + valid_ports = [["8000", "9000"], ["8000/8050"], ["8000"], "8000", 8000] for ports in valid_ports: config.load( config.ConfigDetails( From ece6a7271259f6d72586eaa066ebb3034e62ff79 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 10 Aug 2015 15:33:47 +0100 Subject: [PATCH 12/17] Clean error.message Unfortunately the way that jsonschema is calling %r on its property and then encoding the complete message means I've had to do this manual way of removing the literal string prefix, u'. eg: key = 'extends' message = "Invalid value for %r" % key error.message = message.encode("utf-8")" results in: "Invalid value for u'extends'" Performing a replace to strip out the extra "u'", does not change the encoding of the string, it is at this point the character u followed by a '. Signed-off-by: Mazz Mosley --- compose/config/validation.py | 9 ++++++--- tests/unit/config_test.py | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index 3f46632b850..7347c0128dc 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -52,6 +52,9 @@ def process_errors(errors): def _parse_key_from_error_msg(error): return error.message.split("'")[1] + def _clean_error_message(message): + return message.replace("u'", "'") + root_msgs = [] invalid_keys = [] required = [] @@ -68,7 +71,7 @@ def _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(error.message) + root_msgs.append(_clean_error_message(error.message)) else: # handle service level errors @@ -83,7 +86,7 @@ def _parse_key_from_error_msg(error): 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)) else: - required.append(error.message) + required.append(_clean_error_message(error.message)) elif error.validator == 'oneOf': config_key = error.path[1] valid_types = [context.validator_value for context in error.context] @@ -104,7 +107,7 @@ def _parse_key_from_error_msg(error): 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(config_key)) elif error.validator == 'required': config_key = error.path[1] - required.append("Service '{}' option '{}' is invalid, {}".format(service_name, config_key, error.message)) + required.append("Service '{}' option '{}' is invalid, {}".format(service_name, config_key, _clean_error_message(error.message))) elif error.validator == 'dependencies': dependency_key = error.validator_value.keys()[0] required_keys = ",".join(error.validator_value[dependency_key]) diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index b4d2ce82f6a..e023153a33a 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -659,7 +659,7 @@ def test_extends_validation_empty_dictionary(self): ) def test_extends_validation_missing_service_key(self): - with self.assertRaisesRegexp(ConfigurationError, "u'service' is a required property"): + with self.assertRaisesRegexp(ConfigurationError, "'service' is a required property"): config.load( config.ConfigDetails( { From e0675b50c0fa0cc737bfd814d45e495e3d7eaba6 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 10 Aug 2015 16:18:21 +0100 Subject: [PATCH 13/17] Retrieve sub property keys The validation message was confusing by displaying only 1 level of property of the service, even if the error was another level down. Eg. if the 'files' property of 'extends' was the incorrect format, it was displaying 'an invalid value for 'extends'', rather than correctly retrieving 'files'. Signed-off-by: Mazz Mosley --- compose/config/validation.py | 14 ++++++++------ tests/unit/config_test.py | 21 ++++++++++++++++++++- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index 7347c0128dc..aa2e0fcf4c4 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -99,12 +99,14 @@ def _clean_error_message(message): if error.validator_value == "array": msg = "an" - try: - config_key = error.path[1] - type_errors.append("Service '{}' has an invalid value for '{}', it should be {} {}".format(service_name, config_key, msg, error.validator_value)) - except IndexError: - config_key = error.path[0] - 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(config_key)) + # pop the service name off our path + error.path.popleft() + + 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, error.validator_value)) + 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[1] required.append("Service '{}' option '{}' is invalid, {}".format(service_name, config_key, _clean_error_message(error.message))) diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index e023153a33a..0ea375db49e 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -124,7 +124,7 @@ def test_invalid_config_build_and_image_specified(self): ) def test_invalid_config_type_should_be_an_array(self): - expected_error_msg = "Service 'foo' has an invalid value for 'links', it should be an array" + expected_error_msg = "Service 'foo' configuration key 'links' contains an invalid type, it should be an array" with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): config.load( config.ConfigDetails( @@ -690,6 +690,25 @@ def test_extends_validation_invalid_key(self): ) ) + def test_extends_validation_sub_property_key(self): + expected_error_msg = "Service 'web' configuration key 'extends' 'file' contains an invalid type" + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): + config.load( + config.ConfigDetails( + { + 'web': { + 'image': 'busybox', + 'extends': { + 'file': 1, + 'service': 'web', + } + }, + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) + def test_extends_validation_no_file_key_no_filename_set(self): dictionary = {'extends': {'service': 'web'}} From df14a4384d44f6a27063de08e66d25854509f8d3 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 10 Aug 2015 16:57:32 +0100 Subject: [PATCH 14/17] Catch non-unique errors When a schema type is set as unique, we should display the validation error to indicate that non-unique values have been provided for a key. Signed-off-by: Mazz Mosley --- compose/config/validation.py | 10 +++++++++- tests/unit/config_test.py | 13 +++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index aa2e0fcf4c4..07b542a116f 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -59,6 +59,7 @@ def _clean_error_message(message): invalid_keys = [] required = [] type_errors = [] + other_errors = [] for error in errors: # handle root level errors @@ -115,8 +116,15 @@ def _clean_error_message(message): 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: + # pop the service name off our path + error.path.popleft() + + 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) + return "\n".join(root_msgs + invalid_keys + required + type_errors + other_errors) def validate_against_schema(config): diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index 0ea375db49e..f35010c6955 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -147,6 +147,19 @@ def test_invalid_config_not_a_dictionary(self): ) ) + def test_invalid_config_not_unique_items(self): + expected_error_msg = "has non-unique elements" + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): + config.load( + config.ConfigDetails( + { + 'web': {'build': '.', 'devices': ['/dev/foo:/dev/foo', '/dev/foo:/dev/foo']} + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) + class InterpolationTest(unittest.TestCase): @mock.patch.dict(os.environ) From 68de84a0bfeb60c4660f110cde850ac17ce3672a Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 10 Aug 2015 17:12:37 +0100 Subject: [PATCH 15/17] Clean up error.path handling Tiny bit of refactoring to make it clearer and only pop service_name once. Signed-off-by: Mazz Mosley --- compose/config/validation.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index 07b542a116f..946acf149b0 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -78,6 +78,9 @@ def _clean_error_message(message): # handle service level errors service_name = error.path[0] + # pop the service name off our path + error.path.popleft() + 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)) @@ -89,7 +92,7 @@ def _clean_error_message(message): else: required.append(_clean_error_message(error.message)) elif error.validator == 'oneOf': - config_key = error.path[1] + config_key = error.path[0] valid_types = [context.validator_value for context in error.context] valid_type_msg = " or ".join(valid_types) type_errors.append("Service '{}' configuration key '{}' contains an invalid type, it should be either {}".format( @@ -100,16 +103,13 @@ def _clean_error_message(message): if error.validator_value == "array": msg = "an" - # pop the service name off our path - error.path.popleft() - 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, error.validator_value)) 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[1] + 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 = error.validator_value.keys()[0] @@ -117,9 +117,6 @@ def _clean_error_message(message): required.append("Invalid '{}' configuration for '{}' service: when defining '{}' you must set '{}' as well".format( dependency_key, service_name, dependency_key, required_keys)) else: - # pop the service name off our path - error.path.popleft() - 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) From f8efb54c80ed661538f06ecea7c1329925442b3a Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 11 Aug 2015 13:06:32 +0100 Subject: [PATCH 16/17] Handle $ref defined types errors We use $ref in the schema to allow us to specify multiple type, eg command, it can be a string or a list of strings. It required some extra parsing to retrieve a helpful type to display in our error message rather than 'string or string'. Which while correct, is not helpful. We value helpful. Signed-off-by: Mazz Mosley --- compose/config/validation.py | 16 ++++++++++++++-- tests/unit/config_test.py | 13 +++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/compose/config/validation.py b/compose/config/validation.py index 946acf149b0..36fd03b5f20 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -55,6 +55,16 @@ def _parse_key_from_error_msg(error): def _clean_error_message(message): return message.replace("u'", "'") + def _parse_valid_types_from_schema(schema): + """ + Our defined types using $ref in the schema require some extra parsing + retrieve a helpful type for error message display. + """ + if '$ref' in schema: + return schema['$ref'].replace("#/definitions/", "").replace("_", " ") + else: + return str(schema['type']) + root_msgs = [] invalid_keys = [] required = [] @@ -93,9 +103,11 @@ def _clean_error_message(message): required.append(_clean_error_message(error.message)) elif error.validator == 'oneOf': config_key = error.path[0] - valid_types = [context.validator_value for context in error.context] + + valid_types = [_parse_valid_types_from_schema(schema) for schema in error.schema['oneOf']] valid_type_msg = " or ".join(valid_types) - type_errors.append("Service '{}' configuration key '{}' contains an invalid type, it should be either {}".format( + + type_errors.append("Service '{}' configuration key '{}' contains an invalid type, valid types are {}".format( service_name, config_key, valid_type_msg) ) elif error.validator == 'type': diff --git a/tests/unit/config_test.py b/tests/unit/config_test.py index f35010c6955..1948e218265 100644 --- a/tests/unit/config_test.py +++ b/tests/unit/config_test.py @@ -160,6 +160,19 @@ def test_invalid_config_not_unique_items(self): ) ) + def test_invalid_list_of_strings_format(self): + expected_error_msg = "'command' contains an invalid type, valid types are string or list of strings" + with self.assertRaisesRegexp(ConfigurationError, expected_error_msg): + config.load( + config.ConfigDetails( + { + 'web': {'build': '.', 'command': [1]} + }, + 'tests/fixtures/extends', + 'filename.yml' + ) + ) + class InterpolationTest(unittest.TestCase): @mock.patch.dict(os.environ) From 810bb702495f1d6d15008ed0cf87956b863a7ad7 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 11 Aug 2015 16:31:56 +0100 Subject: [PATCH 17/17] Include schema in manifest Signed-off-by: Mazz Mosley --- MANIFEST.in | 1 + 1 file changed, 1 insertion(+) diff --git a/MANIFEST.in b/MANIFEST.in index 6c756417e04..7d48d347a84 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -4,6 +4,7 @@ include requirements.txt include requirements-dev.txt include tox.ini include *.md +include compose/config/schema.json recursive-include contrib/completion * recursive-include tests * global-exclude *.pyc