From 1dff022d05fc05f2e1f6e0fb3d8782e8deb4bd81 Mon Sep 17 00:00:00 2001 From: Amjad Mashaal Date: Wed, 6 Nov 2019 21:29:07 +0200 Subject: [PATCH] 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 | 5 +++ certbot/cli.py | 8 +---- certbot/client.py | 15 --------- certbot/main.py | 20 ----------- certbot/plugins/common.py | 20 ----------- certbot/plugins/common_test.py | 17 ---------- certbot/reverter.py | 61 ---------------------------------- certbot/tests/cli_test.py | 2 +- certbot/tests/main_test.py | 11 ------ certbot/tests/reverter_test.py | 25 -------------- docs/cli-help.txt | 3 -- pytest.ini | 2 -- 12 files changed, 7 insertions(+), 182 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 806905564..cef8789a1 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/certbot/cli.py b/certbot/cli.py index 97a0abbfb..9b116fe3f 100644 --- a/certbot/cli.py +++ b/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, diff --git a/certbot/client.py b/certbot/client.py index 1800a301c..91b54b205 100644 --- a/certbot/client.py +++ b/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. diff --git a/certbot/main.py b/certbot/main.py index fc91aca5f..620e33be9 100644 --- a/certbot/main.py +++ b/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 diff --git a/certbot/plugins/common.py b/certbot/plugins/common.py index 50b07ad99..8123f0027 100644 --- a/certbot/plugins/common.py +++ b/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.""" diff --git a/certbot/plugins/common_test.py b/certbot/plugins/common_test.py index 80df666a5..94f7dd0be 100644 --- a/certbot/plugins/common_test.py +++ b/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. diff --git a/certbot/reverter.py b/certbot/reverter.py index f4f1c8c67..ac2804164 100644 --- a/certbot/reverter.py +++ b/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. diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 7f3830d72..7a2a7fabf 100644 --- a/certbot/tests/cli_test.py +++ b/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) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index d8c7f4ab8..05266835f 100644 --- a/certbot/tests/main_test.py +++ b/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']) diff --git a/certbot/tests/reverter_test.py b/certbot/tests/reverter_test.py index 31b0ea754..3ba628475 100644 --- a/certbot/tests/reverter_test.py +++ b/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 diff --git a/docs/cli-help.txt b/docs/cli-help.txt index 37539a24b..dfcd84ccd 100644 --- a/docs/cli-help.txt +++ b/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 diff --git a/pytest.ini b/pytest.ini index 12b081b0e..54ae4e9d6 100644 --- a/pytest.ini +++ b/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