From 4a9846b91184fcd95947c12e95a6d1d8ceb6a928 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 6 Jul 2016 20:02:40 -0700 Subject: [PATCH 1/8] Begin work on multi-topic help listings --- certbot/cli.py | 84 +++++++++++++++++++++++++++----------------------- 1 file changed, 46 insertions(+), 38 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 35b3b74ae..fe3a26c87 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -497,16 +497,23 @@ class HelpfulArgumentParser(object): pass return True - def add(self, topic, *args, **kwargs): + def add(self, topics, *args, **kwargs): """Add a new command line argument. - :param str: help topic this should be listed under, can be None for - "always documented" + :param topics: str or [str] help topic(s) this should be listed under, + or None for "always documented" :param list *args: the names of this argument flag :param dict **kwargs: various argparse settings for this argument """ + if isinstance(topics, list): + # if this flag can be listed in multiple sections, try to pick the one + # that the user has asked for help about + topic = self.help_arg if self.help_arg in topics else topics[0] + else: + topic = topics # there's only one + if self.detect_defaults: kwargs = self.modify_kwargs_for_default_detection(**kwargs) @@ -607,6 +614,34 @@ class HelpfulArgumentParser(object): else: return dict([(t, t == chosen_topic) for t in self.help_topics]) +def _add_all_groups(helpful): + helpful.add_group("automation", description="Arguments for automating execution & other tweaks") + helpful.add_group("security", description="Security parameters & server settings") + helpful.add_group( + "testing", description="The following flags are meant for " + "testing purposes only! Do NOT change them, unless you " + "really know what you're doing!") + # VERBS + helpful.add_group( + "renew", description="The 'renew' subcommand will attempt to renew all" + " certificates (or more precisely, certificate lineages) you have" + " previously obtained if they are close to expiry, and print a" + " summary of the results. By default, 'renew' will reuse the options" + " used to create obtain or most recently successfully renew each" + " certificate lineage. You can try it with `--dry-run` first. For" + " more fine-grained control, you can renew individual lineages with" + " the `certonly` subcommand. Hooks are available to run commands " + " before and after renewal; see" + " https://certbot.eff.org/docs/using.html#renewal for more information on these.") + helpful.add_group("certonly", 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='Options for the "plugins" subcommand') + helpful.add_group("config_changes", + description="Options for showing a history of config changes") + helpful.add_group("paths", description="Arguments changing execution paths & servers") + def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: disable=too-many-statements """Returns parsed command line arguments. @@ -622,6 +657,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis # pylint: disable=too-many-statements helpful = HelpfulArgumentParser(args, plugins, detect_defaults) + _add_all_groups(helpful) # --help is automatically provided by argparse helpful.add( @@ -678,11 +714,8 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help="Domain names to apply. For multiple domains you can use " "multiple -d flags or enter a comma separated list of domains " "as a parameter.") - helpful.add_group( - "automation", - description="Arguments for automating execution & other tweaks") helpful.add( - "automation", "--keep-until-expiring", "--keep", "--reinstall", + ["automation", "renew"], "--keep-until-expiring", "--keep", "--reinstall", dest="reinstall", action="store_true", help="If the requested cert matches an existing cert, always keep the " "existing one until it is due for renewal (for the " @@ -721,23 +754,19 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "(both can be renewed in parallel)") helpful.add( "automation", "--os-packages-only", action="store_true", - help="(letsencrypt-auto only) install OS package dependencies and then stop") + help="(certbot-auto only) install OS package dependencies and then stop") helpful.add( "automation", "--no-self-upgrade", action="store_true", - help="(letsencrypt-auto only) prevent the letsencrypt-auto script from" + help="(certbot-auto only) prevent the certbot-auto script from" " upgrading itself to newer released versions") helpful.add( "automation", "-q", "--quiet", dest="quiet", action="store_true", help="Silence all output except errors. Useful for automation via cron." " Implies --non-interactive.") - helpful.add_group( - "testing", description="The following flags are meant for " - "testing purposes only! Do NOT change them, unless you " - "really know what you're doing!") helpful.add( "testing", "--debug", action="store_true", - help="Show tracebacks in case of errors, and allow letsencrypt-auto " + help="Show tracebacks in case of errors, and allow certbot-auto " "execution on experimental platforms") helpful.add( "testing", "--no-verify-ssl", action="store_true", @@ -754,8 +783,6 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "testing", "--break-my-certs", action="store_true", help="Be willing to replace or renew valid certs with invalid " "(testing/staging) certs") - helpful.add_group( - "security", description="Security parameters & server settings") helpful.add( "security", "--rsa-key-size", type=int, metavar="N", default=flag_default("rsa_key_size"), help=config_help("rsa_key_size")) @@ -805,18 +832,6 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help="Require that all configuration files are owned by the current " "user; only needed if your config is somewhere unsafe like /tmp/") - helpful.add_group( - "renew", description="The 'renew' subcommand will attempt to renew all" - " certificates (or more precisely, certificate lineages) you have" - " previously obtained if they are close to expiry, and print a" - " summary of the results. By default, 'renew' will reuse the options" - " used to create obtain or most recently successfully renew each" - " certificate lineage. You can try it with `--dry-run` first. For" - " more fine-grained control, you can renew individual lineages with" - " the `certonly` subcommand. Hooks are available to run commands " - " before and after renewal; see" - " https://certbot.eff.org/docs/using.html#renewal for more information on these.") - helpful.add( "renew", "--pre-hook", help="Command to be run in a shell before obtaining any certificates. Intended" @@ -859,13 +874,6 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis def _create_subparsers(helpful): - helpful.add_group("certonly", 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") - helpful.add_group("config_changes", - description="Options for showing a history of config changes") helpful.add("config_changes", "--num", type=int, help="How many past revisions you want to be displayed") helpful.add( @@ -901,8 +909,6 @@ def _paths_parser(helpful): verb = helpful.verb if verb == "help": verb = helpful.help_arg - helpful.add_group( - "paths", description="Arguments changing execution paths & servers") cph = "Path to where cert is saved (with auth --csr), installed from or revoked." section = "paths" @@ -948,12 +954,14 @@ def _paths_parser(helpful): def _plugins_parsing(helpful, plugins): + # It's nuts, but there are two "plugins" topics. Somehow this works helpful.add_group( - "plugins", description="Certbot client supports an " + "plugins", description="Plugin Selection: Certbot client supports an " "extensible plugins architecture. See '%(prog)s plugins' for a " "list of all installed plugins and their names. You can force " "a particular plugin by setting options provided below. Running " "--help will list flags specific to that plugin.") + helpful.add( "plugins", "-a", "--authenticator", help="Authenticator plugin name.") helpful.add( From d96278505e6b01f115c597aaf035452924714425 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 6 Jul 2016 20:07:33 -0700 Subject: [PATCH 2/8] Fix test --- certbot/tests/cli_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 896550837..388c38152 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -113,7 +113,7 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods out = self._help_output(['--help', 'plugins']) self.assertTrue("--manual-test-mode" not in out) self.assertTrue("--prepare" in out) - self.assertTrue("Plugin options" in out) + self.assertTrue('"plugins" subcommand' in out) out = self._help_output(['--help', 'install']) self.assertTrue("--cert-path" in out) From 204c3f0dfbd242751efaf7a36ced35f7e5d16ca5 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 7 Jul 2016 15:11:23 -0700 Subject: [PATCH 3/8] Start using multi-topic CLI flag documentation --- certbot/cli.py | 53 +++++++++++++++++++++------------------ certbot/tests/cli_test.py | 8 ++++++ 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index fe3a26c87..5b7f6ca8a 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -88,8 +88,8 @@ More detailed help: the available topics are: all, automation, paths, security, testing, or any of the subcommands or - plugins (certonly, install, register, nginx, apache, standalone, webroot, - etc.) + plugins (certonly, renew, install, register, nginx, apache, standalone, + webroot, etc.) """ @@ -390,8 +390,7 @@ class HelpfulArgumentParser(object): if getattr(parsed_args, arg): raise errors.Error( ("Conflicting values for displayer." - " {0} conflicts with dialog_mode").format(arg) - ) + " {0} conflicts with dialog_mode").format(arg)) if parsed_args.validate_hooks: hooks.validate_hooks(parsed_args) @@ -676,9 +675,18 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "which ones are required if it finds one missing") helpful.add( None, "--dialog", dest="dialog_mode", action="store_true", - help="Run using dialog") + help="Run using interactive dialog menus") helpful.add( - None, "--dry-run", action="store_true", dest="dry_run", + [None, "run", "certonly"], + "-d", "--domains", "--domain", dest="domains", + metavar="DOMAIN", action=_DomainsAction, default=[], + help="Domain names to apply. For multiple domains you can use " + "multiple -d flags or enter a comma separated list of domains " + "as a parameter.") + + helpful.add( + [None, "testing", "renew", "certonly"], + "--dry-run", action="store_true", dest="dry_run", help="Perform a test run of the client, obtaining test (invalid) certs" " but not saving them to disk. This can currently only be used" " with the 'certonly' and 'renew' subcommands. \nNote: Although --dry-run" @@ -705,17 +713,9 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "with an existing registration, such as the e-mail address, " "should be updated, rather than registering a new account.") helpful.add(None, "-m", "--email", help=config_help("email")) - # positional arg shadows --domains, instead of appending, and - # --domains is useful, because it can be stored in config - #for subparser in parser_run, parser_auth, parser_install: - # subparser.add_argument("domains", nargs="*", metavar="domain") - helpful.add(None, "-d", "--domains", "--domain", dest="domains", - metavar="DOMAIN", action=_DomainsAction, default=[], - help="Domain names to apply. For multiple domains you can use " - "multiple -d flags or enter a comma separated list of domains " - "as a parameter.") helpful.add( - ["automation", "renew"], "--keep-until-expiring", "--keep", "--reinstall", + ["automation", "renew", "certonly", "run"], + "--keep-until-expiring", "--keep", "--reinstall", dest="reinstall", action="store_true", help="If the requested cert matches an existing cert, always keep the " "existing one until it is due for renewal (for the " @@ -729,14 +729,16 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis version="%(prog)s {0}".format(certbot.__version__), help="show program's version number and exit") helpful.add( - "automation", "--force-renewal", "--renew-by-default", + ["automation", "renew"], + "--force-renewal", "--renew-by-default", action="store_true", dest="renew_by_default", help="If a certificate " "already exists for the requested domains, renew it now, " "regardless of whether it is near expiry. (Often " "--keep-until-expiring is more appropriate). Also implies " "--expand.") helpful.add( - "automation", "--allow-subset-of-names", action="store_true", + ["automation", "renew"], + "--allow-subset-of-names", action="store_true", help="When performing domain validation, do not consider it a failure " "if authorizations can not be obtained for a strict subset of " "the requested domains. This may be useful for allowing renewals for " @@ -760,7 +762,8 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help="(certbot-auto only) prevent the certbot-auto script from" " upgrading itself to newer released versions") helpful.add( - "automation", "-q", "--quiet", dest="quiet", action="store_true", + ["automation", "renew"], + "-q", "--quiet", dest="quiet", action="store_true", help="Silence all output except errors. Useful for automation via cron." " Implies --non-interactive.") @@ -970,15 +973,17 @@ def _plugins_parsing(helpful, plugins): "plugins", "--configurator", help="Name of the plugin that is " "both an authenticator and an installer. Should not be used " "together with --authenticator or --installer.") - helpful.add("plugins", "--apache", action="store_true", + helpful.add(["plugins", "certonly", "run", "install"], + "--apache", action="store_true", help="Obtain and install certs using Apache") - helpful.add("plugins", "--nginx", action="store_true", + helpful.add(["plugins", "certonly", "run", "install"], + "--nginx", action="store_true", help="Obtain and install certs using Nginx") - helpful.add("plugins", "--standalone", action="store_true", + helpful.add(["plugins", "certonly"], "--standalone", action="store_true", help='Obtain certs using a "standalone" webserver.') - helpful.add("plugins", "--manual", action="store_true", + helpful.add(["plugins", "certonly"], "--manual", action="store_true", help='Provide laborious manual instructions for obtaining a cert') - helpful.add("plugins", "--webroot", action="store_true", + helpful.add(["plugins", "certonly"], "--webroot", action="store_true", help='Obtain certs by placing files in a webroot directory.') # things should not be reorder past/pre this comment: diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 388c38152..63c682b20 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -115,6 +115,14 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods self.assertTrue("--prepare" in out) self.assertTrue('"plugins" subcommand' in out) + # test multiple topics + out = self._help_output(['-h', 'renew']) + self.assertTrue("--keep" in out) + out = self._help_output(['-h', 'automation']) + self.assertTrue("--keep" in out) + out = self._help_output(['-h', 'revoke']) + self.assertTrue("--keep" not in out) + out = self._help_output(['--help', 'install']) self.assertTrue("--cert-path" in out) self.assertTrue("--key-path" in out) From ecd1ca4645fe594df805d1e4b6afa2c6b4441245 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 8 Jul 2016 10:05:56 -0700 Subject: [PATCH 4/8] grammatical improvement --- certbot/cli.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 5b7f6ca8a..89dacac0d 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -828,13 +828,10 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "security", "--no-staple-ocsp", action="store_false", help="Do not automatically enable OCSP Stapling.", dest="staple", default=None) - - helpful.add( "security", "--strict-permissions", action="store_true", help="Require that all configuration files are owned by the current " "user; only needed if your config is somewhere unsafe like /tmp/") - helpful.add( "renew", "--pre-hook", help="Command to be run in a shell before obtaining any certificates. Intended" From c3244df951fefdbc5158f59383f6695978dee1a6 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 8 Jul 2016 10:12:57 -0700 Subject: [PATCH 5/8] more doc improvements --- certbot-apache/certbot_apache/configurator.py | 2 +- certbot/cli.py | 9 ++++----- certbot/interfaces.py | 2 +- certbot/plugins/standalone.py | 2 +- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index d1c2b7165..50fd10895 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -83,7 +83,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ - description = "Apache Web Server - Alpha" + description = "Apache Web Server plugin - Beta" @classmethod def add_parser_arguments(cls, add): diff --git a/certbot/cli.py b/certbot/cli.py index 89dacac0d..97dfc6d7e 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -766,7 +766,10 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "-q", "--quiet", dest="quiet", action="store_true", help="Silence all output except errors. Useful for automation via cron." " Implies --non-interactive.") - + # overwrites server, handled in HelpfulArgumentParser.parse_args() + helpful.add("testing", "--test-cert", "--staging", action='store_true', dest='staging', + help='Use the staging server to obtain test (invalid) certs; equivalent' + ' to --server ' + constants.STAGING_URI) helpful.add( "testing", "--debug", action="store_true", help="Show tracebacks in case of errors, and allow certbot-auto " @@ -947,10 +950,6 @@ def _paths_parser(helpful): help="Logs directory.") add("paths", "--server", default=flag_default("server"), help=config_help("server")) - # overwrites server, handled in HelpfulArgumentParser.parse_args() - add("testing", "--test-cert", "--staging", action='store_true', dest='staging', - help='Use the staging server to obtain test (invalid) certs; equivalent' - ' to --server ' + constants.STAGING_URI) def _plugins_parsing(helpful, plugins): diff --git a/certbot/interfaces.py b/certbot/interfaces.py index e4e62e0a2..d4b391378 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -227,7 +227,7 @@ class IConfig(zope.interface.Interface): "Location of renewal configuration file.") no_verify_ssl = zope.interface.Attribute( - "Disable SSL certificate verification.") + "Disable verification of the ACME server's certificate.") tls_sni_01_port = zope.interface.Attribute( "Port number to perform tls-sni-01 challenge. " "Boulder in testing mode defaults to 5001.") diff --git a/certbot/plugins/standalone.py b/certbot/plugins/standalone.py index 8e1cb72a4..97aca351a 100644 --- a/certbot/plugins/standalone.py +++ b/certbot/plugins/standalone.py @@ -154,7 +154,7 @@ class Authenticator(common.Plugin): rely on any existing server program. """ - description = "Automatically use a temporary webserver" + description = "Spin up a temporary webserver" def __init__(self, *args, **kwargs): super(Authenticator, self).__init__(*args, **kwargs) From 894af917778761c2cd3e9143402f71dc1070fe0f Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 8 Jul 2016 14:09:26 -0700 Subject: [PATCH 6/8] Fix test --- certbot/plugins/disco_test.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/certbot/plugins/disco_test.py b/certbot/plugins/disco_test.py index cef6ede8f..75544ee9b 100644 --- a/certbot/plugins/disco_test.py +++ b/certbot/plugins/disco_test.py @@ -55,9 +55,7 @@ class PluginEntryPointTest(unittest.TestCase): name, PluginEntryPoint.entry_point_to_plugin_name(entry_point)) def test_description(self): - self.assertEqual( - "Automatically use a temporary webserver", - self.plugin_ep.description) + self.assertTrue("temporary webserver" in self.plugin_ep.description) def test_description_with_name(self): self.plugin_ep.plugin_cls = mock.MagicMock(description="Desc") From df42f69d8ce646367aa382f982a2124031567b29 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 26 Jul 2016 18:08:21 -0700 Subject: [PATCH 7/8] Address review comments --- certbot/cli.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 9537af5f3..a93e072e3 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -503,7 +503,9 @@ class HelpfulArgumentParser(object): """Add a new command line argument. :param topics: str or [str] help topic(s) this should be listed under, - or None for "always documented" + or None for "always documented". The first entry + determines where the flag lives in the "--help all" + output (None -> "optional arguments"). :param list *args: the names of this argument flag :param dict **kwargs: various argparse settings for this argument @@ -671,7 +673,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis None, "-t", "--text", dest="text_mode", action="store_true", help="Use the text output instead of the curses UI.") helpful.add( - None, "-n", "--non-interactive", "--noninteractive", + [None, "automation"], "-n", "--non-interactive", "--noninteractive", dest="noninteractive_mode", action="store_true", help="Run without ever asking for user input. This may require " "additional command line flags; the client will try to explain " @@ -701,7 +703,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis " if they are defined because they may be necessary to accurately simulate" " renewal. --renew-hook commands are not called.") helpful.add( - None, "--register-unsafely-without-email", action="store_true", + ["register", "automation"], "--register-unsafely-without-email", action="store_true", help="Specifying this flag enables registering an account with no " "email address. This is strongly discouraged, because in the " "event of key loss or account compromise you will irrevocably " @@ -740,7 +742,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis "--keep-until-expiring is more appropriate). Also implies " "--expand.") helpful.add( - ["automation", "renew"], + ["automation", "renew", "certonly"], "--allow-subset-of-names", action="store_true", help="When performing domain validation, do not consider it a failure " "if authorizations can not be obtained for a strict subset of " From fbadf7550c81d317aa70a8238389709884049e61 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 26 Jul 2016 19:20:02 -0700 Subject: [PATCH 8/8] Also put -q in certonly --- certbot/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/cli.py b/certbot/cli.py index a93e072e3..22787392d 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -767,7 +767,7 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False): # pylint: dis help="(certbot-auto only) prevent the certbot-auto script from" " upgrading itself to newer released versions") helpful.add( - ["automation", "renew"], + ["automation", "renew", "certonly"], "-q", "--quiet", dest="quiet", action="store_true", help="Silence all output except errors. Useful for automation via cron." " Implies --non-interactive.")