Merge lp:~allenap/maas/cannot-set-database-port--bug-1642200 into lp:~maas-committers/maas/trunk

[Gavin Panella]

Deal with string input for database port.

Previously database ports passed in from the command-line were not being parsed as integers. This change also improves the display of validation errors when using local_config_set; previously a full traceback was being printed.

[Brendan Donegan (brendan-donegan)]

LGTM!

review: Approve

=== modified file 'src/maasserver/config.py'
--- src/maasserver/config.py	2016-11-10 09:46:34 +0000
+++ src/maasserver/config.py	2016-11-16 11:16:50 +0000
@@ -9,7 +9,7 @@
 
 from os import path
 
-from formencode.validators import RangeValidator
+from formencode.validators import Int
 from provisioningserver.config import (
     Configuration,
     ConfigurationFile,
@@ -44,7 +44,7 @@
         UnicodeString(if_missing="localhost", accept_python=False))
     database_port = ConfigurationOption(
         "database_port", "The port of the PostgreSQL database.",
-        RangeValidator(if_missing=5432, accept_python=False, min=1, max=65535))
+        Int(if_missing=5432, accept_python=False, min=1, max=65535))
     database_name = ConfigurationOption(
         "database_name", "The name of the PostgreSQL database.",
         UnicodeString(if_missing="maasdb", accept_python=False))

=== modified file 'src/maasserver/management/commands/_config.py'
--- src/maasserver/management/commands/_config.py	2015-12-10 10:31:31 +0000
+++ src/maasserver/management/commands/_config.py	2016-11-16 11:16:50 +0000
@@ -14,7 +14,11 @@
 from optparse import make_option
 import sys
 
-from django.core.management.base import BaseCommand
+from django.core.management.base import (
+    BaseCommand,
+    CommandError,
+)
+import formencode
 from maascli.utils import parse_docstring
 from maasserver.config import RegionConfiguration
 from provisioningserver.config import ConfigurationOption
@@ -208,4 +212,9 @@
             for name, option in gen_configuration_options():
                 value = options.get(name)
                 if value is not None:
-                    setattr(config, name, value)
+                    try:
+                        setattr(config, name, value)
+                    except formencode.Invalid as error:
+                        message = str(error).rstrip(".")
+                        raise CommandError("%s: %s." % (
+                            name.replace("_", "-"), message))

=== modified file 'src/maasserver/management/commands/tests/test_config.py'
--- src/maasserver/management/commands/tests/test_config.py	2016-11-10 22:45:28 +0000
+++ src/maasserver/management/commands/tests/test_config.py	2016-11-16 11:16:50 +0000
@@ -9,6 +9,7 @@
 import json
 
 from django.core.management import call_command
+from django.core.management.base import CommandError
 from maasserver.config import RegionConfiguration
 from maasserver.management.commands import _config as config
 from maasserver.testing.config import RegionConfigurationFixture
@@ -120,7 +121,8 @@
         else:
             value = factory.make_name("foobar")
 
-        stdio = call_set(**{self.option.dest: value})
+        # Values are coming from the command-line so stringify.
+        stdio = call_set(**{self.option.dest: str(value)})
 
         # Nothing is echoed back to the user.
         self.assertThat(stdio.getOutput(), Equals(""))
@@ -136,6 +138,33 @@
                 Contains(str(value)))
 
 
+class TestConfigurationSet_DatabasePort(MAASTestCase):
+    """Tests for setting the database port.
+
+    Setting the port is slightly special because the other options are mostly
+    (at the time of writing) defined using `UnicodeString` validators, roughly
+    meaning that anything goes, but the port is defined with `Int`.
+    """
+
+    def test__exception_when_port_is_not_an_integer(self):
+        self.useFixture(RegionConfigurationFixture())
+        error = self.assertRaises(CommandError, call_set, database_port="foo")
+        self.assertThat(str(error), Equals(
+            "database-port: Please enter an integer value."))
+
+    def test__exception_when_port_is_too_low(self):
+        self.useFixture(RegionConfigurationFixture())
+        error = self.assertRaises(CommandError, call_set, database_port=0)
+        self.assertThat(str(error), Equals(
+            "database-port: Please enter a number that is 1 or greater."))
+
+    def test__exception_when_port_is_too_high(self):
+        self.useFixture(RegionConfigurationFixture())
+        error = self.assertRaises(CommandError, call_set, database_port=2**16)
+        self.assertThat(str(error), Equals(
+            "database-port: Please enter a number that is 65535 or smaller."))
+
+
 class TestConfigurationCommon(MAASTestCase):
 
     is_string = IsInstance(str)

=== modified file 'src/maasserver/tests/test_config.py'
--- src/maasserver/tests/test_config.py	2016-11-10 09:46:34 +0000
+++ src/maasserver/tests/test_config.py	2016-11-16 11:16:50 +0000
@@ -84,7 +84,9 @@
             example_value = factory.make_name(self.option)
         else:
             example_value = factory.pick_port()
-        setattr(config, self.option, example_value)
+        # Argument values will most often be passed in from the command-line,
+        # so convert to a string before use to reflect that usage.
+        setattr(config, self.option, str(example_value))
         self.assertEqual(example_value, getattr(config, self.option))
         # It's also stored in the configuration database.
         self.assertEqual({self.option: example_value}, config.store)