Deprecate config_changes (#7469)

Closes #7454

* Deprecate config_changes

* Error on config_changes

* Fix tests for main.py

* Fix CHANGELOG entry

* Remove remnants of config_changes

* Fix CHANGELOG and add removed functions

CHANGELOG.md

@@ -12,6 +12,11 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 * Added back support for Python 3.4 to Certbot components and certbot-auto due
   to a bug when requiring Python 2.7 or 3.5+ on RHEL 6 based systems.
+* Certbot's `config_changes` subcommand has been removed
+* The functions `certbot.client.view_config_changes`,
+  `certbot.main.config_changes`,
+  `certbot.plugins.common.Installer.view_config_changes`, and
+  `certbot.reverter.Reverter.view_config_changes` have been removed
 
 ### Fixed
 

certbot/cli.py

@@ -115,7 +115,7 @@ More detailed help:
    all, automation, commands, paths, security, testing, or any of the
    subcommands or plugins (certonly, renew, install, register, nginx,
    apache, standalone, webroot, etc.)
-  -h all                print a detailed help page including all topics 
+  -h all                print a detailed help page including all topics
   --version             print the version number
 """
 
@@ -398,11 +398,6 @@ VERB_HELP = [
         "usage": "\n\n  certbot install --cert-path /path/to/fullchain.pem "
         " --key-path /path/to/private-key [options]\n\n"
     }),
-    ("config_changes", {
-        "short": "Show changes that Certbot has made to server configurations",
-        "opts": "Options for viewing configuration changes",
-        "usage": "\n\n  certbot config_changes [options]\n\n"
-    }),
     ("rollback", {
         "short": "Roll back server conf changes made during certificate installation",
         "opts": "Options for rolling back server configuration changes",
@@ -447,7 +442,6 @@ class HelpfulArgumentParser(object):
         self.VERBS = {
             "auth": main.certonly,
             "certonly": main.certonly,
-            "config_changes": main.config_changes,
             "run": main.run,
             "install": main.install,
             "plugins": main.plugins_cmd,

certbot/client.py

@@ -27,7 +27,6 @@ from certbot import eff
 from certbot import error_handler
 from certbot import errors
 from certbot import interfaces
-from certbot import reverter
 from certbot import storage
 from certbot import util
 from certbot.compat import os
@@ -710,20 +709,6 @@ def rollback(default_installer, checkpoints, config, plugins):
         installer.rollback_checkpoints(checkpoints)
         installer.restart()
 
-
-def view_config_changes(config):
-    """View checkpoints and associated configuration changes.
-
-    .. note:: This assumes that the installation is using a Reverter object.
-
-    :param config: Configuration.
-    :type config: :class:`certbot.interfaces.IConfig`
-
-    """
-    rev = reverter.Reverter(config)
-    rev.recovery_routine()
-    rev.view_config_changes()
-
 def _open_pem_file(cli_arg_path, pem_path):
     """Open a pem file.
 

certbot/main.py

@@ -961,26 +961,6 @@ def rollback(config, plugins):
     """
     client.rollback(config.installer, config.checkpoints, config, plugins)
 
-
-def config_changes(config, unused_plugins):
-    """Show changes made to server config during installation
-
-    View checkpoints and associated configuration changes.
-
-    :param config: Configuration object
-    :type config: interfaces.IConfig
-
-    :param unused_plugins: List of plugins (deprecated)
-    :type unused_plugins: `list` of `str`
-
-    :returns: `None`
-    :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):
     """Update the certificate file family symlinks
 

certbot/plugins/common.py

@@ -192,26 +192,6 @@ class Installer(Plugin):
         except errors.ReverterError as err:
             raise errors.PluginError(str(err))
 
-    def view_config_changes(self):
-        """Show all of the configuration changes that have taken place.
-
-        :raises .errors.PluginError: If there is a problem while processing
-            the checkpoints directories.
-
-        """
-        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):
         """Full absolute path to ssl_dhparams file."""

certbot/plugins/common_test.py

@@ -3,7 +3,6 @@ import functools
 import shutil
 import tempfile
 import unittest
-import warnings
 
 import OpenSSL
 import josepy as jose
@@ -140,22 +139,6 @@ class InstallerTest(test_util.ConfigTestCase):
     def test_rollback_checkpoints(self):
         self._test_wrapped_method("rollback_checkpoints", 42)
 
-    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.
 

certbot/reverter.py

@@ -6,14 +6,11 @@ import shutil
 import sys
 import time
 import traceback
-import warnings
 
 import six
-import zope.component
 
 from certbot import constants
 from certbot import errors
-from certbot import interfaces
 from certbot import util
 from certbot.compat import os
 from certbot.compat import filesystem
@@ -131,64 +128,6 @@ class Reverter(object):
                     "Unable to load checkpoint during rollback")
             rollback -= 1
 
-    def view_config_changes(self):
-        """Displays all saved checkpoints.
-
-        All checkpoints are printed by
-        :meth:`certbot.interfaces.IDisplay.notification`.
-
-        .. todo:: Decide on a policy for error handling, OSError IOError...
-
-        :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:
-            logger.info("Certbot has not saved backups of your configuration")
-
-            return None
-        # Make sure there isn't anything unexpected in the backup folder
-        # There should only be timestamped (float) directories
-        try:
-            for bkup in backups:
-                float(bkup)
-        except ValueError:
-            raise errors.ReverterError(
-                "Invalid directories in {0}".format(self.config.backup_dir))
-
-        output = []
-        for bkup in backups:
-            output.append(time.ctime(float(bkup)))
-            cur_dir = os.path.join(self.config.backup_dir, bkup)
-            with open(os.path.join(cur_dir, "CHANGES_SINCE")) as changes_fd:
-                output.append(changes_fd.read())
-
-            output.append("Affected files:")
-            with open(os.path.join(cur_dir, "FILEPATHS")) as paths_fd:
-                filepaths = paths_fd.read().splitlines()
-                for path in filepaths:
-                    output.append("  {0}".format(path))
-
-            if os.path.isfile(os.path.join(cur_dir, "NEW_FILES")):
-                with open(os.path.join(cur_dir, "NEW_FILES")) as new_fd:
-                    output.append("New Configuration Files:")
-                    filepaths = new_fd.read().splitlines()
-                    for path in filepaths:
-                        output.append("  {0}".format(path))
-
-            output.append('\n')
-
-        zope.component.getUtility(interfaces.IDisplay).notification(
-            '\n'.join(output), force_interactive=True, pause=False)
-        return None
-
     def add_to_temp_checkpoint(self, save_files, save_notes):
         """Add files to temporary checkpoint.
 

certbot/tests/cli_test.py

@@ -186,7 +186,7 @@ class ParseTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertTrue("--delete-after-revoke" in out)
         self.assertTrue("--no-delete-after-revoke" in out)
 
-        out = self._help_output(['-h', 'config_changes'])
+        out = self._help_output(['-h', 'register'])
         self.assertTrue("--cert-path" not in out)
         self.assertTrue("--key-path" not in out)
 

certbot/tests/main_test.py

@@ -753,17 +753,6 @@ class MainTest(test_util.ConfigTestCase):  # pylint: disable=too-many-public-met
         client.rollback.assert_called_once_with(
             mock.ANY, 123, mock.ANY, mock.ANY)
 
-    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'])

certbot/tests/reverter_test.py

@@ -10,7 +10,6 @@ import six
 
 from certbot import errors
 from certbot.compat import os
-from certbot.compat import filesystem
 from certbot.tests import util as test_util
 
 
@@ -377,30 +376,6 @@ class TestFullCheckpointsReverter(test_util.ConfigTestCase):
         self.assertEqual(read_in(self.config2), "directive-dir2")
         self.assertFalse(os.path.isfile(config3))
 
-    @test_util.patch_get_utility()
-    def test_view_config_changes(self, mock_output):
-        """This is not strict as this is subject to change."""
-        self._setup_three_checkpoints()
-
-        # Make sure it doesn't throw any errors
-        self.reverter.view_config_changes()
-
-        # Make sure notification is output
-        self.assertEqual(mock_output().notification.call_count, 1)
-
-    @mock.patch("certbot.reverter.logger")
-    def test_view_config_changes_no_backups(self, mock_logger):
-        self.reverter.view_config_changes()
-        self.assertTrue(mock_logger.info.call_count > 0)
-
-    def test_view_config_changes_bad_backups_dir(self):
-        # There shouldn't be any "in progress directories when this is called
-        # It must just be clean checkpoints
-        filesystem.makedirs(os.path.join(self.config.backup_dir, "in_progress"))
-
-        self.assertRaises(
-            errors.ReverterError, self.reverter.view_config_changes)
-
     def _setup_three_checkpoints(self):
         """Generate some finalized checkpoints."""
         # Checkpoint1 - config1

docs/cli-help.txt

@@ -392,9 +392,6 @@ unregister:
 install:
   Options for modifying how a certificate is deployed
 
-config_changes:
-  Options for viewing configuration changes
-
 rollback:
   Options for rolling back server configuration changes
 

pytest.ini

@@ -10,12 +10,10 @@
 #      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
     ignore:.*get_systemd_os_info:DeprecationWarning