Improve error messages for invalid versions

Signed-off-by: Aanand Prasad <[email protected]>

compose/config/config.py

@@ -16,10 +16,10 @@ from cached_property import cached_property
 
 from ..const import COMPOSEFILE_V1 as V1
 from ..const import COMPOSEFILE_V2_0 as V2_0
-from ..const import COMPOSEFILE_VERSIONS
 from .errors import CircularReference
 from .errors import ComposeFileNotFound
 from .errors import ConfigurationError
+from .errors import VERSION_EXPLANATION
 from .interpolation import interpolate_environment_variables
 from .sort_services import get_container_name_from_network_mode
 from .sort_services import get_service_name_from_network_mode
@@ -105,6 +105,7 @@ SUPPORTED_FILENAMES = [
 
 DEFAULT_OVERRIDE_FILENAME = 'docker-compose.override.yml'
 
+
 log = logging.getLogger(__name__)
 
 
@@ -131,7 +132,10 @@ class ConfigFile(namedtuple('_ConfigFile', 'filename config')):
 
     @cached_property
     def version(self):
-        version = self.config.get('version', V1)
+        if 'version' not in self.config:
+            return V1
+
+        version = self.config['version']
 
         if isinstance(version, dict):
             log.warn("Unexpected type for field 'version', in file {} assuming "
@@ -139,12 +143,23 @@ class ConfigFile(namedtuple('_ConfigFile', 'filename config')):
                      "Compose file version 1".format(self.filename))
             return V1
 
+        if not isinstance(version, six.string_types):
+            raise ConfigurationError(
+                'Version in "{}" is invalid - it should be a string.'
+                .format(self.filename))
+
+        if version == '1':
+            raise ConfigurationError(
+                'Version in "{}" is invalid. {}'
+                .format(self.filename, VERSION_EXPLANATION))
+
         if version == '2':
             version = V2_0
 
-        if version not in COMPOSEFILE_VERSIONS:
+        if version != V2_0:
             raise ConfigurationError(
-                'Invalid Compose file version: {0}'.format(version))
+                'Version in "{}" is unsupported. {}'
+                .format(self.filename, VERSION_EXPLANATION))
 
         return version
 

compose/config/errors.py

@@ -2,6 +2,14 @@ from __future__ import absolute_import
 from __future__ import unicode_literals
 
 
+VERSION_EXPLANATION = (
+    'Either specify a version of "2" (or "2.0") and place your service '
+    'definitions under the `services` key, or omit the `version` key and place '
+    'your service definitions at the root of the file to use version 1.\n'
+    'For more on the Compose file format versions, see '
+    'https://docs.docker.com/compose/compose-file/')
+
+
 class ConfigurationError(Exception):
     def __init__(self, msg):
         self.msg = msg

compose/config/validation.py

@@ -15,6 +15,7 @@ from jsonschema import RefResolver
 from jsonschema import ValidationError
 
 from .errors import ConfigurationError
+from .errors import VERSION_EXPLANATION
 from .sort_services import get_service_name_from_network_mode
 
 
@@ -229,11 +230,14 @@ def handle_error_for_schema_with_id(error, path):
                 "A service can either be built to image or use an existing "
                 "image, not both.".format(path_string(path)))
 
-    if schema_id == '#/definitions/service':
-        if error.validator == 'additionalProperties':
+    if error.validator == 'additionalProperties':
+        if schema_id == '#/definitions/service':
             invalid_config_key = parse_key_from_error_msg(error)
             return get_unsupported_config_msg(path, invalid_config_key)
 
+        if not error.path:
+            return '{}\n{}'.format(error.message, VERSION_EXPLANATION)
+
 
 def handle_generic_service_error(error, path):
     msg_format = None

compose/const.py

@@ -17,7 +17,6 @@ LABEL_CONFIG_HASH = 'com.docker.compose.config-hash'
 
 COMPOSEFILE_V1 = '1'
 COMPOSEFILE_V2_0 = '2.0'
-COMPOSEFILE_VERSIONS = (COMPOSEFILE_V1, COMPOSEFILE_V2_0)
 
 API_VERSIONS = {
     COMPOSEFILE_V1: '1.21',

tests/unit/config/config_test.py

@@ -17,6 +17,7 @@ from compose.config.config import resolve_environment
 from compose.config.config import V1
 from compose.config.config import V2_0
 from compose.config.errors import ConfigurationError
+from compose.config.errors import VERSION_EXPLANATION
 from compose.config.types import VolumeSpec
 from compose.const import IS_WINDOWS_PLATFORM
 from tests import mock
@@ -159,8 +160,8 @@ class ConfigTest(unittest.TestCase):
         assert list(s['name'] for s in cfg.services) == ['version']
 
     def test_wrong_version_type(self):
-        for version in [None, 2, 2.0]:
-            with pytest.raises(ConfigurationError):
+        for version in [None, 1, 2, 2.0]:
+            with pytest.raises(ConfigurationError) as excinfo:
                 config.load(
                     build_config_details(
                         {'version': version},
@@ -168,8 +169,11 @@ class ConfigTest(unittest.TestCase):
                     )
                 )
 
+            assert 'Version in "filename.yml" is invalid - it should be a string.' \
+                in excinfo.exconly()
+
     def test_unsupported_version(self):
-        with pytest.raises(ConfigurationError):
+        with pytest.raises(ConfigurationError) as excinfo:
             config.load(
                 build_config_details(
                     {'version': '2.1'},
@@ -177,18 +181,38 @@ class ConfigTest(unittest.TestCase):
                 )
             )
 
+        assert 'Version in "filename.yml" is unsupported' in excinfo.exconly()
+        assert VERSION_EXPLANATION in excinfo.exconly()
+
+    def test_version_1_is_invalid(self):
+        with pytest.raises(ConfigurationError) as excinfo:
+            config.load(
+                build_config_details(
+                    {
+                        'version': '1',
+                        'web': {'image': 'busybox'},
+                    },
+                    filename='filename.yml',
+                )
+            )
+
+        assert 'Version in "filename.yml" is invalid' in excinfo.exconly()
+        assert VERSION_EXPLANATION in excinfo.exconly()
+
     def test_v1_file_with_version_is_invalid(self):
-        for version in [1, "1"]:
-            with pytest.raises(ConfigurationError):
-                config.load(
-                    build_config_details(
-                        {
-                            'version': version,
-                            'web': {'image': 'busybox'},
-                        },
-                        filename='filename.yml',
-                    )
+        with pytest.raises(ConfigurationError) as excinfo:
+            config.load(
+                build_config_details(
+                    {
+                        'version': '2',
+                        'web': {'image': 'busybox'},
+                    },
+                    filename='filename.yml',
                 )
+            )
+
+        assert 'Additional properties are not allowed' in excinfo.exconly()
+        assert VERSION_EXPLANATION in excinfo.exconly()
 
     def test_named_volume_config_empty(self):
         config_details = build_config_details({
@@ -226,27 +250,6 @@ class ConfigTest(unittest.TestCase):
             ])
         )
 
-    def test_load_invalid_version(self):
-        with self.assertRaises(ConfigurationError):
-            config.load(
-                build_config_details({
-                    'version': 18,
-                    'services': {
-                        'foo': {'image': 'busybox'}
-                    }
-                }, 'working_dir', 'filename.yml')
-            )
-
-        with self.assertRaises(ConfigurationError):
-            config.load(
-                build_config_details({
-                    'version': 'two point oh',
-                    'services': {
-                        'foo': {'image': 'busybox'}
-                    }
-                }, 'working_dir', 'filename.yml')
-            )
-
     def test_load_throws_error_when_not_dict(self):
         with self.assertRaises(ConfigurationError):
             config.load(