Merge lp:~allenap/maas/fix-mutable-argument-defaults into lp:~maas-committers/maas/trunk

[Gavin Panella]

Replace mutable arguments.

Mutable arguments are generally a bad thing. One pattern in particular
has appeared in MAAS; here's a reduced example:

  @attr.s
  class DiscoveredMachineInterface(AttrHelperMixin):
      tags = attr.ib(convert=convert_list(str), default=[])

The problem here is that every new instance will share the same initial
list of tags.

  >>> dmi = DiscoveredMachineInterface("mac")
  >>> dmi.tags.append("foo")
  >>> dmi
  DiscoveredMachineInterface(
      mac_address='mac', vid=-1, tags=['foo'], boot=False)

  >>> dmi2 = DiscoveredMachineInterface("mac2")
  >>> dmi2
  DiscoveredMachineInterface(
      mac_address='mac2', vid=-1, tags=['foo'], boot=False)

The fix here is to use a default *factory*:

  tags = attr.ib(convert=convert_list(str), default=attr.Factory(list))

I discovered these problems using findmutargs.py from:

  lp:~allenap/+junk/find-bad-argument-defaults

The change in this branch to `logsomething.py` was made entirely to
allow `findmutargs.py` to load it without setting it off.

[Mike Pontillo (mpontillo)]

Looks good.

review: Approve

=== modified file 'src/maasserver/forms/ephemeral.py'
--- src/maasserver/forms/ephemeral.py	2017-03-02 09:41:37 +0000
+++ src/maasserver/forms/ephemeral.py	2017-03-29 14:02:03 +0000
@@ -46,7 +46,8 @@
         self.fields['testing_scripts'] = MultipleChoiceField(
             required=False, initial=None, choices=choices)
 
-    def __init__(self, instance, user, data={}, **kwargs):
+    def __init__(self, instance, user, data=None, **kwargs):
+        data = {} if data is None else data
         super().__init__(data=data, **kwargs)
         self._set_up_script_fields()
         self.instance = instance

=== modified file 'src/provisioningserver/drivers/pod/__init__.py'
--- src/provisioningserver/drivers/pod/__init__.py	2017-03-23 18:20:00 +0000
+++ src/provisioningserver/drivers/pod/__init__.py	2017-03-29 14:02:03 +0000
@@ -129,7 +129,7 @@
     """Discovered machine interface."""
     mac_address = attr.ib(convert=str)
     vid = attr.ib(convert=int, default=-1)
-    tags = attr.ib(convert=convert_list(str), default=[])
+    tags = attr.ib(convert=convert_list(str), default=attr.Factory(list))
     boot = attr.ib(convert=bool, default=False)
 
 
@@ -140,7 +140,7 @@
     serial = attr.ib(convert=convert_obj(str, optional=True))
     size = attr.ib(convert=int)
     block_size = attr.ib(convert=int, default=512)
-    tags = attr.ib(convert=convert_list(str), default=[])
+    tags = attr.ib(convert=convert_list(str), default=attr.Factory(list))
     id_path = attr.ib(convert=convert_obj(str, optional=True), default=None)
 
 
@@ -155,8 +155,9 @@
     block_devices = attr.ib(
         convert=convert_list(DiscoveredMachineBlockDevice))
     power_state = attr.ib(convert=str, default='unknown')
-    power_parameters = attr.ib(convert=convert_obj(dict), default={})
-    tags = attr.ib(convert=convert_list(str), default=[])
+    power_parameters = attr.ib(
+        convert=convert_obj(dict), default=attr.Factory(dict))
+    tags = attr.ib(convert=convert_list(str), default=attr.Factory(list))
 
 
 @attr.s
@@ -184,11 +185,12 @@
     hints = attr.ib(convert=convert_obj(DiscoveredPodHints))
     local_disks = attr.ib(convert=int, default=-1)
     capabilities = attr.ib(
-        convert=convert_list(str), default=[Capabilities.FIXED_LOCAL_STORAGE])
+        convert=convert_list(str), default=attr.Factory(
+            lambda: [Capabilities.FIXED_LOCAL_STORAGE]))
     machines = attr.ib(
-        convert=convert_list(DiscoveredMachine), default=[])
+        convert=convert_list(DiscoveredMachine), default=attr.Factory(list))
     storage = attr.ib(
-        convert=convert_list(DiscoveredStorage), default=[])
+        convert=convert_list(DiscoveredStorage), default=attr.Factory(list))
 
 
 @attr.s

=== modified file 'src/provisioningserver/drivers/storage/__init__.py'
--- src/provisioningserver/drivers/storage/__init__.py	2017-03-22 20:13:59 +0000
+++ src/provisioningserver/drivers/storage/__init__.py	2017-03-29 14:02:03 +0000
@@ -104,7 +104,7 @@
     """Discovered storage volume."""
     size = attr.ib(convert=int)
     block_size = attr.ib(convert=int, default=512)
-    tags = attr.ib(convert=convert_list(str), default=[])
+    tags = attr.ib(convert=convert_list(str), default=attr.Factory(list))
 
 
 @attr.s
@@ -112,8 +112,8 @@
     """Discovered storage information."""
     size = attr.ib(convert=int)
     volumes = attr.ib(
-        convert=convert_list(DiscoveredVolume), default=[])
-    parameters = attr.ib(convert=convert_obj(dict), default={})
+        convert=convert_list(DiscoveredVolume), default=attr.Factory(list))
+    parameters = attr.ib(convert=convert_obj(dict), default=attr.Factory(dict))
 
     # When a Pod discovers storage it sets the driver_type so MAAS knows
     # which storage driver to call to perform the action. When a storage

=== modified file 'src/provisioningserver/logger/testing/logsomething.py'
--- src/provisioningserver/logger/testing/logsomething.py	2016-12-07 12:46:14 +0000
+++ src/provisioningserver/logger/testing/logsomething.py	2017-03-29 14:02:03 +0000
@@ -27,62 +27,67 @@
 import twisted.python.log
 
 
-modes = provisioningserver.logger.LoggingMode
-
-parser = argparse.ArgumentParser()
-parser.add_argument("--name", required=True)
-parser.add_argument("--verbosity", type=int, required=True)
-# Resets the verbosity at runtime (after initially setting it).
-parser.add_argument("--set-verbosity", type=int, required=False)
-parser.add_argument(
-    "--mode", type=modes.__getitem__, help=" or ".join(
-        mode.name for mode in modes))
-options = parser.parse_args()
-
-# Configure logging. This is the main entry-point.
-provisioningserver.logger.configure(
-    verbosity=options.verbosity, mode=options.mode)
-
-if options.set_verbosity is not None:
-    provisioningserver.logger.set_verbosity(options.set_verbosity)
-
-# Simulate what `twistd` does when passed `--logger=...` in Twisted 16.0. In
-# 16.4 something similar happens, but globalLogBeginner.beginLoggingTo() is
-# called without going via `twisted.python.log`.
-if options.mode == modes.TWISTD:
-    emitter = provisioningserver.logger.EventLogger()
-    twisted.python.log.startLoggingWithObserver(emitter)
-
-# Twisted, new.
-twisted.logger.Logger(options.name).debug("From `twisted.logger`.")
-twisted.logger.Logger(options.name).info("From `twisted.logger`.")
-twisted.logger.Logger(options.name).warn("From `twisted.logger`.")
-twisted.logger.Logger(options.name).error("From `twisted.logger`.")
-
-# Twisted, legacy.
-twisted.python.log.msg("From `twisted.python.log`.", system=options.name)
-
-# Standard library.
-logging.getLogger(options.name).debug("From `logging`.")
-logging.getLogger(options.name).info("From `logging`.")
-logging.getLogger(options.name).warning("From `logging`.")
-logging.getLogger(options.name).error("From `logging`.")
-
-# Standard library, "maas" logger.
-maaslog = provisioningserver.logger.get_maas_logger(options.name)
-maaslog.debug("From `get_maas_logger`.")
-maaslog.info("From `get_maas_logger`.")
-maaslog.warning("From `get_maas_logger`.")
-maaslog.error("From `get_maas_logger`.")
-
-# Standard IO.
-print("Printing to stdout.", file=sys.stdout, flush=True)
-print("Printing to stderr.", file=sys.stderr, flush=True)
-
-# Warnings.
-warnings.formatwarning = lambda message, *_, **__: str(message)
-warnings.warn("This is a warning!")
-
-# Make sure everything is flushed.
-sys.stdout.flush()
-sys.stderr.flush()
+def main():
+    modes = provisioningserver.logger.LoggingMode
+
+    parser = argparse.ArgumentParser()
+    parser.add_argument("--name", required=True)
+    parser.add_argument("--verbosity", type=int, required=True)
+    # Resets the verbosity at runtime (after initially setting it).
+    parser.add_argument("--set-verbosity", type=int, required=False)
+    parser.add_argument(
+        "--mode", type=modes.__getitem__, help=" or ".join(
+            mode.name for mode in modes))
+    options = parser.parse_args()
+
+    # Configure logging. This is the main entry-point.
+    provisioningserver.logger.configure(
+        verbosity=options.verbosity, mode=options.mode)
+
+    if options.set_verbosity is not None:
+        provisioningserver.logger.set_verbosity(options.set_verbosity)
+
+    # Simulate what `twistd` does when passed `--logger=...` in Twisted 16.0.
+    # In 16.4 a similar thing happens, but globalLogBeginner.beginLoggingTo()
+    # is called without going via `twisted.python.log`.
+    if options.mode == modes.TWISTD:
+        emitter = provisioningserver.logger.EventLogger()
+        twisted.python.log.startLoggingWithObserver(emitter)
+
+    # Twisted, new.
+    twisted.logger.Logger(options.name).debug("From `twisted.logger`.")
+    twisted.logger.Logger(options.name).info("From `twisted.logger`.")
+    twisted.logger.Logger(options.name).warn("From `twisted.logger`.")
+    twisted.logger.Logger(options.name).error("From `twisted.logger`.")
+
+    # Twisted, legacy.
+    twisted.python.log.msg("From `twisted.python.log`.", system=options.name)
+
+    # Standard library.
+    logging.getLogger(options.name).debug("From `logging`.")
+    logging.getLogger(options.name).info("From `logging`.")
+    logging.getLogger(options.name).warning("From `logging`.")
+    logging.getLogger(options.name).error("From `logging`.")
+
+    # Standard library, "maas" logger.
+    maaslog = provisioningserver.logger.get_maas_logger(options.name)
+    maaslog.debug("From `get_maas_logger`.")
+    maaslog.info("From `get_maas_logger`.")
+    maaslog.warning("From `get_maas_logger`.")
+    maaslog.error("From `get_maas_logger`.")
+
+    # Standard IO.
+    print("Printing to stdout.", file=sys.stdout, flush=True)
+    print("Printing to stderr.", file=sys.stderr, flush=True)
+
+    # Warnings.
+    warnings.formatwarning = lambda message, *_, **__: str(message)
+    warnings.warn("This is a warning!")
+
+    # Make sure everything is flushed.
+    sys.stdout.flush()
+    sys.stderr.flush()
+
+
+if __name__ == '__main__':
+    main()