incorporate feedback

Signed-off-by: Dustin Falgout <dustin@falgout.us>

docker/utils/utils.py

@@ -236,7 +236,7 @@ def convert_port_bindings(port_bindings):
     for k, v in six.iteritems(port_bindings):
         key = str(k)
         if '/' not in key:
-            key = key + '/tcp'
+            key += '/tcp'
         if isinstance(v, list):
             result[key] = [_convert_port_binding(binding) for binding in v]
         else:
@@ -434,7 +434,7 @@ def parse_bytes(s):
         s = 0
     else:
         if s[-2:-1].isalpha() and s[-1].isalpha():
-            if (s[-1] == "b" or s[-1] == "B"):
+            if s[-1] == "b" or s[-1] == "B":
                 s = s[:-1]
         units = BYTE_UNITS
         suffix = s[-1].lower()
@@ -467,17 +467,20 @@ def parse_bytes(s):
     return s
 
 
-def host_config_type_error(param_value=None, expected=None):
-    return TypeError(
-        'Invalid type for {0} param: expected {1} but found {2}'.format(param_value, expected,
-                                                                        type(param_value))
-    )
+def host_config_type_error(param, param_value, expected):
+    error_msg = 'Invalid type for {0} param: expected {1} but found {2}'
+    return TypeError(error_msg.format(param, expected, type(param_value)))
 
 
-def host_config_version_error(param=None, version=None):
-    return errors.InvalidVersion(
-        '{0} param is not supported in API versions < {1}'.format(param, version)
-    )
+def host_config_version_error(param, version, less_than=True):
+    operator = '<' if less_than else '>'
+    error_msg = '{0} param is not supported in API versions {1} {2}'
+    return errors.InvalidVersion(error_msg.format(param, operator, version))
+
+
+def host_config_value_error(param, param_value):
+    error_msg = 'Invalid value for {0} param: {1}'
+    return ValueError(error_msg.format(param, param_value))
 
 
 def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
@@ -509,20 +512,21 @@ def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
     if memswap_limit is not None:
         if isinstance(memswap_limit, six.string_types):
             memswap_limit = parse_bytes(memswap_limit)
+
         host_config['MemorySwap'] = memswap_limit
 
     if mem_swappiness is not None:
         if version_lt(version, '1.20'):
             raise host_config_version_error('mem_swappiness', '1.20')
         if not isinstance(mem_swappiness, int):
-            raise host_config_type_error(mem_swappiness, 'int')
+            raise host_config_type_error(
+                'mem_swappiness', mem_swappiness, 'int'
+            )
 
         host_config['MemorySwappiness'] = mem_swappiness
 
     if pid_mode not in (None, 'host'):
-        raise errors.DockerException(
-            'Invalid value for pid param: {0}'.format(pid_mode)
-        )
+        raise host_config_value_error('pid_mode', pid_mode)
     elif pid_mode:
         host_config['PidMode'] = pid_mode
 
@@ -533,10 +537,9 @@ def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
         host_config['Privileged'] = privileged
 
     if oom_kill_disable:
-        if version_lt(version, '1.19'):
-            raise errors.InvalidVersion(
-                'oom_kill_disable param not supported for API version < 1.19'
-            )
+        if version_lt(version, '1.20'):
+            raise host_config_version_error('oom_kill_disable', '1.19')
+
         host_config['OomKillDisable'] = oom_kill_disable
 
     if publish_all_ports:
@@ -554,6 +557,11 @@ def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
         host_config['NetworkMode'] = 'default'
 
     if restart_policy:
+        if not isinstance(restart_policy, dict):
+            raise host_config_type_error(
+                'restart_policy', restart_policy, 'dict'
+            )
+
         host_config['RestartPolicy'] = restart_policy
 
     if cap_add:
@@ -568,6 +576,7 @@ def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
     if group_add:
         if version_lt(version, '1.20'):
             raise host_config_version_error('group_add', '1.20')
+
         host_config['GroupAdd'] = [six.text_type(grp) for grp in group_add]
 
     if dns is not None:
@@ -575,21 +584,21 @@ def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
 
     if security_opt is not None:
         if not isinstance(security_opt, list):
-            raise host_config_type_error(security_opt, 'list')
+            raise host_config_type_error('security_opt', security_opt, 'list')
+
         host_config['SecurityOpt'] = security_opt
 
     if volumes_from is not None:
         if isinstance(volumes_from, six.string_types):
             volumes_from = volumes_from.split(',')
+
         host_config['VolumesFrom'] = volumes_from
 
     if binds is not None:
         host_config['Binds'] = convert_volume_binds(binds)
 
     if port_bindings is not None:
-        host_config['PortBindings'] = convert_port_bindings(
-            port_bindings
-        )
+        host_config['PortBindings'] = convert_port_bindings(port_bindings)
 
     if extra_hosts is not None:
         if isinstance(extra_hosts, dict):
@@ -604,9 +613,7 @@ def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
         if isinstance(links, dict):
             links = six.iteritems(links)
 
-        formatted_links = [
-            '{0}:{1}'.format(k, v) for k, v in sorted(links)
-        ]
+        formatted_links = ['{0}:{1}'.format(k, v) for k, v in sorted(links)]
 
         host_config['Links'] = formatted_links
 
@@ -624,7 +631,7 @@ def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
 
     if ulimits is not None:
         if not isinstance(ulimits, list):
-            raise host_config_type_error(ulimits, 'list')
+            raise host_config_type_error('ulimits', ulimits, 'list')
         host_config['Ulimits'] = []
         for l in ulimits:
             if not isinstance(l, Ulimit):
@@ -634,13 +641,16 @@ def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
     if log_config is not None:
         if not isinstance(log_config, LogConfig):
             if not isinstance(log_config, dict):
-                raise host_config_type_error(log_config, 'LogConfig')
+                raise host_config_type_error(
+                    'log_config', log_config, 'LogConfig'
+                )
             log_config = LogConfig(**log_config)
+
         host_config['LogConfig'] = log_config
 
     if cpu_quota:
         if not isinstance(cpu_quota, int):
-            raise host_config_type_error(cpu_quota, 'int')
+            raise host_config_type_error('cpu_quota', cpu_quota, 'int')
         if version_lt(version, '1.19'):
             raise host_config_version_error('cpu_quota', '1.19')
 
@@ -648,7 +658,7 @@ def create_host_config(binds=None, port_bindings=None, lxc_conf=None,
 
     if cpu_period:
         if not isinstance(cpu_period, int):
-            raise host_config_type_error(cpu_period, 'int')
+            raise host_config_type_error('cpu_period', cpu_period, 'int')
         if version_lt(version, '1.19'):
             raise host_config_version_error('cpu_period', '1.19')
 

tests/integration/container_test.py

@@ -365,21 +365,11 @@ class CreateContainerTest(helpers.BaseTestCase):
         self.assertIn('MemorySwappiness', host_config)
 
     def test_create_host_config_exception_raising(self):
-        with self.assertRaises(TypeError) as exc:
-            ctnr1 = self.client.create_container(
-                BUSYBOX, 'true',
-                host_config=self.client.create_host_config(mem_swappiness='40')
-            )
-        self.assertIn('Invalid type for', exc.exception.response.text)
-        self.client.remove_container(ctnr1.get('Id', ctnr1), force=True)
+        self.assertRaises(TypeError,
+                          self.client.create_host_config, mem_swappiness='40')
 
-        with self.assertRaises(docker.errors.InvalidVersion) as exc:
-            ctnr2 = self.client.create_container(
-                BUSYBOX, 'true',
-                host_config=self.client.create_host_config(version='1.18', mem_swappiness=40)
-            )
-        self.assertIn('param is not supported in', exc.exception.response.text)
-        self.client.remove_container(ctnr2.get('Id', ctnr2), force=True)
+        self.assertRaises(ValueError,
+                          self.client.create_host_config, pid_mode='40')
 
 
 class VolumeBindTest(helpers.BaseTestCase):

tests/unit/api_test.py

@@ -314,8 +314,7 @@ class DockerApiTest(DockerClientTest):
         self.assertIn('SecurityOpt', result)
         self.assertEqual(result['SecurityOpt'], security_opt)
         self.assertRaises(
-            docker.errors.DockerException, self.client.create_host_config,
-            security_opt='wrong'
+            TypeError, self.client.create_host_config, security_opt='wrong'
         )