From 31fef196c0ed57a5cc6b1c4d409ff8097afbc716 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sun, 27 Sep 2015 01:15:35 +0000 Subject: [PATCH 01/24] --help is effectively a verb for CLI purposes... --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 7cb4a0458..711ac0048 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -679,7 +679,7 @@ 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"] + "plugins", "--help"] def _create_subparsers(helpful): From 001d37f9650d0c5f7521673f9fd075a7bd662b0c Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sun, 27 Sep 2015 02:41:55 +0000 Subject: [PATCH 02/24] "-h" is also a ver. --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index a5968ec9c..2c996cd3e 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -679,7 +679,7 @@ 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"] + "plugins", "--help", "-h"] def _create_subparsers(helpful): From f3c2a096b54950e5368907b698c488a458180961 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sun, 27 Sep 2015 02:48:44 +0000 Subject: [PATCH 03/24] Move the verb/subcommand to the end of the argparse line --- letsencrypt/cli.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 2c996cd3e..8afbf023f 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -495,13 +495,14 @@ 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. """ - - 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]] + return reordered + return args + ["run"] def prescan_for_flag(self, flag, possible_arguments): """Checks cli input for flags. From ddc04c755bf6a3e9da956ecf8782ee32b6464cc0 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sun, 27 Sep 2015 07:56:38 +0000 Subject: [PATCH 04/24] work in progress --- letsencrypt/cli.py | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 6d3aa9d2c..53609009b 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -730,53 +730,55 @@ def _create_subparsers(helpful): # 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) + helpful.add_group("auth", "Options for modifying how a cert is obtained") parser_install = add_subparser("install", install) + helpful.add_group("install", "Options for modifying how a cert is deployed") parser_revoke = add_subparser("revoke", revoke) + helpful.add_group("revoke", "Options for revocation of certs") parser_rollback = add_subparser("rollback", rollback) + helpful.add_group("rollback", "Options for reverting config changes") add_subparser("config_changes", config_changes) parser_plugins = add_subparser("plugins", plugins_cmd) + helpful.add_group("plugins", "Plugin options") - parser_auth.add_argument( - "--csr", type=read_file, help="Path to a Certificate Signing " - "Request (CSR) in DER format.") - parser_auth.add_argument( + helpful.add("auth", + "--csr", type=read_file, help="Path to a Certificate Signing Request (CSR) in DER format.") + helpful.add("auth", "--cert-path", default=flag_default("auth_cert_path"), help="When using --csr this is where certificate is saved.") - parser_auth.add_argument( + helpful.add("auth", "--chain-path", default=flag_default("auth_chain_path"), help="When using --csr this is where certificate chain is saved.") - parser_install.add_argument( - "--cert-path", required=True, help="Path to a certificate that " - "is going to be installed.") - parser_install.add_argument( + helpful.add("install", + "--cert-path", required=True, help="Path to a certificate that is going to be installed.") + helpful.add("install", "--key-path", required=True, help="Accompanying private key") - parser_install.add_argument( + helpful.add("install", "--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( + helpful.add("revoke", + "--cert-path", type=read_file, help="Revoke a specific certificate.", required=True) + helpful.add("revoke", "--key-path", type=read_file, - help="Revoke certificate using its accompanying key. Useful if " - "Account Key is lost.") + help="Revoke certificate using its accompanying key. Useful if Account Key is lost.") - parser_rollback.add_argument( + 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( + helpful.add("plugins", "--prepare", action="store_true", help="Initialize and prepare plugins.") - parser_plugins.add_argument( + helpful.add("plugins", "--authenticators", action="append_const", dest="ifaces", const=interfaces.IAuthenticator, help="Limit to authenticator plugins only.") - parser_plugins.add_argument( + helpful.add("plugins", "--installers", action="append_const", dest="ifaces", const=interfaces.IInstaller, help="Limit to installer plugins only.") From 2e0fd36c2831db4fcdaefdd5c43fac41ee7fbac6 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 21:01:02 +0000 Subject: [PATCH 05/24] Improve flag and help processing * letsencrypt --help $SUBCOMMAND now works. Fixes #787 #819 * subcommand arguments are now actually argument groups, so that all flags can be placed before or after subcommand verbs as the user wishes Fixes: #820 A limitation: * args like --cert-path were previously present for multiple verbs (auth/install/revoke) with separate docs; they are now in the "paths" topic. That's fine, though it would be good to *also* list them when the user types letsencrypt --help install. --- letsencrypt/cli.py | 64 ++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 34 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 53609009b..ac2c55551 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -489,8 +489,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. @@ -529,12 +527,17 @@ class HelpfulArgumentParser(object): def preprocess_args(self, args): """Work around some limitations in argparse. - Currently: add the default verb "run" as a default, and ensure that the + 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 + return args + for i,token in enumerate(args): if token in VERBS: - reordered = args[:i] + args[i+1:] + [args[i]] + reordered = args[:i] + args[i+1:] + [args[i]] return reordered return args + ["run"] @@ -717,6 +720,9 @@ def create_parser(plugins, args): VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", "plugins", "--help", "-h"] +HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + + [v for v in VERBS if "-" not in v]) + def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") @@ -732,53 +738,34 @@ def _create_subparsers(helpful): add_subparser("run", run) parser_auth = add_subparser("auth", auth) - helpful.add_group("auth", "Options for modifying how a cert is obtained") + helpful.add_group("auth", description="Options for modifying how a cert is obtained") parser_install = add_subparser("install", install) - helpful.add_group("install", "Options for modifying how a cert is deployed") + helpful.add_group("install", description="Options for modifying how a cert is deployed") parser_revoke = add_subparser("revoke", revoke) - helpful.add_group("revoke", "Options for revocation of certs") + helpful.add_group("revoke", description="Options for revocation of certs") parser_rollback = add_subparser("rollback", rollback) - helpful.add_group("rollback", "Options for reverting config changes") + helpful.add_group("rollback", description="Options for reverting config changes") add_subparser("config_changes", config_changes) parser_plugins = add_subparser("plugins", plugins_cmd) - helpful.add_group("plugins", "Plugin options") + helpful.add_group("plugins", description="Plugin options") - helpful.add("auth", - "--csr", type=read_file, help="Path to a Certificate Signing Request (CSR) in DER format.") - helpful.add("auth", - "--cert-path", default=flag_default("auth_cert_path"), - help="When using --csr this is where certificate is saved.") helpful.add("auth", - "--chain-path", default=flag_default("auth_chain_path"), - help="When using --csr this is where certificate chain is saved.") - - helpful.add("install", - "--cert-path", required=True, help="Path to a certificate that is going to be installed.") - helpful.add("install", - "--key-path", required=True, help="Accompanying private key") - helpful.add("install", - "--chain-path", help="Accompanying path to a certificate chain.") - helpful.add("revoke", - "--cert-path", type=read_file, help="Revoke a specific certificate.", required=True) - helpful.add("revoke", - "--key-path", type=read_file, - help="Revoke certificate using its accompanying key. Useful if Account Key is lost.") - - helpful.add("rollback", + "--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.") - helpful.add("plugins", + helpful.add("plugins", "--init", action="store_true", help="Initialize plugins.") - helpful.add("plugins", + helpful.add("plugins", "--prepare", action="store_true", help="Initialize and prepare plugins.") - helpful.add("plugins", + helpful.add("plugins", "--authenticators", action="append_const", dest="ifaces", const=interfaces.IAuthenticator, help="Limit to authenticator plugins only.") - helpful.add("plugins", + helpful.add("plugins", "--installers", action="append_const", dest="ifaces", const=interfaces.IInstaller, help="Limit to installer plugins only.") @@ -787,6 +774,15 @@ def _paths_parser(helpful): add = helpful.add helpful.add_group( "paths", description="Arguments changing execution paths & servers") + helpful.add("paths", + "--cert-path", default=flag_default("auth_cert_path"), + help="Path to where certificate is saved (with auth), " + "installed (with install --csr) or revoked.") + helpful.add("paths", + "--key-path", required=True, + help="Path to private key for cert creation or revocation (if account key is missing)") + helpful.add("paths", + "--chain-path", 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"), From a0af023b1436e27f5c1a7626aeeab374d927cf3b Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 14:48:26 -0700 Subject: [PATCH 06/24] --key-path is mandatory for install, optional for revoke --- letsencrypt/cli.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index ac2c55551..8042173e8 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -530,7 +530,6 @@ class HelpfulArgumentParser(object): 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 return args @@ -779,7 +778,7 @@ def _paths_parser(helpful): help="Path to where certificate is saved (with auth), " "installed (with install --csr) or revoked.") helpful.add("paths", - "--key-path", required=True, + "--key-path", required=("install" in helpful.args), help="Path to private key for cert creation or revocation (if account key is missing)") helpful.add("paths", "--chain-path", help="Accompanying path to a certificate chain.") From 05d439a33937c4c46d2ee949dc70c9126f463efd Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 14:48:40 -0700 Subject: [PATCH 07/24] Update cli tests We don't expect to error out if called with no args --- letsencrypt/tests/cli_test.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 2e9f3330c..992b254a7 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -40,7 +40,9 @@ class CLITest(unittest.TestCase): return ret, stdout, 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']) From 2297349b95f1451d10caae24297ba3b84dd7d6ce Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 16:56:36 -0700 Subject: [PATCH 08/24] lintian --- letsencrypt/cli.py | 24 ++++++++++++------------ letsencrypt/tests/cli_test.py | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 0d72a3eb5..b7efa041a 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -500,7 +500,6 @@ class SilentParser(object): # pylint: disable=too-few-public-methods self.parser.add_argument(*args, **kwargs) - class HelpfulArgumentParser(object): """Argparse Wrapper. @@ -545,7 +544,7 @@ class HelpfulArgumentParser(object): # all verbs double as help arguments; don't get them confused return args - for i,token in enumerate(args): + for i, token in enumerate(args): if token in VERBS: reordered = args[:i] + args[i+1:] + [args[i]] return reordered @@ -745,18 +744,21 @@ def _create_subparsers(helpful): # 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) + # 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 - parser_auth = add_subparser("auth", auth) + add_subparser("run", run) + add_subparser("auth", auth) helpful.add_group("auth", description="Options for modifying how a cert is obtained") - parser_install = add_subparser("install", install) + add_subparser("install", install) helpful.add_group("install", description="Options for modifying how a cert is deployed") - parser_revoke = add_subparser("revoke", revoke) + add_subparser("revoke", revoke) helpful.add_group("revoke", description="Options for revocation of certs") - parser_rollback = add_subparser("rollback", rollback) + add_subparser("rollback", rollback) helpful.add_group("rollback", description="Options for reverting config changes") add_subparser("config_changes", config_changes) - parser_plugins = add_subparser("plugins", plugins_cmd) + add_subparser("plugins", plugins_cmd) helpful.add_group("plugins", description="Plugin options") helpful.add("auth", @@ -769,12 +771,10 @@ def _create_subparsers(helpful): helpful.add("plugins", "--init", action="store_true", help="Initialize plugins.") helpful.add("plugins", - "--prepare", action="store_true", - help="Initialize and prepare 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.") + 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.") diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index ce32b8f78..9a99a74cc 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -44,8 +44,8 @@ class CLITest(unittest.TestCase): def test_no_flags(self): with mock.patch('letsencrypt.cli.run') as mock_run: - self._call([]) - self.assertEqual(1, mock_run.call_count) + self._call([]) + self.assertEqual(1, mock_run.call_count) def test_help(self): self.assertRaises(SystemExit, self._call, ['--help']) From 6b6bc038827e359173039a5cb229104ef257e127 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 17:12:38 -0700 Subject: [PATCH 09/24] --cert-path was required for install and revoke Oops --- letsencrypt/cli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index b7efa041a..82bd57ec8 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -786,6 +786,7 @@ def _paths_parser(helpful): "paths", description="Arguments changing execution paths & servers") helpful.add("paths", "--cert-path", default=flag_default("auth_cert_path"), + required=("install" in helpful.args or "revoke" in helpful.args), help="Path to where certificate is saved (with auth), " "installed (with install --csr) or revoked.") helpful.add("paths", From 18dacc528df67e703336bd778518a25f4850b345 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 18:08:58 -0700 Subject: [PATCH 10/24] Preserve all argparse parameters Try to restore all variants that applied to the different subcomannds --- letsencrypt/cli.py | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 82bd57ec8..ac79ab93c 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -521,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) @@ -542,12 +543,16 @@ class HelpfulArgumentParser(object): """ 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 i, token in enumerate(args): if token in VERBS: 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): @@ -782,18 +787,28 @@ def _create_subparsers(helpful): def _paths_parser(helpful): add = helpful.add + verb = helpful.verb helpful.add_group( "paths", description="Arguments changing execution paths & servers") - helpful.add("paths", - "--cert-path", default=flag_default("auth_cert_path"), - required=("install" in helpful.args or "revoke" in helpful.args), - help="Path to where certificate is saved (with auth), " - "installed (with install --csr) or revoked.") - helpful.add("paths", - "--key-path", required=("install" in helpful.args), + + 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)") - helpful.add("paths", - "--chain-path", help="Accompanying path to a certificate chain.") + + 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"), From 627fca37b4e45d300fdb3a023943b11c6bbae593 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 29 Sep 2015 18:18:18 -0700 Subject: [PATCH 11/24] We didn't actually need to define --help as a verb --- letsencrypt/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index ac79ab93c..efc0f9f70 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -732,10 +732,10 @@ 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", "-h"] + "plugins"] HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + - [v for v in VERBS if "-" not in v]) + [v for v in VERBS]) def _create_subparsers(helpful): From 1e3c92c714bb382298343d5c14f14aa896e765ab Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 11:49:46 -0700 Subject: [PATCH 12/24] Cleanup the verb -> subparser mapping --- letsencrypt/cli.py | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index efc0f9f70..fd9d8cbb6 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -9,6 +9,7 @@ import os import pkg_resources import sys import time +import types import traceback import configargparse @@ -731,17 +732,16 @@ 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_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + - [v for v in VERBS]) - +VERBS = ["run", "auth", "install", "revoke", "rollback", "config_changes", "plugins"] +HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + VERBS) def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") - def add_subparser(name, func): # pylint: disable=missing-docstring + def add_subparser(name): # pylint: disable=missing-docstring + # Each subcommand is implemented by a function of the same name + func = eval(name) # pylint: disable=eval-used + assert isinstance(func, types.FunctionType), "squirrels in namespace" subparser = subparsers.add_parser( name, help=func.__doc__.splitlines()[0], description=func.__doc__) subparser.set_defaults(func=func) @@ -752,18 +752,13 @@ def _create_subparsers(helpful): # 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) - add_subparser("run", run) - add_subparser("auth", auth) helpful.add_group("auth", description="Options for modifying how a cert is obtained") - add_subparser("install", install) helpful.add_group("install", description="Options for modifying how a cert is deployed") - add_subparser("revoke", revoke) helpful.add_group("revoke", description="Options for revocation of certs") - add_subparser("rollback", rollback) helpful.add_group("rollback", description="Options for reverting config changes") - add_subparser("config_changes", config_changes) - add_subparser("plugins", plugins_cmd) helpful.add_group("plugins", description="Plugin options") helpful.add("auth", From 2a3a111d628711e131ea202511a1383c10dfe378 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 12:09:38 -0700 Subject: [PATCH 13/24] Disable pylint invalid-name It's clearly making our code harder to read and write --- .pylintrc | 2 +- acme/acme/challenges.py | 4 ++-- acme/acme/challenges_test.py | 2 +- acme/acme/jose/interfaces_test.py | 4 ++-- acme/acme/jose/json_util_test.py | 6 +++--- acme/acme/jose/jwa_test.py | 2 +- acme/acme/jose/jwk.py | 2 +- acme/acme/jose/util.py | 4 ++-- acme/acme/jose/util_test.py | 4 ++-- acme/acme/messages_test.py | 2 +- letsencrypt/account.py | 2 +- letsencrypt/display/enhancements.py | 2 +- letsencrypt/display/ops.py | 2 +- letsencrypt/log.py | 2 +- letsencrypt/plugins/common.py | 6 +++--- letsencrypt/tests/auth_handler_test.py | 2 +- letsencrypt/tests/log_test.py | 2 +- letsencrypt/tests/reverter_test.py | 8 ++++---- 18 files changed, 29 insertions(+), 29 deletions(-) diff --git a/.pylintrc b/.pylintrc index bf318704a..268d61ec6 100644 --- a/.pylintrc +++ b/.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) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index d81e77f83..f5763adc4 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -315,7 +315,7 @@ class DVSNIResponse(ChallengeResponse): validation = jose.Field("validation", decoder=jose.JWS.from_json) @property - def z(self): # pylint: disable=invalid-name + def z(self): """The ``z`` parameter. :rtype: bytes @@ -333,7 +333,7 @@ class DVSNIResponse(ChallengeResponse): :rtype: bytes """ - z = self.z # pylint: disable=invalid-name + z = self.z return z[:32] + b'.' + z[32:] + self.DOMAIN_SUFFIX @property diff --git a/acme/acme/challenges_test.py b/acme/acme/challenges_test.py index ed44d4c45..b3f48cdf2 100644 --- a/acme/acme/challenges_test.py +++ b/acme/acme/challenges_test.py @@ -269,7 +269,7 @@ class DVSNIResponseTest(unittest.TestCase): 'validation': self.validation.to_json(), } - # pylint: disable=invalid-name + label1 = b'e2df3498860637c667fedadc5a8494ec' label2 = b'09dcc75553c9b3bd73662b50e71b1e42' self.z = label1 + label2 diff --git a/acme/acme/jose/interfaces_test.py b/acme/acme/jose/interfaces_test.py index 380c3a2a5..a3ee124ff 100644 --- a/acme/acme/jose/interfaces_test.py +++ b/acme/acme/jose/interfaces_test.py @@ -8,7 +8,7 @@ class JSONDeSerializableTest(unittest.TestCase): def setUp(self): from acme.jose.interfaces import JSONDeSerializable - # pylint: disable=missing-docstring,invalid-name + # pylint: disable=missing-docstring class Basic(JSONDeSerializable): def __init__(self, v): @@ -53,7 +53,7 @@ class JSONDeSerializableTest(unittest.TestCase): self.nested = Basic([[self.basic1]]) self.tuple = Basic(('foo',)) - # pylint: disable=invalid-name + self.Basic = Basic self.Sequence = Sequence self.Mapping = Mapping diff --git a/acme/acme/jose/json_util_test.py b/acme/acme/jose/json_util_test.py index a055f3bf7..f751382e0 100644 --- a/acme/acme/jose/json_util_test.py +++ b/acme/acme/jose/json_util_test.py @@ -92,7 +92,7 @@ class JSONObjectWithFieldsMetaTest(unittest.TestCase): from acme.jose.json_util import JSONObjectWithFieldsMeta self.field = Field('Baz') self.field2 = Field('Baz2') - # pylint: disable=invalid-name,missing-docstring,too-few-public-methods + # pylint: disable=missing-docstring,too-few-public-methods # pylint: disable=blacklisted-name @six.add_metaclass(JSONObjectWithFieldsMeta) @@ -138,7 +138,7 @@ class JSONObjectWithFieldsTest(unittest.TestCase): from acme.jose.json_util import Field class MockJSONObjectWithFields(JSONObjectWithFields): - # pylint: disable=invalid-name,missing-docstring,no-self-argument + # pylint: disable=missing-docstring,no-self-argument # pylint: disable=too-few-public-methods x = Field('x', omitempty=True, encoder=(lambda x: x * 2), @@ -158,7 +158,7 @@ class JSONObjectWithFieldsTest(unittest.TestCase): raise errors.DeserializationError() return value - # pylint: disable=invalid-name + self.MockJSONObjectWithFields = MockJSONObjectWithFields self.mock = MockJSONObjectWithFields(x=None, y=2, z=3) diff --git a/acme/acme/jose/jwa_test.py b/acme/acme/jose/jwa_test.py index 3328d083a..8ca512043 100644 --- a/acme/acme/jose/jwa_test.py +++ b/acme/acme/jose/jwa_test.py @@ -26,7 +26,7 @@ class JWASignatureTest(unittest.TestCase): def verify(self, key, msg, sig): raise NotImplementedError() # pragma: no cover - # pylint: disable=invalid-name + self.Sig1 = MockSig('Sig1') self.Sig2 = MockSig('Sig2') diff --git a/acme/acme/jose/jwk.py b/acme/acme/jose/jwk.py index 7a976f189..67f243347 100644 --- a/acme/acme/jose/jwk.py +++ b/acme/acme/jose/jwk.py @@ -186,7 +186,7 @@ class JWKRSA(JWK): @classmethod def fields_from_json(cls, jobj): - # pylint: disable=invalid-name + n, e = (cls._decode_param(jobj[x]) for x in ('n', 'e')) public_numbers = rsa.RSAPublicNumbers(e=e, n=n) if 'd' not in jobj: # public key diff --git a/acme/acme/jose/util.py b/acme/acme/jose/util.py index ab3606efc..46c43bf35 100644 --- a/acme/acme/jose/util.py +++ b/acme/acme/jose/util.py @@ -7,7 +7,7 @@ import six class abstractclassmethod(classmethod): - # pylint: disable=invalid-name,too-few-public-methods + # pylint: disable=too-few-public-methods """Descriptor for an abstract classmethod. It augments the :mod:`abc` framework with an abstract @@ -172,7 +172,7 @@ class ImmutableMap(collections.Mapping, collections.Hashable): class frozendict(collections.Mapping, collections.Hashable): - # pylint: disable=invalid-name,too-few-public-methods + # pylint: disable=too-few-public-methods """Frozen dictionary.""" __slots__ = ('_items', '_keys') diff --git a/acme/acme/jose/util_test.py b/acme/acme/jose/util_test.py index 4cdd9127f..295c70fee 100644 --- a/acme/acme/jose/util_test.py +++ b/acme/acme/jose/util_test.py @@ -92,7 +92,7 @@ class ImmutableMapTest(unittest.TestCase): """Tests for acme.jose.util.ImmutableMap.""" def setUp(self): - # pylint: disable=invalid-name,too-few-public-methods + # pylint: disable=too-few-public-methods # pylint: disable=missing-docstring from acme.jose.util import ImmutableMap @@ -156,7 +156,7 @@ class ImmutableMapTest(unittest.TestCase): self.assertEqual("B(x='foo', y='bar')", repr(self.B(x='foo', y='bar'))) -class frozendictTest(unittest.TestCase): # pylint: disable=invalid-name +class frozendictTest(unittest.TestCase): """Tests for acme.jose.util.frozendict.""" def setUp(self): diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py index d2d859bc5..718a936dd 100644 --- a/acme/acme/messages_test.py +++ b/acme/acme/messages_test.py @@ -64,7 +64,7 @@ class ConstantTest(unittest.TestCase): class MockConstant(_Constant): # pylint: disable=missing-docstring POSSIBLE_NAMES = {} - self.MockConstant = MockConstant # pylint: disable=invalid-name + self.MockConstant = MockConstant self.const_a = MockConstant('a') self.const_b = MockConstant('b') diff --git a/letsencrypt/account.py b/letsencrypt/account.py index c97e4f6fe..81d31b831 100644 --- a/letsencrypt/account.py +++ b/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) diff --git a/letsencrypt/display/enhancements.py b/letsencrypt/display/enhancements.py index 8edc72ba0..c56198161 100644 --- a/letsencrypt/display/enhancements.py +++ b/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): diff --git a/letsencrypt/display/ops.py b/letsencrypt/display/ops.py index 43705e309..cb424a81b 100644 --- a/letsencrypt/display/ops.py +++ b/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): diff --git a/letsencrypt/log.py b/letsencrypt/log.py index e800d37c9..6436f6fc2 100644 --- a/letsencrypt/log.py +++ b/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 = [] diff --git a/letsencrypt/plugins/common.py b/letsencrypt/plugins/common.py index 59598a35e..95ad56a0a 100644 --- a/letsencrypt/plugins/common.py +++ b/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) diff --git a/letsencrypt/tests/auth_handler_test.py b/letsencrypt/tests/auth_handler_test.py index ed29ead25..18ee56081 100644 --- a/letsencrypt/tests/auth_handler_test.py +++ b/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 diff --git a/letsencrypt/tests/log_test.py b/letsencrypt/tests/log_test.py index 50d0712e7..c1afd2c8a 100644 --- a/letsencrypt/tests/log_test.py +++ b/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) diff --git a/letsencrypt/tests/reverter_test.py b/letsencrypt/tests/reverter_test.py index 62c47f8d6..d31b6f2cc 100644 --- a/letsencrypt/tests/reverter_test.py +++ b/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 From 2d578468bde4cbba2138216633de44bfdd46cf04 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 12:32:44 -0700 Subject: [PATCH 14/24] Use a verb -> function table instead of eval() - plugins_cmd() not plugins() broke the more minimalist eval() approach - more wrangling was required to mock out calls via the VERBS table --- letsencrypt/cli.py | 25 +++++++++++++++---------- letsencrypt/tests/cli_test.py | 2 ++ 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index fd9d8cbb6..66f991063 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -9,7 +9,6 @@ import os import pkg_resources import sys import time -import types import traceback import configargparse @@ -731,17 +730,23 @@ def create_parser(plugins, args): return helpful.parser, helpful.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_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + VERBS) +# there isn't an elegant way to autogenerate it in time. pylint: disable=bad-whitespace +VERBS = { + "run" : run, + "auth" : auth, + "install" : install, + "revoke" : revoke, + "rollback" : rollback, + "config_changes" : config_changes, + "plugins" : plugins_cmd +} +HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] + + VERBS.keys()) def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") - def add_subparser(name): # pylint: disable=missing-docstring - # Each subcommand is implemented by a function of the same name - func = eval(name) # pylint: disable=eval-used - assert isinstance(func, types.FunctionType), "squirrels in namespace" + def add_subparser(name, func): # pylint: disable=missing-docstring subparser = subparsers.add_parser( name, help=func.__doc__.splitlines()[0], description=func.__doc__) subparser.set_defaults(func=func) @@ -752,8 +757,8 @@ def _create_subparsers(helpful): # 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) + for v, func in VERBS.items(): + add_subparser(v, func) 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") diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 9a99a74cc..f5613ee58 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -44,6 +44,8 @@ class CLITest(unittest.TestCase): def test_no_flags(self): with mock.patch('letsencrypt.cli.run') as mock_run: + from letsencrypt import cli + cli.VERBS["run"] = mock_run self._call([]) self.assertEqual(1, mock_run.call_count) From d85f42d71f5aba91cb96af6f9959c778fb047b9f Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 15:29:29 -0700 Subject: [PATCH 15/24] Plugins don't need to be in HELP_TOPICS They're already added as topics automatically, though they do need to be in the hand-written top level help. --- letsencrypt/cli.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 66f991063..1ad57b738 100644 --- a/letsencrypt/cli.py +++ b/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 """ @@ -740,8 +740,7 @@ VERBS = { "config_changes" : config_changes, "plugins" : plugins_cmd } -HELP_TOPICS = (["all", "security", "paths", "automation", "testing", "apache", "nginx"] - + VERBS.keys()) +HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + VERBS.keys() def _create_subparsers(helpful): subparsers = helpful.parser.add_subparsers(metavar="SUBCOMMAND") From 5ca1a27200fb17ac04104ba65c05d810bb20b906 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 15:31:32 -0700 Subject: [PATCH 16/24] Keep the acme/ subtree compatible with strict pylinting --- acme/acme/challenges.py | 4 ++-- acme/acme/challenges_test.py | 2 +- acme/acme/jose/interfaces_test.py | 4 ++-- acme/acme/jose/json_util_test.py | 6 +++--- acme/acme/jose/jwa_test.py | 2 +- acme/acme/jose/jwk.py | 2 +- acme/acme/jose/util.py | 4 ++-- acme/acme/jose/util_test.py | 4 ++-- acme/acme/messages_test.py | 2 +- 9 files changed, 15 insertions(+), 15 deletions(-) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index f5763adc4..d81e77f83 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -315,7 +315,7 @@ class DVSNIResponse(ChallengeResponse): validation = jose.Field("validation", decoder=jose.JWS.from_json) @property - def z(self): + def z(self): # pylint: disable=invalid-name """The ``z`` parameter. :rtype: bytes @@ -333,7 +333,7 @@ class DVSNIResponse(ChallengeResponse): :rtype: bytes """ - z = self.z + z = self.z # pylint: disable=invalid-name return z[:32] + b'.' + z[32:] + self.DOMAIN_SUFFIX @property diff --git a/acme/acme/challenges_test.py b/acme/acme/challenges_test.py index b3f48cdf2..ed44d4c45 100644 --- a/acme/acme/challenges_test.py +++ b/acme/acme/challenges_test.py @@ -269,7 +269,7 @@ class DVSNIResponseTest(unittest.TestCase): 'validation': self.validation.to_json(), } - + # pylint: disable=invalid-name label1 = b'e2df3498860637c667fedadc5a8494ec' label2 = b'09dcc75553c9b3bd73662b50e71b1e42' self.z = label1 + label2 diff --git a/acme/acme/jose/interfaces_test.py b/acme/acme/jose/interfaces_test.py index a3ee124ff..380c3a2a5 100644 --- a/acme/acme/jose/interfaces_test.py +++ b/acme/acme/jose/interfaces_test.py @@ -8,7 +8,7 @@ class JSONDeSerializableTest(unittest.TestCase): def setUp(self): from acme.jose.interfaces import JSONDeSerializable - # pylint: disable=missing-docstring + # pylint: disable=missing-docstring,invalid-name class Basic(JSONDeSerializable): def __init__(self, v): @@ -53,7 +53,7 @@ class JSONDeSerializableTest(unittest.TestCase): self.nested = Basic([[self.basic1]]) self.tuple = Basic(('foo',)) - + # pylint: disable=invalid-name self.Basic = Basic self.Sequence = Sequence self.Mapping = Mapping diff --git a/acme/acme/jose/json_util_test.py b/acme/acme/jose/json_util_test.py index f751382e0..a055f3bf7 100644 --- a/acme/acme/jose/json_util_test.py +++ b/acme/acme/jose/json_util_test.py @@ -92,7 +92,7 @@ class JSONObjectWithFieldsMetaTest(unittest.TestCase): from acme.jose.json_util import JSONObjectWithFieldsMeta self.field = Field('Baz') self.field2 = Field('Baz2') - # pylint: disable=missing-docstring,too-few-public-methods + # pylint: disable=invalid-name,missing-docstring,too-few-public-methods # pylint: disable=blacklisted-name @six.add_metaclass(JSONObjectWithFieldsMeta) @@ -138,7 +138,7 @@ class JSONObjectWithFieldsTest(unittest.TestCase): from acme.jose.json_util import Field class MockJSONObjectWithFields(JSONObjectWithFields): - # pylint: disable=missing-docstring,no-self-argument + # pylint: disable=invalid-name,missing-docstring,no-self-argument # pylint: disable=too-few-public-methods x = Field('x', omitempty=True, encoder=(lambda x: x * 2), @@ -158,7 +158,7 @@ class JSONObjectWithFieldsTest(unittest.TestCase): raise errors.DeserializationError() return value - + # pylint: disable=invalid-name self.MockJSONObjectWithFields = MockJSONObjectWithFields self.mock = MockJSONObjectWithFields(x=None, y=2, z=3) diff --git a/acme/acme/jose/jwa_test.py b/acme/acme/jose/jwa_test.py index 8ca512043..3328d083a 100644 --- a/acme/acme/jose/jwa_test.py +++ b/acme/acme/jose/jwa_test.py @@ -26,7 +26,7 @@ class JWASignatureTest(unittest.TestCase): def verify(self, key, msg, sig): raise NotImplementedError() # pragma: no cover - + # pylint: disable=invalid-name self.Sig1 = MockSig('Sig1') self.Sig2 = MockSig('Sig2') diff --git a/acme/acme/jose/jwk.py b/acme/acme/jose/jwk.py index 67f243347..7a976f189 100644 --- a/acme/acme/jose/jwk.py +++ b/acme/acme/jose/jwk.py @@ -186,7 +186,7 @@ class JWKRSA(JWK): @classmethod def fields_from_json(cls, jobj): - + # pylint: disable=invalid-name n, e = (cls._decode_param(jobj[x]) for x in ('n', 'e')) public_numbers = rsa.RSAPublicNumbers(e=e, n=n) if 'd' not in jobj: # public key diff --git a/acme/acme/jose/util.py b/acme/acme/jose/util.py index 46c43bf35..ab3606efc 100644 --- a/acme/acme/jose/util.py +++ b/acme/acme/jose/util.py @@ -7,7 +7,7 @@ import six class abstractclassmethod(classmethod): - # pylint: disable=too-few-public-methods + # pylint: disable=invalid-name,too-few-public-methods """Descriptor for an abstract classmethod. It augments the :mod:`abc` framework with an abstract @@ -172,7 +172,7 @@ class ImmutableMap(collections.Mapping, collections.Hashable): class frozendict(collections.Mapping, collections.Hashable): - # pylint: disable=too-few-public-methods + # pylint: disable=invalid-name,too-few-public-methods """Frozen dictionary.""" __slots__ = ('_items', '_keys') diff --git a/acme/acme/jose/util_test.py b/acme/acme/jose/util_test.py index 295c70fee..4cdd9127f 100644 --- a/acme/acme/jose/util_test.py +++ b/acme/acme/jose/util_test.py @@ -92,7 +92,7 @@ class ImmutableMapTest(unittest.TestCase): """Tests for acme.jose.util.ImmutableMap.""" def setUp(self): - # pylint: disable=too-few-public-methods + # pylint: disable=invalid-name,too-few-public-methods # pylint: disable=missing-docstring from acme.jose.util import ImmutableMap @@ -156,7 +156,7 @@ class ImmutableMapTest(unittest.TestCase): self.assertEqual("B(x='foo', y='bar')", repr(self.B(x='foo', y='bar'))) -class frozendictTest(unittest.TestCase): +class frozendictTest(unittest.TestCase): # pylint: disable=invalid-name """Tests for acme.jose.util.frozendict.""" def setUp(self): diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py index 718a936dd..d2d859bc5 100644 --- a/acme/acme/messages_test.py +++ b/acme/acme/messages_test.py @@ -64,7 +64,7 @@ class ConstantTest(unittest.TestCase): class MockConstant(_Constant): # pylint: disable=missing-docstring POSSIBLE_NAMES = {} - self.MockConstant = MockConstant + self.MockConstant = MockConstant # pylint: disable=invalid-name self.const_a = MockConstant('a') self.const_b = MockConstant('b') From 2406fc0486d8f74ad9979b1973e9e24d9d453df7 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 15:56:58 -0700 Subject: [PATCH 17/24] Go back to VERBS as a list The dictionary was destroying the ordering, which was important. --- letsencrypt/cli.py | 28 ++++++++++++---------------- letsencrypt/tests/cli_test.py | 2 -- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 1ad57b738..73dd24bdb 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -730,24 +730,20 @@ def create_parser(plugins, args): return helpful.parser, helpful.args # For now unfortunately this constant just needs to match the code below; -# there isn't an elegant way to autogenerate it in time. pylint: disable=bad-whitespace -VERBS = { - "run" : run, - "auth" : auth, - "install" : install, - "revoke" : revoke, - "rollback" : rollback, - "config_changes" : config_changes, - "plugins" : plugins_cmd -} -HELP_TOPICS = ["all", "security", "paths", "automation", "testing"] + VERBS.keys() +# there isn't an elegant way to autogenerate it in time. +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 @@ -756,8 +752,8 @@ def _create_subparsers(helpful): # 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, func in VERBS.items(): - add_subparser(v, func) + for v in VERBS: + add_subparser(v) 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") diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index f5613ee58..9a99a74cc 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -44,8 +44,6 @@ class CLITest(unittest.TestCase): def test_no_flags(self): with mock.patch('letsencrypt.cli.run') as mock_run: - from letsencrypt import cli - cli.VERBS["run"] = mock_run self._call([]) self.assertEqual(1, mock_run.call_count) From 95c4b55da09aee285bd823bf993d43089907456a Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 30 Sep 2015 16:49:03 -0700 Subject: [PATCH 18/24] Mark Nginx as non-working. --- letsencrypt-nginx/letsencrypt_nginx/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index 3f6d6f327..a88607e58 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -56,7 +56,7 @@ class NginxConfigurator(common.Plugin): zope.interface.implements(interfaces.IAuthenticator, interfaces.IInstaller) zope.interface.classProvides(interfaces.IPluginFactory) - description = "Nginx Web Server - Alpha" + description = "Nginx Web Server - currently doesn't work" @classmethod def add_parser_arguments(cls, add): From 11ca1108c2536adb0d735e76829f178f06a08715 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 16:53:08 -0700 Subject: [PATCH 19/24] Test cases for command line help --- letsencrypt/tests/cli_test.py | 37 ++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 9a99a74cc..75eec1978 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -2,6 +2,7 @@ import itertools import os import shutil +import StringIO import traceback import tempfile import unittest @@ -42,6 +43,21 @@ 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): with mock.patch('letsencrypt.cli.run') as mock_run: self._call([]) @@ -49,7 +65,26 @@ class CLITest(unittest.TestCase): 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) def test_rollback(self): _, _, _, client = self._call(['rollback']) From 43cb36807a001562726bceaaaae00d708fcc5ed2 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 17:00:09 -0700 Subject: [PATCH 20/24] Also test top level help --- letsencrypt/tests/cli_test.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 75eec1978..0a92aba62 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -85,6 +85,12 @@ class CLITest(unittest.TestCase): 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']) From 9cf2ea8a5742d8868f1f1c47377626a741464bc7 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 30 Sep 2015 17:16:27 -0700 Subject: [PATCH 21/24] Report Apache correctly when uninstalled --- .../letsencrypt_apache/configurator.py | 6 +++ .../tests/configurator_test.py | 10 ++++- .../letsencrypt_apache/tests/util.py | 45 ++++++++++--------- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index ad3c62d2c..f3d2b5f9a 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -137,6 +137,12 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): :raises .errors.PluginError: If there is any other error """ + # Verify Apache is installed + for exe in (self.conf("ctl"), self.conf("enmod"), + self.conf("dismod"), self.conf("init-script")): + if not le_util.exe_exists(exe): + raise errors.NoInstallationError + # Make sure configuration is valid self.config_test() diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 026594a8f..7c2137c45 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -37,8 +37,16 @@ class TwoVhost80Test(util.ApacheTest): shutil.rmtree(self.config_dir) shutil.rmtree(self.work_dir) + @mock.patch("letsencrypt_apache.configurator.le_util.exe_exists") + def test_prepare_no_install(self, mock_exe_exists): + mock_exe_exists.return_value = False + self.assertRaises( + errors.NoInstallationError, self.config.prepare) + @mock.patch("letsencrypt_apache.parser.ApacheParser") - def test_prepare_version(self, _): + @mock.patch("letsencrypt_apache.configurator.le_util.exe_exists") + def test_prepare_version(self, mock_exe_exists, _): + mock_exe_exists.return_value = True self.config.version = None self.config.config_test = mock.Mock() self.config.get_version = mock.Mock(return_value=(1, 1)) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/util.py b/letsencrypt-apache/letsencrypt_apache/tests/util.py index b544e06ee..2594ba773 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/util.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/util.py @@ -66,31 +66,34 @@ def get_apache_configurator( """ backups = os.path.join(work_dir, "backups") + mock_le_config = mock.MagicMock( + apache_server_root=config_path, + apache_le_vhost_ext=constants.CLI_DEFAULTS["le_vhost_ext"], + backup_dir=backups, + config_dir=config_dir, + temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"), + in_progress_dir=os.path.join(backups, "IN_PROGRESS"), + work_dir=work_dir) with mock.patch("letsencrypt_apache.configurator." "subprocess.Popen") as mock_popen: - with mock.patch("letsencrypt_apache.parser.ApacheParser." - "update_runtime_variables"): - # This indicates config_test passes - mock_popen().communicate.return_value = ("Fine output", "No problems") - mock_popen().returncode = 0 + # This indicates config_test passes + mock_popen().communicate.return_value = ("Fine output", "No problems") + mock_popen().returncode = 0 + with mock.patch("letsencrypt_apache.configurator.le_util." + "exe_exists") as mock_exe_exists: + mock_exe_exists.return_value = True + with mock.patch("letsencrypt_apache.parser.ApacheParser." + "update_runtime_variables"): + config = configurator.ApacheConfigurator( + config=mock_le_config, + name="apache", + version=version) + # This allows testing scripts to set it a bit more quickly + if conf is not None: + config.conf = conf # pragma: no cover - config = configurator.ApacheConfigurator( - config=mock.MagicMock( - apache_server_root=config_path, - apache_le_vhost_ext=constants.CLI_DEFAULTS["le_vhost_ext"], - backup_dir=backups, - config_dir=config_dir, - temp_checkpoint_dir=os.path.join(work_dir, "temp_checkpoints"), - in_progress_dir=os.path.join(backups, "IN_PROGRESS"), - work_dir=work_dir), - name="apache", - version=version) - # This allows testing scripts to set it a bit more quickly - if conf is not None: - config.conf = conf # pragma: no cover - - config.prepare() + config.prepare() return config From 8041b35f9988d1193528df0d36b14eca35babc3a Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 30 Sep 2015 19:06:16 -0700 Subject: [PATCH 22/24] Make sure the LICENSE file is accurate for first pre-relase - In general copyrights remain with their respective authors or authors' organizations, but with license granted by clause 5 of the Apache License. - Presently the plurality of the copyright in the client is held by EFF as a result of work-for-hire by jdkasten, bmw, schoen, pde, rolandshoemaker and jsha; or by Jakub Warmuz or his employer, Google. --- LICENSE.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/LICENSE.txt b/LICENSE.txt index 5a9f6fa55..2ed752521 100644 --- a/LICENSE.txt +++ b/LICENSE.txt @@ -1,5 +1,5 @@ -Let's Encrypt: -Copyright (c) Internet Security Research Group +Let's Encrypt Python Client +Copyright (c) Electronic Frontier Foundation and others Licensed Apache Version 2.0 Incorporating code from nginxparser From 268368b3e928e669420beeefd5d82a8af4de4a1f Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 1 Oct 2015 10:12:38 -0700 Subject: [PATCH 23/24] Updated README to reflect state of Nginx plugin --- README.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.rst b/README.rst index 23e4dad29..43ecd413c 100644 --- a/README.rst +++ b/README.rst @@ -79,7 +79,7 @@ Current Features * web servers supported: - apache/2.x (tested and working on Ubuntu Linux) - - nginx/0.8.48+ (tested and mostly working on Ubuntu Linux) + - nginx/0.8.48+ (under development) - standalone (runs its own webserver to prove you control the domain) * the private key is generated locally on your system From 6bde83c9835b1fba9a935f341e62a48b8393d189 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 1 Oct 2015 11:53:11 -0700 Subject: [PATCH 24/24] Fixed indentation in storage.py --- letsencrypt/storage.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/storage.py b/letsencrypt/storage.py index 08dff25a1..be270a762 100644 --- a/letsencrypt/storage.py +++ b/letsencrypt/storage.py @@ -520,7 +520,7 @@ class RenewableCert(object): # pylint: disable=too-many-instance-attributes remaining = expiry - now if remaining < autorenew_interval: return True - return False + return False @classmethod def new_lineage(cls, lineagename, cert, privkey, chain,