Merge lp:~jseutter/landscape-client/invalid-integer into lp:~landscape/landscape-client/trunk

[Jerry Seutter]

This branch addresses a bug caused by the exchange interval having an empty string in debconf. Trying to call landscape-config --exchange-interval "" resulted in a command line parse error. The fix changes the parsing to allow an empty string to be passed in, which causes the default setting to be used.

[Geoff Teale (tealeg)]

+1 Looks good to me.

review: Approve

Unmerged revisions

458. By Jerry Seutter on 2012-02-21

Adding a missing docstring.

457. By Jerry Seutter on 2012-02-21

exchange-interval, urgent-exchange-interval and ping-interval now accept
the empty string on the command line.

=== modified file 'landscape/broker/config.py'
--- landscape/broker/config.py	2012-02-06 21:04:44 +0000
+++ landscape/broker/config.py	2012-02-21 21:36:18 +0000
@@ -63,18 +63,19 @@
         parser.add_option("-k", "--ssl-public-key",
                           help="The public SSL key to verify the server. "
                                "Only used if the given URL is https.")
-        parser.add_option("--exchange-interval", default=15 * 60, type="int",
-                          metavar="INTERVAL",
+        parser.add_option("--exchange-interval", default=15 * 60,
+                          type="int_or_empty", metavar="INTERVAL",
                           help="The number of seconds between server "
-                               "exchanges.")
+                               "exchanges.",
+                          action="callback", callback=self.int_or_empty)
         parser.add_option("--urgent-exchange-interval", default=1 * 60,
-                          type="int", metavar="INTERVAL",
+                          type="int_or_empty", metavar="INTERVAL",
                           help="The number of seconds between urgent server "
                                "exchanges.")
         parser.add_option("--ping-url",
                           help="The URL to perform lightweight exchange "
                                "initiation with.")
-        parser.add_option("--ping-interval", default=30, type="int",
+        parser.add_option("--ping-interval", default=30, type="int_or_empty",
                           metavar="INTERVAL",
                           help="The number of seconds between pings.")
         parser.add_option("--http-proxy", metavar="URL",
@@ -133,3 +134,18 @@
 
         autodiscover = str(self.server_autodiscover).lower()
         self.server_autodiscover = (autodiscover == "true")
+
+        if self.exchange_interval is "":
+            self.exchange_interval = 15 * 60
+        else:
+            self.exchange_interval = int(self.exchange_interval)
+
+        if self.urgent_exchange_interval is "":
+            self.urgent_exchange_interval = 1 * 60
+        else:
+            self.urgent_exchange_interval = int(self.urgent_exchange_interval)
+
+        if self.ping_interval is "":
+            self.ping_interval = 30
+        else:
+            self.ping_interval = int(self.ping_interval)

=== modified file 'landscape/deployment.py'
--- landscape/deployment.py	2011-10-13 06:53:24 +0000
+++ landscape/deployment.py	2012-02-21 21:36:18 +0000
@@ -4,7 +4,8 @@
 from logging import (getLevelName, getLogger,
                      FileHandler, StreamHandler, Formatter)
 
-from optparse import OptionParser, SUPPRESS_HELP
+from copy import copy
+from optparse import OptionParser, Option, SUPPRESS_HELP
 from ConfigParser import ConfigParser, NoSectionError
 
 from landscape import VERSION
@@ -30,6 +31,23 @@
         handler.setFormatter(Formatter(format))
 
 
+def check_int_or_empty(option, opt, value):
+    """Validate the input of a int_or_empty parser type."""
+    if value == '':
+        return value
+    try:
+        return int(value)
+    except ValueError:
+        raise OptionValueError(
+            "option %s: invalid integer value: %r" % (opt, value))
+
+
+class OptionClass(Option):
+    TYPES = Option.TYPES + ("int_or_empty",)
+    TYPE_CHECKER = copy(Option.TYPE_CHECKER)
+    TYPE_CHECKER["int_or_empty"] = check_int_or_empty
+
+
 class BaseConfiguration(object):
     """Base class for configuration implementations.
 
@@ -230,7 +248,7 @@
             landscape-related programs accept. These include
               - C{config} (C{None})
         """
-        parser = OptionParser(version=VERSION)
+        parser = OptionParser(version=VERSION, option_class=OptionClass)
         parser.add_option("-c", "--config", metavar="FILE",
                           help="Use config from this file (any command line "
                                "options override settings from the file) "

=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2012-02-02 02:49:13 +0000
+++ landscape/tests/test_configuration.py	2012-02-21 21:36:18 +0000
@@ -1631,6 +1631,66 @@
                   # care for this test.  Mocker will ensure the tests
                   # we care about are done.
 
+    def test_empty_exchange_interval(self):
+        """
+        Make sure an empty string can be used for the exchange interval.
+        """
+        # Make sure no sysvconfig modifications are attempted
+        self.mocker.patch(SysVConfig)
+        self.mocker.replay()
+
+        args = ["--silent", "--no-start",
+                "--exchange-interval", "",]
+        config = self.get_config(args)
+        setup(config)
+        self.assertEqual(config.exchange_interval, 15 * 60)
+        self.assertConfigEqual(self.get_content(config),
+            "[client]\n"
+            "data_path = %s\n"
+            "url = https://landscape.canonical.com/message-system\n"
+            "server_autodiscover = False\n"
+             % config.data_path)
+
+    def test_empty_urgent_exchange_interval(self):
+        """
+        Make sure an empty string can be used for the urgent exchange interval.
+        """
+        # Make sure no sysvconfig modifications are attempted
+        self.mocker.patch(SysVConfig)
+        self.mocker.replay()
+
+        args = ["--silent", "--no-start",
+                "--urgent-exchange-interval", "",]
+        config = self.get_config(args)
+        setup(config)
+        self.assertEqual(config.urgent_exchange_interval, 60)
+        self.assertConfigEqual(self.get_content(config),
+            "[client]\n"
+            "data_path = %s\n"
+            "url = https://landscape.canonical.com/message-system\n"
+            "server_autodiscover = False\n"
+             % config.data_path)
+
+    def test_empty_ping_interval(self):
+        """
+        Make sure an empty string can be used for the ping interval.
+        """
+        # Make sure no sysvconfig modifications are attempted
+        self.mocker.patch(SysVConfig)
+        self.mocker.replay()
+
+        args = ["--silent", "--no-start",
+                "--ping-interval", "",]
+        config = self.get_config(args)
+        setup(config)
+        self.assertEqual(config.ping_interval, 30)
+        self.assertConfigEqual(self.get_content(config),
+            "[client]\n"
+            "data_path = %s\n"
+            "url = https://landscape.canonical.com/message-system\n"
+            "server_autodiscover = False\n"
+             % config.data_path)
+
 
 class RegisterFunctionTest(LandscapeTest):