Simplify and deprecate viewing config changes (certbot#7198)

* Remove apache and nginx from config_changes help

* Deprecate certbot_config changes.

* Document config_changes deprecation.

* Remove view_config_changes as IInstaller method.

* Remove view_config_changes from plugins.

* Add view_config_changes warnings.

* simplify test_config_changes_deprecation

CHANGELOG.md

@@ -15,7 +15,12 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 * Update the 'manage your account' help to be more generic.
 * The error message when Certbot's Apache plugin is unable to modify your
   Apache configuration has been improved.
+* Certbot's config_changes subcommand has been deprecated and will be
+  removed in a future release.
 * `certbot config_changes` no longer accepts a --num parameter.
+* The functions `certbot.plugins.common.Installer.view_config_changes` and
+  `certbot.reverter.Reverter.view_config_changes` have been deprecated and will
+  be removed in a future release.
 
 ### Fixed
 

certbot-apache/certbot_apache/tests/configurator_reverter_test.py

@@ -74,14 +74,6 @@ def test_rollback_error(self):
             side_effect=errors.ReverterError)
         self.assertRaises(errors.PluginError, self.config.rollback_checkpoints)
 
-    def test_view_config_changes(self):
-        self.config.view_config_changes()
-
-    def test_view_config_changes_error(self):
-        self.config.reverter.view_config_changes = mock.Mock(
-            side_effect=errors.ReverterError)
-        self.assertRaises(errors.PluginError, self.config.view_config_changes)
-
     def test_recovery_routine_reload(self):
         mock_load = mock.Mock()
         self.config.parser.aug.load = mock_load

certbot-nginx/certbot_nginx/tests/configurator_test.py

@@ -427,11 +427,6 @@ def test_recovery_routine_throws_error_from_reverter(self, mock_recovery_routine
         mock_recovery_routine.side_effect = errors.ReverterError("foo")
         self.assertRaises(errors.PluginError, self.config.recovery_routine)
 
-    @mock.patch("certbot.reverter.Reverter.view_config_changes")
-    def test_view_config_changes_throws_error_from_reverter(self, mock_view_config_changes):
-        mock_view_config_changes.side_effect = errors.ReverterError("foo")
-        self.assertRaises(errors.PluginError, self.config.view_config_changes)
-
     @mock.patch("certbot.reverter.Reverter.rollback_checkpoints")
     def test_rollback_checkpoints_throws_error_from_reverter(self, mock_rollback_checkpoints):
         mock_rollback_checkpoints.side_effect = errors.ReverterError("foo")

certbot/cli.py

@@ -1418,10 +1418,10 @@ def _plugins_parsing(helpful, plugins):
                 help="Authenticator plugin name.")
     helpful.add("plugins", "-i", "--installer", default=flag_default("installer"),
                 help="Installer plugin name (also used to find domains).")
-    helpful.add(["plugins", "certonly", "run", "install", "config_changes"],
+    helpful.add(["plugins", "certonly", "run", "install"],
                 "--apache", action="store_true", default=flag_default("apache"),
                 help="Obtain and install certificates using Apache")
-    helpful.add(["plugins", "certonly", "run", "install", "config_changes"],
+    helpful.add(["plugins", "certonly", "run", "install"],
                 "--nginx", action="store_true", default=flag_default("nginx"),
                 help="Obtain and install certificates using Nginx")
     helpful.add(["plugins", "certonly"], "--standalone", action="store_true",

certbot/interfaces.py

@@ -355,13 +355,6 @@ def recovery_routine():  # type: ignore
 
         """
 
-    def view_config_changes():  # type: ignore
-        """Display all of the LE config changes.
-
-        :raises .PluginError: when config changes cannot be parsed
-
-        """
-
     def config_test():  # type: ignore
         """Make sure the configuration is valid.
 

certbot/main.py

@@ -976,6 +976,8 @@ def config_changes(config, unused_plugins):
     :rtype: None
 
     """
+    logger.warning("The config_changes subcommand has been deprecated"
+                   " and will be removed in a future release.")
     client.view_config_changes(config)
 
 def update_symlinks(config, unused_plugins):

certbot/plugins/common.py

@@ -3,6 +3,7 @@
 import re
 import shutil
 import tempfile
+import warnings
 
 import OpenSSL
 import pkg_resources
@@ -197,10 +198,18 @@ def view_config_changes(self):
             the checkpoints directories.
 
         """
-        try:
-            self.reverter.view_config_changes()
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))
+        warnings.warn(
+            "The view_config_changes method is no longer part of Certbot's"
+            " plugin interface, has been deprecated, and will be removed in a"
+            " future release.", DeprecationWarning, stacklevel=2)
+        with warnings.catch_warnings():
+            # Don't let the reverter code warn about this function. Calling
+            # this function in the first place is the real problem.
+            warnings.filterwarnings("ignore", ".*view_config_changes", DeprecationWarning)
+            try:
+                self.reverter.view_config_changes()
+            except errors.ReverterError as err:
+                raise errors.PluginError(str(err))
 
     @property
     def ssl_dhparams(self):

certbot/plugins/common_test.py

@@ -3,6 +3,7 @@
 import shutil
 import tempfile
 import unittest
+import warnings
 
 import OpenSSL
 import josepy as jose
@@ -97,9 +98,8 @@ def setUp(self):
         os.mkdir(self.config.config_dir)
         from certbot.plugins.common import Installer
 
-        with mock.patch("certbot.plugins.common.reverter.Reverter"):
-            self.installer = Installer(config=self.config,
-                                       name="Installer")
+        self.installer = Installer(config=self.config,
+                                   name="Installer")
         self.reverter = self.installer.reverter
 
     def test_add_to_real_checkpoint(self):
@@ -121,12 +121,11 @@ def _test_add_to_checkpoint_common(self, temporary):
                                            temporary=temporary)
 
         if temporary:
-            reverter_func = self.reverter.add_to_temp_checkpoint
+            reverter_func_name = "add_to_temp_checkpoint"
         else:
-            reverter_func = self.reverter.add_to_checkpoint
+            reverter_func_name = "add_to_checkpoint"
 
-        self._test_adapted_method(
-            installer_func, reverter_func, files, save_notes)
+        self._test_adapted_method(installer_func, reverter_func_name, files, save_notes)
 
     def test_finalize_checkpoint(self):
         self._test_wrapped_method("finalize_checkpoint", "foo")
@@ -143,6 +142,19 @@ def test_rollback_checkpoints(self):
     def test_view_config_changes(self):
         self._test_wrapped_method("view_config_changes")
 
+    def test_view_config_changes_warning_supression(self):
+        with warnings.catch_warnings():
+            # Without the catch_warnings() code in
+            # common.Installer.view_config_changes, this would raise an
+            # exception. The module parameter here is ".*common$" because the
+            # stacklevel=2 parameter of warnings.warn causes the warning to
+            # refer to the code in the caller rather than the call to
+            # warnings.warn. This means the warning in common.Installer refers
+            # to this module and the warning in the reverter refers to the
+            # plugins.common module.
+            warnings.filterwarnings("error", ".*view_config_changes", module=".*common$")
+            self.installer.view_config_changes()
+
     def _test_wrapped_method(self, name, *args, **kwargs):
         """Test a wrapped reverter method.
 
@@ -152,28 +164,27 @@ def _test_wrapped_method(self, name, *args, **kwargs):
 
         """
         installer_func = getattr(self.installer, name)
-        reverter_func = getattr(self.reverter, name)
-        self._test_adapted_method(
-            installer_func, reverter_func, *args, **kwargs)
+        self._test_adapted_method(installer_func, name, *args, **kwargs)
 
     def _test_adapted_method(self, installer_func,
-                             reverter_func, *passed_args, **passed_kwargs):
+                             reverter_func_name, *passed_args, **passed_kwargs):
         """Test an adapted reverter method
 
         :param callable installer_func: installer method to test
-        :param mock.MagicMock reverter_func: mocked adapated
-            reverter method
+        :param str reverter_func_name: name of the method on the
+            reverter that should be called
         :param tuple passed_args: positional arguments passed from
             installer method to the reverter method
         :param dict passed_kargs: keyword arguments passed from
             installer method to the reverter method
 
         """
-        installer_func(*passed_args, **passed_kwargs)
-        reverter_func.assert_called_once_with(*passed_args, **passed_kwargs)
-        reverter_func.side_effect = errors.ReverterError
-        self.assertRaises(
-            errors.PluginError, installer_func, *passed_args, **passed_kwargs)
+        with mock.patch.object(self.reverter, reverter_func_name) as reverter_func:
+            installer_func(*passed_args, **passed_kwargs)
+            reverter_func.assert_called_once_with(*passed_args, **passed_kwargs)
+            reverter_func.side_effect = errors.ReverterError
+            self.assertRaises(
+                errors.PluginError, installer_func, *passed_args, **passed_kwargs)
 
     def test_install_ssl_dhparams(self):
         self.installer.install_ssl_dhparams()

certbot/plugins/null.py

@@ -49,9 +49,6 @@ def rollback_checkpoints(self, rollback=1):
     def recovery_routine(self):
         pass  # pragma: no cover
 
-    def view_config_changes(self):
-        pass  # pragma: no cover
-
     def config_test(self):
         pass  # pragma: no cover
 

certbot/reverter.py

@@ -6,6 +6,7 @@
 import sys
 import time
 import traceback
+import warnings
 
 import six
 import zope.component
@@ -143,6 +144,12 @@ def view_config_changes(self):
         :raises .errors.ReverterError: If invalid directory structure.
 
         """
+        warnings.warn(
+            "The view_config_changes method has been deprecated and will be"
+            " removed in a future release. If you were using this method to"
+            " implement the view_config_changes method of IInstaller, know that"
+            " that method has been removed from the plugin interface and is no"
+            " longer used by Certbot.", DeprecationWarning, stacklevel=2)
         backups = os.listdir(self.config.backup_dir)
         backups.sort(reverse=True)
         if not backups:

certbot/tests/main_test.py

@@ -757,6 +757,13 @@ def test_config_changes(self):
         _, _, _, client = self._call(['config_changes'])
         self.assertEqual(1, client.view_config_changes.call_count)
 
+    @mock.patch('certbot.main.logger.warning')
+    def test_config_changes_deprecation(self, mock_warning):
+        self._call(['config_changes'])
+        self.assertTrue(mock_warning.called)
+        msg = mock_warning.call_args[0][0]
+        self.assertIn("config_changes subcommand has been deprecated", msg)
+
     @mock.patch('certbot.cert_manager.update_live_symlinks')
     def test_update_symlinks(self, mock_cert_manager):
         self._call_no_clientmock(['update_symlinks'])

pytest.ini

@@ -10,9 +10,11 @@
 #      but it should be corrected to allow Certbot compatiblity with Python >= 3.8
 #   4- ipdb uses deprecated functionality of IPython. See
 #      https://github.com/gotcha/ipdb/issues/144.
+#   5- ignore our own warnings about deprecated view_config_changes methods
 filterwarnings =
     error
     ignore:decodestring:DeprecationWarning
     ignore:(TLSSNI01|TLS-SNI-01):DeprecationWarning
     ignore:.*collections\.abc:DeprecationWarning
     ignore:The `color_scheme` argument is deprecated:DeprecationWarning:IPython.*
+    ignore:.*view_config_changes:DeprecationWarning