From fa7aed4d63184e0e8a428cb03ae5c0794103e06e Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 30 Oct 2015 14:44:17 -0700 Subject: [PATCH 1/6] Ensure that mandatory flags are displayed under the relevant verb help topics Closes: #996 In part our problem was trying to pick a single topic ("paths") for flags that become essential in some cases. But we were also calling _paths_parser before all the topic groups had been created. --- letsencrypt/cli.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 641d0d341..a2d3652b0 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -650,12 +650,12 @@ class HelpfulArgumentParser(object): help1 = self.prescan_for_flag("-h", self.help_topics) help2 = self.prescan_for_flag("--help", self.help_topics) assert max(True, "a") == "a", "Gravity changed direction" - help_arg = max(help1, help2) - if help_arg is True: + self.help_arg = max(help1, help2) + if self.help_arg is True: # just --help with no topic; avoid argparse altogether print usage sys.exit(0) - self.visible_topics = self.determine_help_topics(help_arg) + self.visible_topics = self.determine_help_topics(self.help_arg) #print self.visible_topics self.groups = {} # elements are added by .add_group() @@ -873,12 +873,12 @@ def prepare_and_parse_args(plugins, args): help="Require that all configuration files are owned by the current " "user; only needed if your config is somewhere unsafe like /tmp/") + _create_subparsers(helpful) _paths_parser(helpful) # _plugins_parsing should be the last thing to act upon the main # parser (--help should display plugin-specific options last) _plugins_parsing(helpful, plugins) - _create_subparsers(helpful) return helpful.parse_args() @@ -914,19 +914,28 @@ def _create_subparsers(helpful): def _paths_parser(helpful): add = helpful.add 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), installed (with install --csr) or revoked." + cph = "Path to where cert is saved (with auth --csr), installed from or revoked." + section = "paths" + if verb in ("install", "revoke", "certonly"): + section = verb if verb == "certonly": - add("paths", "--cert-path", default=flag_default("auth_cert_path"), help=cph) + add(section, "--cert-path", default=flag_default("auth_cert_path"), help=cph) elif verb == "revoke": - add("paths", "--cert-path", type=read_file, required=True, help=cph) + add(section, "--cert-path", type=read_file, required=True, help=cph) else: - add("paths", "--cert-path", help=cph, required=(verb == "install")) + add(section, "--cert-path", help=cph, required=(verb == "install")) + section = "paths" + if verb in ("install", "revoke"): + section = verb + print helpful.help_arg, helpful.help_arg == "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), + add(section, "--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)") From 3356ce75586ff79c6f6d4e24f0645b5269464c0b Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 30 Oct 2015 14:55:51 -0700 Subject: [PATCH 2/6] This is a pretty silly lint warning that we're hitting a lot --- .pylintrc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pylintrc b/.pylintrc index 268d61ec6..e882d0858 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,invalid-name +disable=fixme,locally-disabled,abstract-class-not-used,bad-continuation,too-few-public-methods,no-self-use,invalid-name,too-many-instance-attributes # abstract-class-not-used cannot be disabled locally (at least in pylint 1.4.1) From e4b10f76f9e491f0008ea7a10b8ffe05f62f85a9 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 30 Oct 2015 14:56:08 -0700 Subject: [PATCH 3/6] Delint --- letsencrypt/cli.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index a2d3652b0..3477e4eb4 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -915,14 +915,14 @@ def _paths_parser(helpful): add = helpful.add verb = helpful.verb if verb == "help": - verb = helpful.help_arg + 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" if verb in ("install", "revoke", "certonly"): - section = verb + section = verb if verb == "certonly": add(section, "--cert-path", default=flag_default("auth_cert_path"), help=cph) elif verb == "revoke": @@ -932,7 +932,7 @@ def _paths_parser(helpful): section = "paths" if verb in ("install", "revoke"): - section = verb + section = verb print helpful.help_arg, helpful.help_arg == "install" # revoke --key-path reads a file, install --key-path takes a string add(section, "--key-path", type=((verb == "revoke" and read_file) or str), From f00280f71adda2fb3957c5c3a35144fb41988853 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 30 Oct 2015 15:12:54 -0700 Subject: [PATCH 4/6] Testiness --- letsencrypt/tests/cli_test.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 669fa1ce8..2f12f055d 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -101,6 +101,24 @@ class CLITest(unittest.TestCase): self.assertTrue("Plugin options" in out) output.truncate(0) + self.assertRaises(SystemExit, self._call_stdout, ['--help', 'install']) + out = output.getvalue() + self.assertTrue("--cert-path" in out) + self.assertTrue("--key-path" in out) + output.truncate(0) + + self.assertRaises(SystemExit, self._call_stdout, ['--help', 'revoke']) + out = output.getvalue() + self.assertTrue("--cert-path" in out) + self.assertTrue("--key-path" in out) + output.truncate(0) + + self.assertRaises(SystemExit, self._call_stdout, ['--help', 'config_changes']) + out = output.getvalue() + self.assertTrue("--cert-path" not in out) + self.assertTrue("--key-path" not in out) + output.truncate(0) + self.assertRaises(SystemExit, self._call_stdout, ['-h']) out = output.getvalue() from letsencrypt import cli From a5e815008e43bfd904e2bc73469a20c6604192fe Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Fri, 30 Oct 2015 15:22:05 -0700 Subject: [PATCH 5/6] Factor out all the stdout wrangling from help tests --- letsencrypt/tests/cli_test.py | 88 ++++++++++++++++------------------- 1 file changed, 40 insertions(+), 48 deletions(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index 2f12f055d..e38448249 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -64,65 +64,57 @@ class CLITest(unittest.TestCase): 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']) + def _help_output(self, args): + "Run a help command, and return the help string for scrutiny" output = StringIO.StringIO() with mock.patch('letsencrypt.cli.sys.stdout', new=output): plugins = disco.PluginsRegistry.find_all() - self.assertRaises(SystemExit, self._call_stdout, ['--help', 'all']) + self.assertRaises(SystemExit, self._call_stdout, args) 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) + return out - self.assertRaises(SystemExit, self._call_stdout, ['-h', 'nginx']) - out = output.getvalue() - if "nginx" in plugins: - # may be false while building distributions without plugins - self.assertTrue("--nginx-ctl" in out) - self.assertTrue("--manual-test-mode" not in out) - self.assertTrue("--checkpoints" not in out) - output.truncate(0) + def test_help(self): + self.assertRaises(SystemExit, self._call, ['--help']) + self.assertRaises(SystemExit, self._call, ['--help', 'all']) + plugins = disco.PluginsRegistry.find_all() + out = self._help_output(['--help', 'all']) + self.assertTrue("--configurator" in out) + self.assertTrue("how a cert is deployed" in out) + self.assertTrue("--manual-test-mode" in out) - self.assertRaises(SystemExit, self._call_stdout, ['-h']) - out = output.getvalue() - if "nginx" in plugins: - self.assertTrue("Use the Nginx plugin" in out) - else: - self.assertTrue("(nginx support is experimental" in out) - output.truncate(0) + out = self._help_output(['-h', 'nginx']) + if "nginx" in plugins: + # may be false while building distributions without plugins + self.assertTrue("--nginx-ctl" in out) + self.assertTrue("--manual-test-mode" not in out) + self.assertTrue("--checkpoints" not in out) - 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) + out = self._help_output(['-h']) + if "nginx" in plugins: + self.assertTrue("Use the Nginx plugin" in out) + else: + self.assertTrue("(nginx support is experimental" in out) - self.assertRaises(SystemExit, self._call_stdout, ['--help', 'install']) - out = output.getvalue() - self.assertTrue("--cert-path" in out) - self.assertTrue("--key-path" in out) - output.truncate(0) + 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.assertRaises(SystemExit, self._call_stdout, ['--help', 'revoke']) - out = output.getvalue() - self.assertTrue("--cert-path" in out) - self.assertTrue("--key-path" in out) - output.truncate(0) + out = self._help_output(['--help', 'install']) + self.assertTrue("--cert-path" in out) + self.assertTrue("--key-path" in out) - self.assertRaises(SystemExit, self._call_stdout, ['--help', 'config_changes']) - out = output.getvalue() - self.assertTrue("--cert-path" not in out) - self.assertTrue("--key-path" not in out) - output.truncate(0) + out = self._help_output(['--help', 'revoke']) + self.assertTrue("--cert-path" in out) + self.assertTrue("--key-path" in out) - self.assertRaises(SystemExit, self._call_stdout, ['-h']) - out = output.getvalue() - from letsencrypt import cli - self.assertTrue(cli.usage_strings(plugins)[0] in out) + out = self._help_output(['-h', 'config_changes']) + self.assertTrue("--cert-path" not in out) + self.assertTrue("--key-path" not in out) + + out = self._help_output(['-h']) + from letsencrypt import cli + self.assertTrue(cli.usage_strings(plugins)[0] in out) def test_configurator_selection(self): real_plugins = disco.PluginsRegistry.find_all() From 21bfe5b98329be348a15cfd97b93f21f7aef26c6 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Sun, 1 Nov 2015 22:48:58 -0800 Subject: [PATCH 6/6] Remove stray variable --- letsencrypt/tests/cli_test.py | 1 - 1 file changed, 1 deletion(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index e38448249..7f8fbb351 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -68,7 +68,6 @@ class CLITest(unittest.TestCase): "Run a help command, and return the help string for scrutiny" output = StringIO.StringIO() with mock.patch('letsencrypt.cli.sys.stdout', new=output): - plugins = disco.PluginsRegistry.find_all() self.assertRaises(SystemExit, self._call_stdout, args) out = output.getvalue() return out