From 1f7faadc7712f5bf734e6254d2a8d6f1427f5029 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 6 Nov 2015 11:57:27 -0500 Subject: [PATCH] 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)