Merge pull request #3168 from certbot/default-detector

Fix default detection

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
@@ -211,6 +212,35 @@ 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 helpful_parser.defaults and
+            helpful_parser.defaults[option] == value)
+
+
+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 not has_default_value(option, value)
+
+
 def argparse_type(variable):
     "Return our argparse type function for a config variable (default: str)"
     # pylint: disable=protected-access
@@ -320,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.
@@ -335,6 +366,9 @@ class HelpfulArgumentParser(object):
         if self.detect_defaults:
             return 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
 
         if self.verb == "renew" and not parsed_args.dialog_mode:

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,36 +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
-
-    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).
-        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):
-                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

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)

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,39 +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.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": 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):