diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fec8b7c6..fc3acb665 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/certbot-apache/certbot_apache/tests/configurator_reverter_test.py b/certbot-apache/certbot_apache/tests/configurator_reverter_test.py index 114f253e6..f3c418a3a 100644 --- a/certbot-apache/certbot_apache/tests/configurator_reverter_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_reverter_test.py @@ -74,14 +74,6 @@ class ConfiguratorReverterTest(util.ApacheTest): 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 diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 5e9d61a44..06c8c53c9 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -427,11 +427,6 @@ class NginxConfiguratorTest(util.NginxTest): 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") diff --git a/certbot/cli.py b/certbot/cli.py index 9f11e5d9b..d22a9a524 100644 --- a/certbot/cli.py +++ b/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", diff --git a/certbot/interfaces.py b/certbot/interfaces.py index 25037a332..2e2df8a73 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -355,13 +355,6 @@ class IInstaller(IPlugin): """ - 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. diff --git a/certbot/main.py b/certbot/main.py index d071ee453..6bd47cee3 100644 --- a/certbot/main.py +++ b/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): diff --git a/certbot/plugins/common.py b/certbot/plugins/common.py index 820e86a13..2e2e0f64c 100644 --- a/certbot/plugins/common.py +++ b/certbot/plugins/common.py @@ -3,6 +3,7 @@ import logging import re import shutil import tempfile +import warnings import OpenSSL import pkg_resources @@ -197,10 +198,18 @@ class Installer(Plugin): 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): diff --git a/certbot/plugins/common_test.py b/certbot/plugins/common_test.py index bce8f833a..ce60046af 100644 --- a/certbot/plugins/common_test.py +++ b/certbot/plugins/common_test.py @@ -3,6 +3,7 @@ import functools import shutil import tempfile import unittest +import warnings import OpenSSL import josepy as jose @@ -97,9 +98,8 @@ class InstallerTest(test_util.ConfigTestCase): 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 @@ class InstallerTest(test_util.ConfigTestCase): 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 @@ class InstallerTest(test_util.ConfigTestCase): 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 @@ class InstallerTest(test_util.ConfigTestCase): """ 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() diff --git a/certbot/plugins/null.py b/certbot/plugins/null.py index 87c0737a5..6deb358f1 100644 --- a/certbot/plugins/null.py +++ b/certbot/plugins/null.py @@ -49,9 +49,6 @@ class Installer(common.Plugin): 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 diff --git a/certbot/reverter.py b/certbot/reverter.py index 201bdc03d..21d33806a 100644 --- a/certbot/reverter.py +++ b/certbot/reverter.py @@ -6,6 +6,7 @@ import shutil import sys import time import traceback +import warnings import six import zope.component @@ -143,6 +144,12 @@ class Reverter(object): :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: diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index bdc42a62f..2cc42cc88 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -757,6 +757,13 @@ class MainTest(test_util.ConfigTestCase): # pylint: disable=too-many-public-met _, _, _, 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/pytest.ini b/pytest.ini index 2531e50d2..27a0f2b74 100644 --- a/pytest.ini +++ b/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