Merge lp:~allenap/maas/drive-bys-2016-09-06 into lp:~maas-committers/maas/trunk

[Gavin Panella]

Eliminate most/all mutable default arguments, and other fixes.

Accumulated drive-by fixes.

[Mike Pontillo (mpontillo)]

Looks good; thanks for the fixes!

review: Approve

=== modified file 'src/maasserver/dhcp.py'
--- src/maasserver/dhcp.py	2016-07-30 01:17:54 +0000
+++ src/maasserver/dhcp.py	2016-09-07 21:49:08 +0000
@@ -235,10 +235,13 @@
     }
 
 
-def make_hosts_for_subnets(subnets, nodes_dhcp_snippets=[]):
+def make_hosts_for_subnets(subnets, nodes_dhcp_snippets: list=None):
     """Return list of host entries to create in the DHCP configuration for the
     given `subnets`.
     """
+    if nodes_dhcp_snippets is None:
+        nodes_dhcp_snippets = []
+
     def get_dhcp_snippets_for_interface(interface):
         dhcp_snippets = list()
         for dhcp_snippet in nodes_dhcp_snippets:
@@ -317,7 +320,7 @@
 
 def make_subnet_config(
         rack_controller, subnet, maas_dns_server, ntp_servers, default_domain,
-        failover_peer=None, subnets_dhcp_snippets=[]):
+        failover_peer=None, subnets_dhcp_snippets: list=None):
     """Return DHCP subnet configuration dict for a rack interface."""
     ip_network = subnet.get_ipnetwork()
     if subnet.dns_servers is not None and len(subnet.dns_servers) > 0:
@@ -327,6 +330,8 @@
         dns_servers = maas_dns_server
     else:
         dns_servers = ""
+    if subnets_dhcp_snippets is None:
+        subnets_dhcp_snippets = []
     return {
         'subnet': str(ip_network.network),
         'subnet_mask': str(ip_network.netmask),
@@ -369,7 +374,7 @@
 
 def get_dhcp_configure_for(
         ip_version, rack_controller, vlan, subnets, ntp_servers, domain,
-        dhcp_snippets=[]):
+        dhcp_snippets: list=None):
     """Get the DHCP configuration for `ip_version`."""
     # Circular imports.
     from maasserver.dns.zonegenerator import get_dns_server_address
@@ -399,6 +404,9 @@
     else:
         peer_name, peer_config = None, None
 
+    if dhcp_snippets is None:
+        dhcp_snippets = []
+
     subnets_dhcp_snippets = [
         dhcp_snippet for dhcp_snippet in dhcp_snippets
         if dhcp_snippet.subnet is not None]

=== modified file 'src/maasserver/fields.py'
--- src/maasserver/fields.py	2016-09-01 01:33:31 +0000
+++ src/maasserver/fields.py	2016-09-07 21:49:08 +0000
@@ -6,6 +6,7 @@
 __all__ = [
     "CIDRField",
     "EditableBinaryField",
+    "Field",
     "HostListFormField",
     "IPListFormField",
     "IPv4CIDRField",
@@ -35,7 +36,7 @@
 from django.db.models import (
     BinaryField,
     CharField,
-    Field,
+    Field as _BrokenField,
     GenericIPAddressField,
     IntegerField,
     Q,
@@ -64,6 +65,17 @@
 import psycopg2.extensions
 
 
+class Field(_BrokenField):
+    """Django's `Field` has a mutable default argument, hence is broken.
+
+    This fixes it.
+    """
+
+    def __init__(self, *args, validators=None, **kwargs):
+        kwargs["validators"] = [] if validators is None else validators
+        super(Field, self).__init__(*args, **kwargs)
+
+
 MAC_RE = re.compile(r'^\s*([0-9a-fA-F]{1,2}[:-]){5}[0-9a-fA-F]{1,2}\s*$')
 
 MAC_ERROR_MSG = "'%(value)s' is not a valid MAC address."

=== modified file 'src/maasserver/models/dnsdata.py'
--- src/maasserver/models/dnsdata.py	2016-05-11 19:01:48 +0000
+++ src/maasserver/models/dnsdata.py	2016-09-07 21:49:08 +0000
@@ -66,10 +66,11 @@
     """This is used to return non-address information for a hostname in a way
        that keeps life simple for the allers.  Rrset is a set of (ttl, rrtype,
        rrdata) tuples."""
-    def __init__(self, system_id=None, rrset=set(), node_type=None):
+
+    def __init__(self, system_id=None, rrset: set=None, node_type=None):
         self.system_id = system_id
         self.node_type = node_type
-        self.rrset = rrset.copy()
+        self.rrset = set() if rrset is None else rrset.copy()
 
     def __repr__(self):
         return "HostnameRRSetMapping(%r, %r, %r)" % (

=== modified file 'src/maasserver/models/staticipaddress.py'
--- src/maasserver/models/staticipaddress.py	2016-09-06 17:02:03 +0000
+++ src/maasserver/models/staticipaddress.py	2016-09-07 21:49:08 +0000
@@ -70,11 +70,13 @@
 class HostnameIPMapping:
     """This is used to return address information for a host in a way that
        keeps life simple for the callers."""
-    def __init__(self, system_id=None, ttl=None, ips=set(), node_type=None):
+
+    def __init__(
+            self, system_id=None, ttl=None, ips: set=None, node_type=None):
         self.system_id = system_id
         self.node_type = node_type
         self.ttl = ttl
-        self.ips = ips.copy()
+        self.ips = set() if ips is None else ips.copy()
 
     def __repr__(self):
         return "HostnameIPMapping(%r, %r, %r, %r)" % (

=== modified file 'src/maasserver/storage_layouts.py'
--- src/maasserver/storage_layouts.py	2016-04-12 22:25:44 +0000
+++ src/maasserver/storage_layouts.py	2016-09-07 21:49:08 +0000
@@ -55,8 +55,9 @@
     boot_size = BytesOrPercentageField(required=False)
     root_size = BytesOrPercentageField(required=False)
 
-    def __init__(self, node, params={}):
-        super(StorageLayoutBase, self).__init__(data=params)
+    def __init__(self, node, params: dict=None):
+        super(StorageLayoutBase, self).__init__(
+            data=({} if params is None else params))
         self.node = node
         self.block_devices = self._load_physical_block_devices()
         self.boot_disk = node.get_boot_disk()
@@ -501,8 +502,9 @@
     bcache0     99.5G       disk    ext4           /
     """
 
-    def __init__(self, node, params={}):
-        super(BcacheStorageLayout, self).__init__(node, params=params)
+    def __init__(self, node, params: dict=None):
+        super(BcacheStorageLayout, self).__init__(
+            node, params=({} if params is None else params))
         self.setup_cache_device_field()
 
     def configure_storage(self, allow_fallback):
@@ -556,10 +558,11 @@
     ]
 
 
-def get_storage_layout_for_node(name, node, params={}):
+def get_storage_layout_for_node(name, node, params: dict=None):
     """Get the storage layout object from its name."""
     if name in STORAGE_LAYOUTS:
-        return STORAGE_LAYOUTS[name][1](node, params=params)
+        return STORAGE_LAYOUTS[name][1](
+            node, params=({} if params is None else params))
     else:
         return None
 

=== modified file 'src/maasserver/tests/test_bootsources.py'
--- src/maasserver/tests/test_bootsources.py	2016-06-16 20:51:55 +0000
+++ src/maasserver/tests/test_bootsources.py	2016-09-07 21:49:08 +0000
@@ -88,10 +88,11 @@
         )
 
 
-def make_boot_image_mapping(image_specs=[]):
+def make_boot_image_mapping(image_specs=None):
     mapping = BootImageMapping()
-    for image_spec in image_specs:
-        mapping.setdefault(image_spec, {})
+    if image_specs is not None:
+        for image_spec in image_specs:
+            mapping.setdefault(image_spec, {})
     return mapping
 
 

=== modified file 'src/maasserver/utils/orm.py'
--- src/maasserver/utils/orm.py	2016-06-16 17:07:19 +0000
+++ src/maasserver/utils/orm.py	2016-09-07 21:49:08 +0000
@@ -44,6 +44,7 @@
 import threading
 from time import sleep
 import types
+from typing import Container
 
 from django.core.exceptions import (
     MultipleObjectsReturned,
@@ -634,15 +635,18 @@
             "Savepoints cannot be created outside of a transaction.")
 
 
-def in_transaction(connection=connection):
-    """Is `connection` in the midst of a transaction?
+def in_transaction(_connection=None):
+    """Is `_connection` in the midst of a transaction?
 
     This only enquires as to Django's perspective on the situation. It does
     not actually check that the database agrees with Django.
 
     :return: bool
     """
-    return connection.in_atomic_block
+    if _connection is None:
+        return connection.in_atomic_block
+    else:
+        return _connection.in_atomic_block
 
 
 def validate_in_transaction(connection):
@@ -826,7 +830,8 @@
     return specifier, op
 
 
-def parse_item_specifier_type(specifier, spec_types={}, separator=':'):
+def parse_item_specifier_type(
+        specifier, spec_types: Container=None, separator=':'):
     """
     Returns a tuple that splits the string int a specifier, and its specifier
     type.
@@ -835,8 +840,8 @@
     be found in the set, returns None in place of the specifier_type.
 
     :param specifier: The specifier string, such as "ip:10.0.0.1".
-    :param spec_types: A dict whose keys are strings that will be recognized
-        as specifier types.
+    :param spec_types: A container whose elements are strings that will be
+        recognized as specifier types.
     :param separator: Optional specifier. Defaults to ':'.
     :return: tuple
     """

=== modified file 'src/maasserver/websockets/protocol.py'
--- src/maasserver/websockets/protocol.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/websockets/protocol.py	2016-09-07 21:49:08 +0000
@@ -24,7 +24,6 @@
 )
 from django.contrib.auth.models import User
 from django.core.exceptions import ValidationError
-from django.utils.importlib import import_module
 from maasserver.eventloop import services
 from maasserver.utils.orm import transactional
 from maasserver.utils.threads import deferToDatabase
@@ -44,6 +43,7 @@
     Protocol,
 )
 from twisted.python import log
+from twisted.python.modules import getModule
 from twisted.web.server import NOT_DONE_YET
 
 
@@ -376,7 +376,7 @@
 
         Used by the protocol to validate the sessionid.
         """
-        return import_module(settings.SESSION_ENGINE)
+        return getModule(settings.SESSION_ENGINE).load()
 
     def cacheHandlers(self):
         """Cache all the websocket handlers."""

=== modified file 'src/maasserver/websockets/tests/test_protocol.py'
--- src/maasserver/websockets/tests/test_protocol.py	2016-09-02 20:31:39 +0000
+++ src/maasserver/websockets/tests/test_protocol.py	2016-09-07 21:49:08 +0000
@@ -737,12 +737,17 @@
         self.assertItemsEqual(ALL_HANDLERS, factory.handlers.keys())
 
     def test_get_SessionEngine_calls_import_module_with_SESSION_ENGINE(self):
-        mock_import = self.patch_autospec(protocol_module, "import_module")
+        getModule = self.patch_autospec(protocol_module, "getModule")
         factory = self.make_factory()
         factory.getSessionEngine()
-        self.assertThat(
-            mock_import,
-            MockCalledOnceWith(protocol_module.settings.SESSION_ENGINE))
+        # A reference to the module was obtained via getModule.
+        self.assertThat(
+            getModule, MockCalledOnceWith(
+                protocol_module.settings.SESSION_ENGINE))
+        # It was then loaded via that reference.
+        self.assertThat(
+            getModule.return_value.load,
+            MockCalledOnceWith())
 
     def test_getHandler_returns_None_on_missing_handler(self):
         factory = self.make_factory()

=== modified file 'src/metadataserver/fields.py'
--- src/metadataserver/fields.py	2016-07-30 01:17:54 +0000
+++ src/metadataserver/fields.py	2016-09-07 21:49:08 +0000
@@ -13,10 +13,8 @@
 )
 
 from django.db import connection
-from django.db.models import (
-    Field,
-    SubfieldBase,
-)
+from django.db.models import SubfieldBase
+from maasserver.fields import Field
 
 
 class Bin(bytes):

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2016-08-31 14:00:09 +0000
+++ src/metadataserver/tests/test_api.py	2016-09-07 21:49:08 +0000
@@ -226,7 +226,9 @@
     return MAASSensibleOAuthClient(get_node_init_user(), token)
 
 
-def call_signal(client=None, version='latest', files={}, headers={}, **kwargs):
+def call_signal(
+        client=None, version='latest', files: dict=None, headers: dict=None,
+        **kwargs):
     """Call the API's signal method.
 
     :param client: Optional client to POST with.  If omitted, will create
@@ -237,6 +239,10 @@
     :param **kwargs: Any other keyword parameters are passed on directly
         to the "signal" call.
     """
+    if files is None:
+        files = {}
+    if headers is None:
+        headers = {}
     if client is None:
         client = make_node_client(factory.make_Node(
             status=NODE_STATUS.COMMISSIONING))

=== modified file 'src/provisioningserver/cluster_config_command.py'
--- src/provisioningserver/cluster_config_command.py	2015-12-10 09:52:21 +0000
+++ src/provisioningserver/cluster_config_command.py	2016-09-07 21:49:08 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Change cluster controller configuration settings.
@@ -68,8 +68,8 @@
     uuid_group = parser.add_mutually_exclusive_group()
     uuid_group.add_argument(
         '--uuid', action='store', required=False,
-        help=('Change the cluster UUID. Pass AUTO to generate a new UUID if '
-              'one is not already set.'))
+        help=('Change the cluster UUID. Use --init instead to generate a '
+              'new UUID if one is not already set.'))
     uuid_group.add_argument(
         '--init', action='store_true', required=False,
         help=('Generate a new UUID for this cluster controller.'))

=== modified file 'src/provisioningserver/dns/tests/test_zoneconfig.py'
--- src/provisioningserver/dns/tests/test_zoneconfig.py	2016-08-22 15:26:25 +0000
+++ src/provisioningserver/dns/tests/test_zoneconfig.py	2016-09-07 21:49:08 +0000
@@ -38,7 +38,7 @@
 class HostnameIPMapping:
     """This is used to return address information for a host in a way that
        keeps life simple for the callers."""
-    def __init__(self, system_id=None, ttl=None, ips=set()):
+    def __init__(self, system_id=None, ttl=None, ips=frozenset()):
         self.system_id = system_id
         self.ttl = ttl
         self.ips = ips.copy()
@@ -55,7 +55,7 @@
     """This is used to return non-address information for a hostname in a way
        that keeps life simple for the allers.  Rrset is a set of (ttl, rrtype,
        rrdata) tuples."""
-    def __init__(self, system_id=None, rrset=set()):
+    def __init__(self, system_id=None, rrset=frozenset()):
         self.system_id = system_id
         self.rrset = rrset.copy()
 

=== modified file 'src/provisioningserver/rackdservices/tests/test_ntp.py'
--- src/provisioningserver/rackdservices/tests/test_ntp.py	2016-08-19 09:38:08 +0000
+++ src/provisioningserver/rackdservices/tests/test_ntp.py	2016-09-07 21:49:08 +0000
@@ -47,12 +47,13 @@
 @attr.s
 class FakeConnectionToRegionForGetControllerType(FakeConnectionToRegion):
 
-    controller_type = attr.ib(default={"is_region": False, "is_rack": False})
+    controller_type = attr.ib(
+        default=(("is_region", False), ("is_rack", False)))
 
     def callRemote(self, cmd, system_id):
         assert cmd is region.GetControllerType, (
             "cmd must be GetControllerType, got: %r" % (cmd,))
-        return succeed(self.controller_type)
+        return succeed(dict(self.controller_type))
 
 
 @attr.s
@@ -60,7 +61,8 @@
     """A stub `ClusterClientService` service."""
 
     addresses = attr.ib(default=frozenset(), convert=frozenset)
-    controller_type = attr.ib(default={"is_region": False, "is_rack": False})
+    controller_type = attr.ib(
+        default=(("is_region", False), ("is_rack", False)))
 
     def getAllClients(self):
         return [

=== modified file 'src/provisioningserver/refresh/__init__.py'
--- src/provisioningserver/refresh/__init__.py	2016-06-14 18:01:02 +0000
+++ src/provisioningserver/refresh/__init__.py	2016-09-07 21:49:08 +0000
@@ -78,7 +78,7 @@
 
 
 def signal(
-        url, creds, status, message, files={}, script_result=None,
+        url, creds, status, message, files: dict=None, script_result=None,
         extra_headers=None):
     """Send a node signal to a given maas_url."""
     if isinstance(status, int):
@@ -93,7 +93,8 @@
             script_result = str(script_result)
         params[b'script_result'] = script_result.encode("utf-8")
 
-    data, headers = encode_multipart_data(params, files)
+    data, headers = encode_multipart_data(
+        params, ({} if files is None else files))
 
     if extra_headers is not None:
         headers.update(extra_headers)

=== modified file 'src/provisioningserver/utils/services.py'
--- src/provisioningserver/utils/services.py	2016-08-31 17:59:55 +0000
+++ src/provisioningserver/utils/services.py	2016-09-07 21:49:08 +0000
@@ -139,6 +139,8 @@
         env = select_c_utf8_bytes_locale()
         log.msg("%s started." % self.description)
         args = self.getProcessParameters()
+        assert all(isinstance(arg, bytes) for arg in args), (
+            "Process arguments must all be bytes, got: %s" % repr(args))
         self.process = reactor.spawnProcess(
             self.protocol, args[0], args, env=env)
         return self.protocol.done.addErrback(
@@ -205,7 +207,7 @@
 
     # The last successfully recorded interfaces.
     _recorded = None
-    _monitored = set()
+    _monitored = frozenset()
 
     # Use a named filesystem lock to prevent more than one monitoring service
     # running on each host machine. This service attempts to acquire this lock

=== modified file 'src/provisioningserver/utils/tests/test_services.py'
--- src/provisioningserver/utils/tests/test_services.py	2016-08-31 17:59:55 +0000
+++ src/provisioningserver/utils/tests/test_services.py	2016-09-07 21:49:08 +0000
@@ -384,25 +384,25 @@
 class TrueProcessProtocolService(ProcessProtocolService):
 
     def getProcessParameters(self):
-        return ["/bin/true"]
+        return [b"/bin/true"]
 
 
 class FalseProcessProtocolService(ProcessProtocolService):
 
     def getProcessParameters(self):
-        return ["/bin/false"]
+        return [b"/bin/false"]
 
 
 class CatProcessProtocolService(ProcessProtocolService):
 
     def getProcessParameters(self):
-        return ["/bin/cat"]
+        return [b"/bin/cat"]
 
 
 class EchoProcessProtocolService(ProcessProtocolService):
 
     def getProcessParameters(self):
-        return ["/bin/echo", "{}\n"]
+        return [b"/bin/echo", b"{}\n"]
 
 
 class MockJSONProtocol(JSONPerLineProtocol):

=== modified file 'src/provisioningserver/utils/znums.py'
--- src/provisioningserver/utils/znums.py	2016-03-28 13:54:47 +0000
+++ src/provisioningserver/utils/znums.py	2016-09-07 21:49:08 +0000
@@ -19,19 +19,19 @@
 znums = dict(zip(zchars, count(0)))
 
 
-def from_int(num, chars=zchars):
-    parts, div = [], len(chars)
+def from_int(num, _chars=zchars):
+    parts, div = [], len(_chars)
     while num > 0:
         num, rem = divmod(num, div)
-        parts.append(chars[rem])
+        parts.append(_chars[rem])
     if len(parts) == 0:
-        return chars[0]
+        return _chars[0]
     else:
         return "".join(parts[::-1])
 
 
-def to_int(zs, nums=znums):
-    total, div = 0, len(nums)
+def to_int(zs, _nums=znums):
+    total, div = 0, len(_nums)
     for char, exp in zip(zs[::-1], count(0)):
-        total += nums[char] * (div ** exp)
+        total += _nums[char] * (div ** exp)
     return total