create_node: simplify validation, clean up tests

src/allmydata/scripts/create_node.py

@@ -1,5 +1,5 @@
 import os
-from twisted.python import usage
+from twisted.python.usage import UsageError
 from allmydata.scripts.common import BasedirOptions, NoDefaultBasedirOptions
 from allmydata.scripts.default_nodedir import _default_nodedir
 from allmydata.util.assertutil import precondition
@@ -23,21 +23,35 @@ WHERE_OPTS = [
     ("port", None, None, "Specify the server endpoint to listen on for this node."),
 ]
 
-def validate_where_options(options):
+def validate_where_options(o):
-    if options['hostname'] and options['port']:
+    # --location and --port: overrides all others, rejects all others
-        raise usage.UsageError("The --hostname option cannot be used with the --port option.")
+    if o['location'] and not o['port']:
-    if options['hostname'] and options['location']:
+        raise UsageError("--location must be used with --port")
-        raise usage.UsageError("The --hostname option cannot be used with the --location option.")
+    if o['port'] and not o['location']:
-    if not options['hostname'] and (options['location'] and not options['port']):
+        raise UsageError("--port must be used with --location")
-        raise usage.UsageError("The --location option must be used with the --port option.")
+
-    if not options['hostname'] and (options['port'] and not options['location']):
+    if o['location'] and o['port']:
-        raise usage.UsageError("The --port option must be used with the --location option.")
+        if o['hostname']:
-    if (options['listen'] != "tcp") and options['hostname']:
+            raise UsageError("--hostname cannot be used with --location/--port")
-        raise usage.UsageError("The listener type must be TCP to use --hostname option.")
+        # TODO: really, we should reject an explicit --listen= option (we
-    if options['listen'] == "tcp" and not options['hostname']:
+        # want them to omit it entirely, because --location/--port would
-        raise usage.UsageError("--listen=tcp requires --hostname=")
+        # override anything --listen= might allocate). For now, just let it
-    if options['listen'] not in ["tcp", "tor", "i2p"]:
+        # pass, because that allows us to use --listen=tcp as the default in
-        raise usage.UsageError("The listener type must set to one of: tcp, tor, i2p.")
+        # optParameters, which (I think) gets included in the rendered --help
+        # output, which is useful. In the future, let's reconsider the value
+        # of that --help text (or achieve that documentation in some other
+        # way), change the default to None, complain here if it's not None,
+        # then change parseArgs() to transform the None into "tcp"
+    else:
+        # no --location and --port? expect --listen= (maybe the default), and --listen=tcp requires --hostname
+        listeners = o['listen'].split(",")
+        if 'tcp' in listeners and not o['hostname']:
+            raise UsageError("--listen=tcp requires --hostname=")
+        if 'tcp' not in listeners and o['hostname']:
+            raise UsageError("--listen= must be tcp to use --hostname")
+        for l in listeners:
+            if l not in ["tcp", "tor", "i2p"]:
+                raise UsageError("--listen= must be: tcp, tor, i2p")
 
 class _CreateBaseOptions(BasedirOptions):
     optFlags = [

src/allmydata/test/cli/test_create.py

@@ -50,14 +50,15 @@ class Config(unittest.TestCase):
     @defer.inlineCallbacks
     def test_node(self):
         basedir = self.mktemp()
-        rc, out, err = yield run_cli("create-node", basedir)
+        rc, out, err = yield run_cli("create-node", "--hostname=foo", basedir)
         cfg = self.read_config(basedir)
         self.assertEqual(cfg.getboolean("node", "reveal-IP-address"), True)
 
     @defer.inlineCallbacks
     def test_node_hide_ip(self):
         basedir = self.mktemp()
-        rc, out, err = yield run_cli("create-node", "--hide-ip", basedir)
+        rc, out, err = yield run_cli("create-node", "--hide-ip",
+                                     "--hostname=foo", basedir)
         cfg = self.read_config(basedir)
         self.assertEqual(cfg.getboolean("node", "reveal-IP-address"), False)
 
@@ -109,31 +110,33 @@ class Config(unittest.TestCase):
         e = self.assertRaises(usage.UsageError,
                               parse_cli,
                               "create-node", "--port=unix:/var/tahoe/socket")
-        self.assertEqual(str(e), "The --port option must be used with the --location option.")
+        self.assertEqual(str(e), "--port must be used with --location")
 
     def test_node_location_only(self):
         e = self.assertRaises(usage.UsageError,
                               parse_cli,
                               "create-node", "--location=tor:myservice.onion:12345")
-        self.assertEqual(str(e), "The --location option must be used with the --port option.")
+        self.assertEqual(str(e), "--location must be used with --port")
 
-    @defer.inlineCallbacks
+    def test_introducer_no_hostname(self):
-    def test_introducer(self):
         basedir = self.mktemp()
-        rc, out, err = yield run_cli("create-introducer", basedir)
+        e = self.assertRaises(usage.UsageError, parse_cli,
-        cfg = self.read_config(basedir)
+                              "create-introducer", basedir)
-        self.assertEqual(cfg.getboolean("node", "reveal-IP-address"), True)
+        self.assertEqual(str(e), "--listen=tcp requires --hostname=")
 
     @defer.inlineCallbacks
     def test_introducer_hide_ip(self):
         basedir = self.mktemp()
-        rc, out, err = yield run_cli("create-introducer", "--hide-ip", basedir)
+        rc, out, err = yield run_cli("create-introducer", "--hide-ip",
+                                     "--hostname=foo", basedir)
         cfg = self.read_config(basedir)
         self.assertEqual(cfg.getboolean("node", "reveal-IP-address"), False)
 
     @defer.inlineCallbacks
     def test_introducer_hostname(self):
         basedir = self.mktemp()
-        rc, out, err = yield run_cli("create-introducer", "--hostname=computer", basedir)
+        rc, out, err = yield run_cli("create-introducer",
+                                     "--hostname=foo", basedir)
         cfg = self.read_config(basedir)
-        self.assertTrue("computer" in cfg.get("node", "tub.location"))
+        self.assertTrue("foo" in cfg.get("node", "tub.location"))
+        self.assertEqual(cfg.getboolean("node", "reveal-IP-address"), True)