From a7689f3da8a671b2a5dacd58ebc9259c86793f86 Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Fri, 11 Dec 2015 17:21:04 -0800 Subject: [PATCH] Handle volume driver change error in config. Assume version=1 if file is empty in get_config_version Empty files are invalid anyway, so this simplifies the algorithm somewhat. https://github.com/docker/compose/pull/2421#discussion_r47223144 Don't leak version considerations in interpolation/service validation Signed-off-by: Joffrey F --- compose/config/config.py | 22 +++++++++++++----- compose/config/interpolation.py | 10 ++------ compose/config/validation.py | 10 ++++---- compose/project.py | 12 ++++++++++ tests/integration/project_test.py | 38 +++++++++++++++++++++++++++++-- tests/unit/config/config_test.py | 23 +++++++++++++++++++ 6 files changed, 94 insertions(+), 21 deletions(-) diff --git a/compose/config/config.py b/compose/config/config.py index 63ef44c3..c77e6100 100644 --- a/compose/config/config.py +++ b/compose/config/config.py @@ -118,6 +118,9 @@ class ConfigFile(namedtuple('_ConfigFile', 'filename config')): def from_filename(cls, filename): return cls(filename, load_yaml(filename)) + def get_service_dicts(self, version): + return self.config if version == 1 else self.config.get('services', {}) + class Config(namedtuple('_Config', 'version services volumes')): """ @@ -164,9 +167,11 @@ def find(base_dir, filenames): def get_config_version(config_details): def get_version(config): if config.config is None: - return None + return 1 version = config.config.get('version', 1) if isinstance(version, dict): + # in that case 'version' is probably a service name, so assume + # this is a legacy (version=1) file version = 1 return version @@ -176,9 +181,6 @@ def get_config_version(config_details): for next_file in config_details.config_files[1:]: validate_top_level_object(next_file) next_file_version = get_version(next_file) - if version is None: - version = next_file_version - continue if version != next_file_version and next_file_version is not None: raise ConfigurationError( @@ -316,8 +318,16 @@ def load_services(working_dir, config_files, version): def process_config_file(config_file, version, service_name=None): - validate_top_level_service_objects(config_file, version) - processed_config = interpolate_environment_variables(config_file.config, version) + service_dicts = config_file.get_service_dicts(version) + validate_top_level_service_objects( + config_file.filename, service_dicts + ) + interpolated_config = interpolate_environment_variables(service_dicts) + if version == 2: + processed_config = dict(config_file.config) + processed_config.update({'services': interpolated_config}) + if version == 1: + processed_config = interpolated_config validate_against_fields_schema( processed_config, config_file.filename, version ) diff --git a/compose/config/interpolation.py b/compose/config/interpolation.py index a8ff08d8..12eb497b 100644 --- a/compose/config/interpolation.py +++ b/compose/config/interpolation.py @@ -8,19 +8,13 @@ from .errors import ConfigurationError log = logging.getLogger(__name__) -def interpolate_environment_variables(config, version): +def interpolate_environment_variables(service_dicts): mapping = BlankDefaultDict(os.environ) - service_dicts = config if version == 1 else config.get('services', {}) - interpolated = dict( + return dict( (service_name, process_service(service_name, service_dict, mapping)) for (service_name, service_dict) in service_dicts.items() ) - if version == 1: - return interpolated - result = dict(config) - result.update({'services': interpolated}) - return result def process_service(service_name, service_dict, mapping): diff --git a/compose/config/validation.py b/compose/config/validation.py index 6e943974..091014f6 100644 --- a/compose/config/validation.py +++ b/compose/config/validation.py @@ -74,19 +74,18 @@ def format_boolean_in_environment(instance): return True -def validate_top_level_service_objects(config_file, version): +def validate_top_level_service_objects(filename, service_dicts): """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. """ - service_dicts = config_file.config if version == 1 else config_file.config.get('services', {}) for service_name, service_dict in service_dicts.items(): if not isinstance(service_name, six.string_types): raise ConfigurationError( "In file '{}' service name: {} needs to be a string, eg '{}'".format( - config_file.filename, + filename, service_name, service_name)) @@ -95,8 +94,9 @@ def validate_top_level_service_objects(config_file, version): "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)) + filename, service_name + ) + ) def validate_top_level_object(config_file): diff --git a/compose/project.py b/compose/project.py index 36855f16..3801bbb9 100644 --- a/compose/project.py +++ b/compose/project.py @@ -236,6 +236,18 @@ class Project(object): raise ConfigurationError( 'Volume %s specifies nonexistent driver %s' % (volume.name, volume.driver) ) + except APIError as e: + if 'Choose a different volume name' in str(e): + raise ConfigurationError( + 'Configuration for volume {0} specifies driver {1}, but ' + 'a volume with the same name uses a different driver ' + '({3}). If you wish to use the new configuration, please ' + 'remove the existing volume "{2}" first:\n' + '$ docker volume rm {2}'.format( + volume.name, volume.driver, volume.full_name, + volume.inspect()['Driver'] + ) + ) def restart(self, service_names=None, **options): containers = self.containers(service_names, stopped=True) diff --git a/tests/integration/project_test.py b/tests/integration/project_test.py index f2c65084..d51830bb 100644 --- a/tests/integration/project_test.py +++ b/tests/integration/project_test.py @@ -579,11 +579,11 @@ class ProjectTest(DockerClientTestCase): vol_name = '{0:x}'.format(random.getrandbits(32)) config_data = config.Config( - 2, [{ + version=2, services=[{ 'name': 'web', 'image': 'busybox:latest', 'command': 'top' - }], {vol_name: {'driver': 'foobar'}} + }], volumes={vol_name: {'driver': 'foobar'}} ) project = Project.from_config( @@ -592,3 +592,37 @@ class ProjectTest(DockerClientTestCase): ) with self.assertRaises(config.ConfigurationError): project.initialize_volumes() + + def test_project_up_updated_driver(self): + vol_name = '{0:x}'.format(random.getrandbits(32)) + full_vol_name = 'composetest_{0}'.format(vol_name) + + config_data = config.Config( + version=2, services=[{ + 'name': 'web', + 'image': 'busybox:latest', + 'command': 'top' + }], volumes={vol_name: {'driver': 'local'}} + ) + project = Project.from_config( + name='composetest', + config_data=config_data, client=self.client + ) + project.initialize_volumes() + + volume_data = self.client.inspect_volume(full_vol_name) + self.assertEqual(volume_data['Name'], full_vol_name) + self.assertEqual(volume_data['Driver'], 'local') + + config_data = config_data._replace( + volumes={vol_name: {'driver': 'smb'}} + ) + project = Project.from_config( + name='composetest', + config_data=config_data, client=self.client + ) + with self.assertRaises(config.ConfigurationError) as e: + project.initialize_volumes() + assert 'Configuration for volume {0} specifies driver smb'.format( + vol_name + ) in str(e.exception) diff --git a/tests/unit/config/config_test.py b/tests/unit/config/config_test.py index 74978150..281e81d1 100644 --- a/tests/unit/config/config_test.py +++ b/tests/unit/config/config_test.py @@ -286,6 +286,18 @@ class ConfigTest(unittest.TestCase): 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_override_v2(self): + base_file = config.ConfigFile( + 'base.yml', + {'version': 2, 'services': {'web': {'image': 'example/web'}}}) + override_file = config.ConfigFile('override.yml', None) + details = config.ConfigDetails('.', [base_file, override_file]) + + with pytest.raises(ConfigurationError) as exc: + config.load(details) + 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.yml', None) override_file = config.ConfigFile( @@ -297,6 +309,17 @@ class ConfigTest(unittest.TestCase): config.load(details) assert "Top level object in 'base.yml' needs to be an object" in exc.exconly() + def test_load_with_multiple_files_and_empty_base_v2(self): + base_file = config.ConfigFile('base.yml', None) + override_file = config.ConfigFile( + 'override.tml', + {'version': 2, 'services': {'web': {'image': 'example/web'}}} + ) + details = config.ConfigDetails('.', [base_file, override_file]) + with pytest.raises(ConfigurationError) as exc: + config.load(details) + 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( 'base.yaml',