Merge pull request #863 from letsencrypt/subargs

Improve CLI flag and help processing

.pylintrc

@@ -38,7 +38,7 @@ load-plugins=linter_plugin
 # --enable=similarities". If you want to run only the classes checker, but have
 # no Warning level messages displayed, use"--disable=all --enable=classes
 # --disable=W"
-disable=fixme,locally-disabled,abstract-class-not-used,bad-continuation,too-few-public-methods,no-self-use
+disable=fixme,locally-disabled,abstract-class-not-used,bad-continuation,too-few-public-methods,no-self-use,invalid-name
 # abstract-class-not-used cannot be disabled locally (at least in pylint 1.4.1)
 
 

letsencrypt/account.py

@@ -54,7 +54,7 @@ class Account(object):  # pylint: disable=too-few-public-methods
                 tz=pytz.UTC).replace(microsecond=0),
             creation_host=socket.getfqdn()) if meta is None else meta
 
-        self.id = hashlib.md5(  # pylint: disable=invalid-name
+        self.id = hashlib.md5(
             self.key.key.public_key().public_bytes(
                 encoding=serialization.Encoding.DER,
                 format=serialization.PublicFormat.SubjectPublicKeyInfo)

letsencrypt/cli.py

@@ -80,8 +80,8 @@ More detailed help:
   -h, --help [topic]    print this message, or detailed help on a topic;
                         the available topics are:
 
-   all, apache, automation, nginx, paths, security, testing, or any of the
-   subcommands
+   all, apache, automation, manual, nginx, paths, security, testing, or any of
+   the subcommands
 """
 
 
@@ -500,9 +500,6 @@ class SilentParser(object):  # pylint: disable=too-few-public-methods
         self.parser.add_argument(*args, **kwargs)
 
 
-HELP_TOPICS = ["all", "security", "paths", "automation", "testing", "plugins"]
-
-
 class HelpfulArgumentParser(object):
     """Argparse Wrapper.
 
@@ -524,6 +521,7 @@ class HelpfulArgumentParser(object):
         self.parser._add_config_file_help = False  # pylint: disable=protected-access
         self.silent_parser = SilentParser(self.parser)
 
+        self.verb = None
         self.args = self.preprocess_args(args)
         help1 = self.prescan_for_flag("-h", self.help_topics)
         help2 = self.prescan_for_flag("--help", self.help_topics)
@@ -540,13 +538,22 @@ class HelpfulArgumentParser(object):
     def preprocess_args(self, args):
         """Work around some limitations in argparse.
 
-        Currently, add the default verb "run" as a default.
+        Currently: add the default verb "run" as a default, and ensure that the
+        subcommand / verb comes last.
         """
+        if "-h" in args or "--help" in args:
+            # all verbs double as help arguments; don't get them confused
+            self.verb = "help"
+            return args
 
-        for token in args:
+        for i, token in enumerate(args):
             if token in VERBS:
-                return args
-        return ["run"] + args
+                reordered = args[:i] + args[i+1:] + [args[i]]
+                self.verb = token
+                return reordered
+
+        self.verb = "run"
+        return args + ["run"]
 
     def prescan_for_flag(self, flag, possible_arguments):
         """Checks cli input for flags.
@@ -724,77 +731,79 @@ def create_parser(plugins, args):
 
 # For now unfortunately this constant just needs to match the code below;
 # there isn't an elegant way to autogenerate it in time.
-VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes",
-         "plugins", "--help"]
-
+VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", "plugins"]
+HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + VERBS
 
 def _create_subparsers(helpful):
     subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND")
 
-    def add_subparser(name, func):  # pylint: disable=missing-docstring
-        subparser = subparsers.add_parser(
-            name, help=func.__doc__.splitlines()[0], description=func.__doc__)
+    def add_subparser(name):  # pylint: disable=missing-docstring
+        if name == "plugins":
+            func = plugins_cmd
+        else:
+            func = eval(name) # pylint: disable=eval-used
+        h = func.__doc__.splitlines()[0]
+        subparser = subparsers.add_parser(name, help=h, description=func.__doc__)
         subparser.set_defaults(func=func)
         return subparser
 
     # the order of add_subparser() calls is important: it defines the
     # order in which subparser names will be displayed in --help
-    add_subparser("run", run)
-    parser_auth = add_subparser("auth", auth)
-    parser_install = add_subparser("install", install)
-    parser_revoke = add_subparser("revoke", revoke)
-    parser_rollback = add_subparser("rollback", rollback)
-    add_subparser("config_changes", config_changes)
-    parser_plugins = add_subparser("plugins", plugins_cmd)
+    # these add_subparser objects return objects to which arguments could be
+    # attached, but they have annoying arg ordering constrains so we use
+    # groups instead: https://github.com/letsencrypt/letsencrypt/issues/820
+    for v in VERBS:
+        add_subparser(v)
 
-    parser_auth.add_argument(
-        "--csr", type=read_file, help="Path to a Certificate Signing "
-        "Request (CSR) in DER format.")
-    parser_auth.add_argument(
-        "--cert-path", default=flag_default("auth_cert_path"),
-        help="When using --csr this is where certificate is saved.")
-    parser_auth.add_argument(
-        "--chain-path", default=flag_default("auth_chain_path"),
-        help="When using --csr this is where certificate chain is saved.")
+    helpful.add_group("auth", description="Options for modifying how a cert is obtained")
+    helpful.add_group("install", description="Options for modifying how a cert is deployed")
+    helpful.add_group("revoke", description="Options for revocation of certs")
+    helpful.add_group("rollback", description="Options for reverting config changes")
+    helpful.add_group("plugins", description="Plugin options")
 
-    parser_install.add_argument(
-        "--cert-path", required=True, help="Path to a certificate that "
-        "is going to be installed.")
-    parser_install.add_argument(
-        "--key-path", required=True, help="Accompanying private key")
-    parser_install.add_argument(
-        "--chain-path", help="Accompanying path to a certificate chain.")
-    parser_revoke.add_argument(
-        "--cert-path", type=read_file, help="Revoke a specific certificate.",
-        required=True)
-    parser_revoke.add_argument(
-        "--key-path", type=read_file,
-        help="Revoke certificate using its accompanying key. Useful if "
-        "Account Key is lost.")
-
-    parser_rollback.add_argument(
+    helpful.add("auth",
+        "--csr", type=read_file, help="Path to a Certificate Signing Request (CSR) in DER format.")
+    helpful.add("rollback",
         "--checkpoints", type=int, metavar="N",
         default=flag_default("rollback_checkpoints"),
         help="Revert configuration N number of checkpoints.")
 
-    parser_plugins.add_argument(
+    helpful.add("plugins",
         "--init", action="store_true", help="Initialize plugins.")
-    parser_plugins.add_argument(
-        "--prepare", action="store_true",
-        help="Initialize and prepare plugins.")
-    parser_plugins.add_argument(
+    helpful.add("plugins",
+        "--prepare", action="store_true", help="Initialize and prepare plugins.")
+    helpful.add("plugins",
         "--authenticators", action="append_const", dest="ifaces",
-        const=interfaces.IAuthenticator,
-        help="Limit to authenticator plugins only.")
-    parser_plugins.add_argument(
+        const=interfaces.IAuthenticator, help="Limit to authenticator plugins only.")
+    helpful.add("plugins",
         "--installers", action="append_const", dest="ifaces",
         const=interfaces.IInstaller, help="Limit to installer plugins only.")
 
 
 def _paths_parser(helpful):
     add = helpful.add
+    verb = helpful.verb
     helpful.add_group(
         "paths", description="Arguments changing execution paths & servers")
+
+    cph = "Path to where cert is saved (with auth), installed (with install --csr) or revoked."
+    if verb == "auth":
+        add("paths", "--cert-path", default=flag_default("auth_cert_path"), help=cph)
+    elif verb == "revoke":
+        add("paths", "--cert-path", type=read_file, required=True, help=cph)
+    else:
+        add("paths", "--cert-path", help=cph, required=(verb == "install"))
+
+    # revoke --key-path reads a file, install --key-path takes a string
+    add("paths", "--key-path", type=((verb == "revoke" and read_file) or str),
+        required=(verb == "install"),
+        help="Path to private key for cert creation or revocation (if account key is missing)")
+
+    default_cp = None
+    if verb == "auth":
+        default_cp = flag_default("auth_chain_path")
+    add("paths", "--chain-path", default=default_cp,
+        help="Accompanying path to a certificate chain.")
     add("paths", "--config-dir", default=flag_default("config_dir"),
         help=config_help("config_dir"))
     add("paths", "--work-dir", default=flag_default("work_dir"),

letsencrypt/display/enhancements.py

@@ -11,7 +11,7 @@ from letsencrypt.display import util as display_util
 logger = logging.getLogger(__name__)
 
 # Define a helper function to avoid verbose code
-util = zope.component.getUtility  # pylint: disable=invalid-name
+util = zope.component.getUtility
 
 
 def ask(enhancement):

letsencrypt/display/ops.py

@@ -12,7 +12,7 @@ from letsencrypt.display import util as display_util
 logger = logging.getLogger(__name__)
 
 # Define a helper function to avoid verbose code
-util = zope.component.getUtility  # pylint: disable=invalid-name
+util = zope.component.getUtility
 
 
 def choose_plugin(prepared, question):

letsencrypt/log.py

@@ -25,7 +25,7 @@ class DialogHandler(logging.Handler):  # pylint: disable=too-few-public-methods
         logging.Handler.__init__(self, level)
         self.height = height
         self.width = width
-        # "dialog" collides with module name... pylint: disable=invalid-name
+        # "dialog" collides with module name...
         self.d = dialog.Dialog() if d is None else d
         self.lines = []
 

letsencrypt/plugins/common.py

@@ -23,10 +23,10 @@ def dest_namespace(name):
     """ArgumentParser dest namespace (prefix of all destinations)."""
     return name.replace("-", "_") + "_"
 
-private_ips_regex = re.compile(  # pylint: disable=invalid-name
+private_ips_regex = re.compile(
     r"(^127\.0\.0\.1)|(^10\.)|(^172\.1[6-9]\.)|"
     r"(^172\.2[0-9]\.)|(^172\.3[0-1]\.)|(^192\.168\.)")
-hostname_regex = re.compile(  # pylint: disable=invalid-name
+hostname_regex = re.compile(
     r"^(([a-z0-9]|[a-z0-9][a-z0-9\-]*[a-z0-9])\.)*[a-z]+$", re.IGNORECASE)
 
 
@@ -173,7 +173,7 @@ class Dvsni(object):
                             achall.chall.encode("token") + '.pem')
 
     def _setup_challenge_cert(self, achall, s=None):
-        # pylint: disable=invalid-name
+
         """Generate and write out challenge certificate."""
         cert_path = self.get_cert_path(achall)
         key_path = self.get_key_path(achall)

letsencrypt/tests/auth_handler_test.py

@@ -355,7 +355,7 @@ class GenChallengePathTest(unittest.TestCase):
 class MutuallyExclusiveTest(unittest.TestCase):
     """Tests for letsencrypt.auth_handler.mutually_exclusive."""
 
-    # pylint: disable=invalid-name,missing-docstring,too-few-public-methods
+    # pylint: disable=missing-docstring,too-few-public-methods
     class A(object):
         pass
 

letsencrypt/tests/cli_test.py

@@ -2,6 +2,7 @@
 import itertools
 import os
 import shutil
+import StringIO
 import traceback
 import tempfile
 import unittest
@@ -42,12 +43,54 @@ class CLITest(unittest.TestCase):
                     ret = cli.main(args)
         return ret, stdout, stderr, client
 
+    def _call_stdout(self, args):
+        """
+        Variant of _call that preserves stdout so that it can be mocked by the
+        caller.
+        """
+        from letsencrypt import cli
+        args = ['--text', '--config-dir', self.config_dir,
+                '--work-dir', self.work_dir, '--logs-dir', self.logs_dir,
+                '--agree-eula'] + args
+        with mock.patch('letsencrypt.cli.sys.stderr') as stderr:
+            with mock.patch('letsencrypt.cli.client') as client:
+                ret = cli.main(args)
+        return ret, None, stderr, client
+
+
     def test_no_flags(self):
-        self.assertRaises(SystemExit, self._call, [])
+        with mock.patch('letsencrypt.cli.run') as mock_run:
+            self._call([])
+            self.assertEqual(1, mock_run.call_count)
 
     def test_help(self):
         self.assertRaises(SystemExit, self._call, ['--help'])
-        self.assertRaises(SystemExit, self._call, ['--help all'])
+        self.assertRaises(SystemExit, self._call, ['--help', 'all'])
+        output = StringIO.StringIO()
+        with mock.patch('letsencrypt.cli.sys.stdout', new=output):
+            self.assertRaises(SystemExit, self._call_stdout, ['--help', 'all'])
+            out = output.getvalue()
+            self.assertTrue("--configurator" in out)
+            self.assertTrue("how a cert is deployed" in out)
+            self.assertTrue("--manual-test-mode" in out)
+            output.truncate(0)
+            self.assertRaises(SystemExit, self._call_stdout, ['-h', 'nginx'])
+            out = output.getvalue()
+            self.assertTrue("--nginx-ctl" in out)
+            self.assertTrue("--manual-test-mode" not in out)
+            self.assertTrue("--checkpoints" not in out)
+            output.truncate(0)
+            self.assertRaises(SystemExit, self._call_stdout, ['--help', 'plugins'])
+            out = output.getvalue()
+            self.assertTrue("--manual-test-mode" not in out)
+            self.assertTrue("--prepare" in out)
+            self.assertTrue("Plugin options" in out)
+            output.truncate(0)
+            self.assertRaises(SystemExit, self._call_stdout, ['-h'])
+            out = output.getvalue()
+            from letsencrypt import cli
+            self.assertTrue(cli.USAGE in out)
+
 
     def test_rollback(self):
         _, _, _, client = self._call(['rollback'])

letsencrypt/tests/log_test.py

@@ -8,7 +8,7 @@ import mock
 class DialogHandlerTest(unittest.TestCase):
 
     def setUp(self):
-        self.d = mock.MagicMock()  # pylint: disable=invalid-name
+        self.d = mock.MagicMock()
 
         from letsencrypt.log import DialogHandler
         self.handler = DialogHandler(height=2, width=6, d=self.d)

letsencrypt/tests/reverter_test.py

@@ -85,7 +85,7 @@ class ReverterCheckpointLocalTest(unittest.TestCase):
         self.assertEqual(read_in(self.config1), "directive-dir1")
 
     def test_multiple_registration_fail_and_revert(self):
-        # pylint: disable=invalid-name
+
         config3 = os.path.join(self.dir1, "config3.txt")
         update_file(config3, "Config3")
         config4 = os.path.join(self.dir2, "config4.txt")
@@ -173,7 +173,7 @@ class ReverterCheckpointLocalTest(unittest.TestCase):
         self.assertRaises(errors.ReverterError, self.reverter.recovery_routine)
 
     def test_recover_checkpoint_revert_temp_failures(self):
-        # pylint: disable=invalid-name
+
         mock_recover = mock.MagicMock(
             side_effect=errors.ReverterError("e"))
 
@@ -291,7 +291,7 @@ class TestFullCheckpointsReverter(unittest.TestCase):
             errors.ReverterError, self.reverter.rollback_checkpoints, "one")
 
     def test_rollback_finalize_checkpoint_valid_inputs(self):
-        # pylint: disable=invalid-name
+
         config3 = self._setup_three_checkpoints()
 
         # Check resulting backup directory
@@ -334,7 +334,7 @@ class TestFullCheckpointsReverter(unittest.TestCase):
 
     @mock.patch("letsencrypt.reverter.os.rename")
     def test_finalize_checkpoint_no_rename_directory(self, mock_rename):
-        # pylint: disable=invalid-name
+
         self.reverter.add_to_checkpoint(self.sets[0], "perm save")
         mock_rename.side_effect = OSError