From c60c28514c6a8dde2f94c5d557cce7649b67a344 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 13:49:32 -0700 Subject: [PATCH 01/11] Add global DEFAULT variable --- certbot/cli.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/certbot/cli.py b/certbot/cli.py index cff111f42..e2475ec31 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -104,6 +104,11 @@ ZERO_ARG_ACTIONS = set(("store_const", "store_true", "store_false", "append_const", "count",)) +# Maps a config option to its default value. This is set during the +# parse_args method of HelpfulArgumentParser. +DEFAULTS = None + + # Maps a config option to a set of config options that may have modified it. # This dictionary is used recursively, so if A modifies B and B modifies C, # it is determined that C was modified by the user if A was modified. From 657c4e725992abba9e2c80a9cd7598d97b79b910 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 14:49:24 -0700 Subject: [PATCH 02/11] Set DEFAULTS during parse_args --- certbot/cli.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/certbot/cli.py b/certbot/cli.py index e2475ec31..3c65527e1 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -1,6 +1,7 @@ """Certbot command line argument & config processing.""" from __future__ import print_function import argparse +import copy import glob import logging import logging.handlers @@ -340,6 +341,10 @@ class HelpfulArgumentParser(object): if self.detect_defaults: return parsed_args + global DEFAULTS # pylint: disable=global-statement + DEFAULTS = dict((key, copy.deepcopy(self.parser.get_default(key))) + for key in vars(parsed_args)) + # Do any post-parsing homework here if self.verb == "renew" and not parsed_args.dialog_mode: From b57677b16a994d21659ea2eed7557c9a583712d1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 14:57:14 -0700 Subject: [PATCH 03/11] Use cli.DEFAULTS in storage.py --- certbot/storage.py | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/certbot/storage.py b/certbot/storage.py index 60886e306..7602ece2d 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -162,30 +162,16 @@ def relevant_values(all_values): from certbot import cli - def _is_cli_default(option, value): - # Look through the CLI parser defaults and see if this option is - # both present and equal to the specified value. If not, return - # False. - # pylint: disable=protected-access - for x in cli.helpful_parser.parser._actions: - if x.dest == option: - if x.default == value: - return True - else: - break - return False - values = dict() for option, value in all_values.iteritems(): # Try to find reasons to store this item in the # renewal config. It can be stored if it is relevant and - # (it is set_by_cli() or flag_default() is different - # from the value or flag_default() doesn't exist). + # (it is set_by_cli(), we don't know the default value, or + # the current value differs from the default value). if _relevant(option): - if (cli.set_by_cli(option) - or not _is_cli_default(option, value)): -# or option not in constants.CLI_DEFAULTS -# or constants.CLI_DEFAULTS[option] != value): + if (cli.set_by_cli(option) or + option not in cli.DEFAULTS or + cli.DEFAULTS[option] != value): values[option] = value return values From 26316fb222cb3cf180810bbf8cdea85e8ddeaf95 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 15:06:49 -0700 Subject: [PATCH 04/11] Ensure changes to webroot_map aren't reflected in cli.DEFAULTS --- certbot/tests/cli_test.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index f9557abfb..03edaa18f 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -490,6 +490,12 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods conflicts += ['--staging'] self._check_server_conflict_message(short_args, conflicts) + def test_defaults_global(self): + namespace = self._get_argument_parser()([]) + namespace.webroot_map['example.com'] = '/var/www/html' + + self.assertTrue(cli.DEFAULTS != namespace.webroot_map) + def _certonly_new_request_common(self, mock_client, args=None): with mock.patch('certbot.main._treat_as_renewal') as mock_renewal: mock_renewal.return_value = ("newcert", None) From 8f6866309781f656725c0ef9d6e89a7b291f4aed Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 16:52:35 -0700 Subject: [PATCH 05/11] Add option_was_set function --- certbot/cli.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/certbot/cli.py b/certbot/cli.py index 3c65527e1..428227657 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -217,6 +217,21 @@ def set_by_cli(var): set_by_cli.detector = None +def option_was_set(option, value): + """Was option set by the user or does it differ from the default? + + :param str option: configuration variable being considered + :param value: value of the configuration variable named option + + :returns: True if the option was set, otherwise, False + :rtype: bool + + """ + return (set_by_cli(option) or + option not in DEFAULTS or + DEFAULTS[option] != value) + + def argparse_type(variable): "Return our argparse type function for a config variable (default: str)" # pylint: disable=protected-access From aaf93b65b0b1b08bec02e84ea92145d3f15e7081 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 16:55:45 -0700 Subject: [PATCH 06/11] Refactor storage.relevant_values --- certbot/storage.py | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/certbot/storage.py b/certbot/storage.py index 7602ece2d..82fdbfd54 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -7,8 +7,10 @@ import re import configobj import parsedatetime import pytz +import six import certbot +from certbot import cli from certbot import constants from certbot import crypto_util from certbot import errors @@ -158,22 +160,13 @@ def relevant_values(all_values): :param dict all_values: The original values. :returns: A new dictionary containing items that can be used in renewal. - :rtype dict:""" + :rtype dict: - from certbot import cli - - values = dict() - for option, value in all_values.iteritems(): - # Try to find reasons to store this item in the - # renewal config. It can be stored if it is relevant and - # (it is set_by_cli(), we don't know the default value, or - # the current value differs from the default value). - if _relevant(option): - if (cli.set_by_cli(option) or - option not in cli.DEFAULTS or - cli.DEFAULTS[option] != value): - values[option] = value - return values + """ + return dict( + (option, value) + for option, value in six.iteritems(all_values) + if _relevant(option) and cli.option_was_set(option, value)) class RenewableCert(object): # pylint: disable=too-many-instance-attributes From 8c174f38b5e59ecf2298abf08a84c52110cc4091 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 17:00:59 -0700 Subject: [PATCH 07/11] Add has_default_value method --- certbot/cli.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 428227657..3c18c3c98 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -217,6 +217,21 @@ def set_by_cli(var): set_by_cli.detector = None +def has_default_value(option, value): + """Does option have the default value? + + If the default value of option is not known, False is returned. + + :param str option: configuration variable being considered + :param value: value of the configuration variable named option + + :returns: True if option has the default value, otherwise, False + :rtype: bool + + """ + return option in DEFAULTS and DEFAULTS[option] == value + + def option_was_set(option, value): """Was option set by the user or does it differ from the default? @@ -227,9 +242,7 @@ def option_was_set(option, value): :rtype: bool """ - return (set_by_cli(option) or - option not in DEFAULTS or - DEFAULTS[option] != value) + return set_by_cli(option) or not has_default_value(option, value) def argparse_type(variable): From 4c68792dd37cc93e17038eba7cd8bee94cdcc6c1 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 17:29:56 -0700 Subject: [PATCH 08/11] Remove test_defaults_global --- certbot/tests/cli_test.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 03edaa18f..f9557abfb 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -490,12 +490,6 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods conflicts += ['--staging'] self._check_server_conflict_message(short_args, conflicts) - def test_defaults_global(self): - namespace = self._get_argument_parser()([]) - namespace.webroot_map['example.com'] = '/var/www/html' - - self.assertTrue(cli.DEFAULTS != namespace.webroot_map) - def _certonly_new_request_common(self, mock_client, args=None): with mock.patch('certbot.main._treat_as_renewal') as mock_renewal: mock_renewal.return_value = ("newcert", None) From 97af8dfb701edece39ae429852ac1a9784f991e2 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 17:31:57 -0700 Subject: [PATCH 09/11] Add defaults to helpful_parser --- certbot/cli.py | 14 +++++--------- certbot/tests/storage_test.py | 5 ++--- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 3c18c3c98..ba1f23708 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -105,11 +105,6 @@ ZERO_ARG_ACTIONS = set(("store_const", "store_true", "store_false", "append_const", "count",)) -# Maps a config option to its default value. This is set during the -# parse_args method of HelpfulArgumentParser. -DEFAULTS = None - - # Maps a config option to a set of config options that may have modified it. # This dictionary is used recursively, so if A modifies B and B modifies C, # it is determined that C was modified by the user if A was modified. @@ -229,7 +224,8 @@ def has_default_value(option, value): :rtype: bool """ - return option in DEFAULTS and DEFAULTS[option] == value + return (option in helpful_parser.defaults and + helpful_parser.defaults[option] == value) def option_was_set(option, value): @@ -354,6 +350,7 @@ class HelpfulArgumentParser(object): sys.exit(0) self.visible_topics = self.determine_help_topics(self.help_arg) self.groups = {} # elements are added by .add_group() + self.defaults = {} # elements are added by .parse_args() def parse_args(self): """Parses command line arguments and returns the result. @@ -369,9 +366,8 @@ class HelpfulArgumentParser(object): if self.detect_defaults: return parsed_args - global DEFAULTS # pylint: disable=global-statement - DEFAULTS = dict((key, copy.deepcopy(self.parser.get_default(key))) - for key in vars(parsed_args)) + self.defaults = dict((key, copy.deepcopy(self.parser.get_default(key))) + for key in vars(parsed_args)) # Do any post-parsing homework here diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 0d907eca3..138f6e2fa 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -533,10 +533,9 @@ class RenewableCertTests(BaseRenewableCertTest): """Test that relevant_values() can reject a default value.""" # pylint: disable=protected-access from certbot import storage - mock_parser.verb = "certonly" mock_parser.args = ["--standalone"] - mock_action = mock.Mock(dest="rsa_key_size", default=2048) - mock_parser.parser._actions = [mock_action] + mock_parser.defaults = {"rsa_key_size": 2048} + mock_parser.verb = "certonly" self.assertEqual(storage.relevant_values({"rsa_key_size": 2048}), {}) @mock.patch("certbot.cli.helpful_parser") From f9d5ecaf6fc6776988ddf5ea9dbb010ad45eeb7f Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 13 Jun 2016 17:45:47 -0700 Subject: [PATCH 10/11] Add option_was_set test --- certbot/tests/cli_test.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index f9557abfb..9d6335b40 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -447,6 +447,19 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods short_args += '--server example.com'.split() self._check_server_conflict_message(short_args, '--staging') + def test_option_was_set(self): + key_size_option = 'rsa_key_size' + key_size_value = cli.flag_default(key_size_option) + self._get_argument_parser()( + '--rsa-key-size {0}'.format(key_size_value).split()) + + self.assertTrue(cli.option_was_set(key_size_option, key_size_value)) + self.assertTrue(cli.option_was_set('no_verify_ssl', True)) + + config_dir_option = 'config_dir' + self.assertFalse(cli.option_was_set( + config_dir_option, cli.flag_default(config_dir_option))) + def _assert_dry_run_flag_worked(self, namespace, existing_account): self.assertTrue(namespace.dry_run) self.assertTrue(namespace.break_my_certs) From 261046a2d7dd01a4ce5383d192ce82d34a92faf8 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 14 Jun 2016 13:52:39 -0700 Subject: [PATCH 11/11] Update relevant_values tests --- certbot/tests/storage_test.py | 50 ++++++++++++++++------------------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 138f6e2fa..261500b98 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -11,6 +11,7 @@ import mock import pytz import certbot +from certbot import cli from certbot import configuration from certbot import errors from certbot.storage import ALL_FOUR @@ -517,38 +518,33 @@ class RenewableCertTests(BaseRenewableCertTest): self.assertFalse(os.path.islink(self.test_rc.version("privkey", 10))) self.assertFalse(os.path.exists(temp_config_file)) - @mock.patch("certbot.cli.helpful_parser") - def test_relevant_values(self, mock_parser): + def _test_relevant_values_common(self, values): + option = "rsa_key_size" + mock_parser = mock.Mock(args=["--standalone"], verb="certonly", + defaults={option: cli.flag_default(option)}) + + from certbot.storage import relevant_values + with mock.patch("certbot.cli.helpful_parser", mock_parser): + return relevant_values(values) + + def test_relevant_values(self): """Test that relevant_values() can reject an irrelevant value.""" - # pylint: disable=protected-access - from certbot import storage - mock_parser.verb = "certonly" - mock_parser.args = ["--standalone"] - mock_action = mock.Mock(dest="rsa_key_size", default=2048) - mock_parser.parser._actions = [mock_action] - self.assertEqual(storage.relevant_values({"hello": "there"}), {}) + self.assertEqual( + self._test_relevant_values_common({"hello": "there"}), {}) - @mock.patch("certbot.cli.helpful_parser") - def test_relevant_values_default(self, mock_parser): + def test_relevant_values_default(self): """Test that relevant_values() can reject a default value.""" - # pylint: disable=protected-access - from certbot import storage - mock_parser.args = ["--standalone"] - mock_parser.defaults = {"rsa_key_size": 2048} - mock_parser.verb = "certonly" - self.assertEqual(storage.relevant_values({"rsa_key_size": 2048}), {}) + option = "rsa_key_size" + values = {option: cli.flag_default(option)} + self.assertEqual(self._test_relevant_values_common(values), {}) - @mock.patch("certbot.cli.helpful_parser") - def test_relevant_values_nondefault(self, mock_parser): + def test_relevant_values_nondefault(self): """Test that relevant_values() can retain a non-default value.""" - # pylint: disable=protected-access - from certbot import storage - mock_parser.verb = "certonly" - mock_parser.args = ["--standalone"] - mock_action = mock.Mock(dest="rsa_key_size", default=2048) - mock_parser.parser._actions = [mock_action] - self.assertEqual(storage.relevant_values({"rsa_key_size": 12}), - {"rsa_key_size": 12}) + values = {"rsa_key_size": 12} + # A copy is given to _test_relevant_values_common + # to make sure values isn't modified by the method + self.assertEqual( + self._test_relevant_values_common(values.copy()), values) @mock.patch("certbot.storage.relevant_values") def test_new_lineage(self, mock_rv):