Wrong format in the healthcheck test does not issue a warning (fixes #4424)

Signed-off-by: Guillermo Arribas <garribas@gmail.com>

compose/config/config.py

@@ -47,6 +47,7 @@ from .validation import validate_config_section
 from .validation import validate_cpu
 from .validation import validate_depends_on
 from .validation import validate_extends_file_path
+from .validation import validate_healthcheck
 from .validation import validate_links
 from .validation import validate_network_mode
 from .validation import validate_pid_mode
@@ -686,6 +687,7 @@ def validate_service(service_config, service_names, config_file):
     validate_pid_mode(service_config, service_names)
     validate_depends_on(service_config, service_names)
     validate_links(service_config, service_names)
+    validate_healthcheck(service_config)
 
     if not service_dict.get('image') and has_uppercase(service_name):
         raise ConfigurationError(
@@ -724,7 +726,7 @@ def process_service(service_config):
             service_dict[field] = to_list(service_dict[field])
 
     service_dict = process_blkio_config(process_ports(
-        process_healthcheck(service_dict, service_config.name)
+        process_healthcheck(service_dict)
     ))
 
     return service_dict
@@ -788,33 +790,20 @@ def process_blkio_config(service_dict):
     return service_dict
 
 
-def process_healthcheck(service_dict, service_name):
+def process_healthcheck(service_dict):
     if 'healthcheck' not in service_dict:
         return service_dict
 
-    hc = {}
+    if 'disable' in service_dict['healthcheck']:
-    raw = service_dict['healthcheck']
+        del service_dict['healthcheck']['disable']
-
+        service_dict['healthcheck']['test'] = ['NONE']
-    if raw.get('disable'):
-        if len(raw) > 1:
-            raise ConfigurationError(
-                'Service "{}" defines an invalid healthcheck: '
-                '"disable: true" cannot be combined with other options'
-                .format(service_name))
-        hc['test'] = ['NONE']
-    elif 'test' in raw:
-        hc['test'] = raw['test']
 
     for field in ['interval', 'timeout', 'start_period']:
-        if field in raw:
+        if field in service_dict['healthcheck']:
-            if not isinstance(raw[field], six.integer_types):
+            if not isinstance(service_dict['healthcheck'][field], six.integer_types):
-                hc[field] = parse_nanoseconds_int(raw[field])
+                service_dict['healthcheck'][field] = parse_nanoseconds_int(
-            else:  # Conversion has been done previously
+                                                        service_dict['healthcheck'][field])
-                hc[field] = raw[field]
-    if 'retries' in raw:
-        hc['retries'] = raw['retries']
 
-    service_dict['healthcheck'] = hc
     return service_dict
 
 

compose/config/validation.py

@@ -465,3 +465,27 @@ def handle_errors(errors, format_error_func, filename):
         "The Compose file{file_msg} is invalid because:\n{error_msg}".format(
             file_msg=" '{}'".format(filename) if filename else "",
             error_msg=error_msg))
+
+
+def validate_healthcheck(service_config):
+    healthcheck = service_config.config.get('healthcheck', {})
+
+    if 'test' in healthcheck and isinstance(healthcheck['test'], list):
+        if len(healthcheck['test']) == 0:
+            raise ConfigurationError(
+                'Service "{}" defines an invalid healthcheck: '
+                '"test" is an empty list'
+                .format(service_config.name))
+
+        # when disable is true config.py::process_healthcheck adds "test: ['NONE']" to service_config
+        elif healthcheck['test'][0] == 'NONE' and len(healthcheck) > 1:
+            raise ConfigurationError(
+                'Service "{}" defines an invalid healthcheck: '
+                '"disable: true" cannot be combined with other options'
+                .format(service_config.name))
+
+        elif healthcheck['test'][0] not in ('NONE', 'CMD', 'CMD-SHELL'):
+            raise ConfigurationError(
+                'Service "{}" defines an invalid healthcheck: '
+                'when "test" is a list the first item must be either NONE, CMD or CMD-SHELL'
+                .format(service_config.name))

tests/unit/config/config_test.py

@@ -34,7 +34,6 @@ from compose.const import COMPOSEFILE_V3_1 as V3_1
 from compose.const import COMPOSEFILE_V3_2 as V3_2
 from compose.const import COMPOSEFILE_V3_3 as V3_3
 from compose.const import IS_WINDOWS_PLATFORM
-from compose.utils import nanoseconds_from_time_seconds
 from tests import mock
 from tests import unittest
 
@@ -4210,52 +4209,103 @@ class BuildPathTest(unittest.TestCase):
 
 class HealthcheckTest(unittest.TestCase):
     def test_healthcheck(self):
-        service_dict = make_service_dict(
+        config_dict = config.load(
-            'test',
+            build_config_details({
-            {'healthcheck': {
+                'version': '2.3',
-                'test': ['CMD', 'true'],
+                'services': {
-                'interval': '1s',
+                    'test': {
-                'timeout': '1m',
+                        'image': 'busybox',
-                'retries': 3,
+                        'healthcheck': {
-                'start_period': '10s'
+                            'test': ['CMD', 'true'],
-            }},
+                            'interval': '1s',
-            '.',
+                            'timeout': '1m',
+                            'retries': 3,
+                            'start_period': '10s',
+                        }
+                    }
+                }
+
+            })
         )
 
-        assert service_dict['healthcheck'] == {
+        serialized_config = yaml.load(serialize_config(config_dict))
+        serialized_service = serialized_config['services']['test']
+
+        assert serialized_service['healthcheck'] == {
             'test': ['CMD', 'true'],
-            'interval': nanoseconds_from_time_seconds(1),
+            'interval': '1s',
-            'timeout': nanoseconds_from_time_seconds(60),
+            'timeout': '1m',
             'retries': 3,
-            'start_period': nanoseconds_from_time_seconds(10)
+            'start_period': '10s'
         }
 
     def test_disable(self):
-        service_dict = make_service_dict(
+        config_dict = config.load(
-            'test',
+            build_config_details({
-            {'healthcheck': {
+                'version': '2.3',
-                'disable': True,
+                'services': {
-            }},
+                    'test': {
-            '.',
+                        'image': 'busybox',
+                        'healthcheck': {
+                            'disable': True,
+                        }
+                    }
+                }
+
+            })
         )
 
-        assert service_dict['healthcheck'] == {
+        serialized_config = yaml.load(serialize_config(config_dict))
+        serialized_service = serialized_config['services']['test']
+
+        assert serialized_service['healthcheck'] == {
             'test': ['NONE'],
         }
 
     def test_disable_with_other_config_is_invalid(self):
         with pytest.raises(ConfigurationError) as excinfo:
-            make_service_dict(
+            config.load(
-                'invalid-healthcheck',
+                build_config_details({
-                {'healthcheck': {
+                    'version': '2.3',
-                    'disable': True,
+                    'services': {
-                    'interval': '1s',
+                        'invalid-healthcheck': {
-                }},
+                            'image': 'busybox',
-                '.',
+                            'healthcheck': {
+                                'disable': True,
+                                'interval': '1s',
+                            }
+                        }
+                    }
+
+                })
             )
 
         assert 'invalid-healthcheck' in excinfo.exconly()
-        assert 'disable' in excinfo.exconly()
+        assert '"disable: true" cannot be combined with other options' in excinfo.exconly()
+
+    def test_healthcheck_with_invalid_test(self):
+        with pytest.raises(ConfigurationError) as excinfo:
+            config.load(
+                build_config_details({
+                    'version': '2.3',
+                    'services': {
+                        'invalid-healthcheck': {
+                            'image': 'busybox',
+                            'healthcheck': {
+                                'test': ['true'],
+                                'interval': '1s',
+                                'timeout': '1m',
+                                'retries': 3,
+                                'start_period': '10s',
+                            }
+                        }
+                    }
+
+                })
+            )
+
+        assert 'invalid-healthcheck' in excinfo.exconly()
+        assert 'the first item must be either NONE, CMD or CMD-SHELL' in excinfo.exconly()
 
 
 class GetDefaultConfigFilesTestCase(unittest.TestCase):