Merge pull request #2608 from dnephin/more_cyclomatic_reduction

More cyclomatic complexity reduction

compose/config/sort_services.py

@@ -23,28 +23,31 @@ def get_source_name_from_network_mode(network_mode, source_type):
     return net_name
 
 
+def get_service_names(links):
+    return [link.split(':')[0] for link in links]
+
+
+def get_service_names_from_volumes_from(volumes_from):
+    return [volume_from.source for volume_from in volumes_from]
+
+
+def get_service_dependents(service_dict, services):
+    name = service_dict['name']
+    return [
+        service for service in services
+        if (name in get_service_names(service.get('links', [])) or
+            name in get_service_names_from_volumes_from(service.get('volumes_from', [])) or
+            name == get_service_name_from_network_mode(service.get('network_mode')) or
+            name in service.get('depends_on', []))
+    ]
+
+
 def sort_service_dicts(services):
     # Topological sort (Cormen/Tarjan algorithm).
     unmarked = services[:]
     temporary_marked = set()
     sorted_services = []
 
-    def get_service_names(links):
-        return [link.split(':')[0] for link in links]
-
-    def get_service_names_from_volumes_from(volumes_from):
-        return [volume_from.source for volume_from in volumes_from]
-
-    def get_service_dependents(service_dict, services):
-        name = service_dict['name']
-        return [
-            service for service in services
-            if (name in get_service_names(service.get('links', [])) or
-                name in get_service_names_from_volumes_from(service.get('volumes_from', [])) or
-                name == get_service_name_from_network_mode(service.get('network_mode')) or
-                name in service.get('depends_on', []))
-        ]
-
     def visit(n):
         if n['name'] in temporary_marked:
             if n['name'] in get_service_names(n.get('links', [])):

compose/service.py

@@ -162,11 +162,11 @@ class Service(object):
         - starts containers until there are at least `desired_num` running
         - removes all stopped containers
         """
-        if self.custom_container_name() and desired_num > 1:
+        if self.custom_container_name and desired_num > 1:
             log.warn('The "%s" service is using the custom container name "%s". '
                      'Docker requires each container to have a unique name. '
                      'Remove the custom name to scale the service.'
-                     % (self.name, self.custom_container_name()))
+                     % (self.name, self.custom_container_name))
 
         if self.specifies_host_port():
             log.warn('The "%s" service specifies a port on the host. If multiple containers '
@@ -496,10 +496,6 @@ class Service(object):
     def get_volumes_from_names(self):
         return [s.source.name for s in self.volumes_from if isinstance(s.source, Service)]
 
-    def get_container_name(self, number, one_off=False):
-        # TODO: Implement issue #652 here
-        return build_container_name(self.project, self.name, number, one_off)
-
     # TODO: this would benefit from github.com/docker/docker/pull/14699
     # to remove the need to inspect every container
     def _next_container_number(self, one_off=False):
@@ -561,13 +557,10 @@ class Service(object):
             for k in DOCKER_CONFIG_KEYS if k in self.options)
         container_options.update(override_options)
 
-        if self.custom_container_name() and not one_off:
-            container_options['name'] = self.custom_container_name()
-        elif not container_options.get('name'):
+        if not container_options.get('name'):
             container_options['name'] = self.get_container_name(number, one_off)
 
-        if 'detach' not in container_options:
-            container_options['detach'] = True
+        container_options.setdefault('detach', True)
 
         # If a qualified hostname was given, split it into an
         # unqualified hostname and a domainname unless domainname
@@ -581,16 +574,9 @@ class Service(object):
             container_options['domainname'] = parts[2]
 
         if 'ports' in container_options or 'expose' in self.options:
-            ports = []
-            all_ports = container_options.get('ports', []) + self.options.get('expose', [])
-            for port_range in all_ports:
-                internal_range, _ = split_port(port_range)
-                for port in internal_range:
-                    port = str(port)
-                    if '/' in port:
-                        port = tuple(port.split('/'))
-                    ports.append(port)
-            container_options['ports'] = ports
+            container_options['ports'] = build_container_ports(
+                container_options,
+                self.options)
 
         container_options['environment'] = merge_environment(
             self.options.get('environment'),
@@ -714,9 +700,16 @@ class Service(object):
             '{0}={1}'.format(LABEL_ONE_OFF, "True" if one_off else "False")
         ]
 
+    @property
     def custom_container_name(self):
         return self.options.get('container_name')
 
+    def get_container_name(self, number, one_off=False):
+        if self.custom_container_name and not one_off:
+            return self.custom_container_name
+
+        return build_container_name(self.project, self.name, number, one_off)
+
     def remove_image(self, image_type):
         if not image_type or image_type == ImageType.none:
             return False
@@ -1031,3 +1024,18 @@ def format_environment(environment):
             return key
         return '{key}={value}'.format(key=key, value=value)
     return [format_env(*item) for item in environment.items()]
+
+# Ports
+
+
+def build_container_ports(container_options, options):
+    ports = []
+    all_ports = container_options.get('ports', []) + options.get('expose', [])
+    for port_range in all_ports:
+        internal_range, _ = split_port(port_range)
+        for port in internal_range:
+            port = str(port)
+            if '/' in port:
+                port = tuple(port.split('/'))
+            ports.append(port)
+    return ports

tests/integration/service_test.py

@@ -782,7 +782,7 @@ class ServiceTest(DockerClientTestCase):
         results in warning output.
         """
         service = self.create_service('app', container_name='custom-container')
-        self.assertEqual(service.custom_container_name(), 'custom-container')
+        self.assertEqual(service.custom_container_name, 'custom-container')
 
         service.scale(3)
 
@@ -963,7 +963,7 @@ class ServiceTest(DockerClientTestCase):
 
     def test_custom_container_name(self):
         service = self.create_service('web', container_name='my-web-container')
-        self.assertEqual(service.custom_container_name(), 'my-web-container')
+        self.assertEqual(service.custom_container_name, 'my-web-container')
 
         container = create_and_start_container(service)
         self.assertEqual(container.name, 'my-web-container')

tox.ini

@@ -45,7 +45,7 @@ directory = coverage-html
 # Allow really long lines for now
 max-line-length = 140
 # Set this high for now
-max-complexity = 12
+max-complexity = 11
 exclude = compose/packages
 
 [pytest]