From bf20f39ceb7297dfbde8bb1c2f6e9d330c2ceea6 Mon Sep 17 00:00:00 2001 From: alexzorin Date: Tue, 10 Nov 2020 10:31:27 +1100 Subject: [PATCH] cli: miscellaneous IReporter removals (#8436) * certbot delete: use undecorated print * certbot revoke: use undecorated print * certbot revoke: remove ireporter usages * eff: remove IReporter usages * certbot unregister: remove IReporter usage * certbot update_account: remove IReporter usages * certbot run: remove IReporter in duplicate prompt * fix test_revoke_multiple_lineages --- .../certbot_tests/test_main.py | 3 +- certbot/certbot/_internal/cert_manager.py | 4 +-- certbot/certbot/_internal/eff.py | 4 +-- certbot/certbot/_internal/main.py | 23 +++++------- certbot/certbot/display/ops.py | 10 +++--- certbot/tests/cert_manager_test.py | 6 +++- certbot/tests/display/ops_test.py | 12 +++---- certbot/tests/eff_test.py | 35 ++++++++++--------- certbot/tests/main_test.py | 25 ++++++------- 9 files changed, 59 insertions(+), 63 deletions(-) diff --git a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py index caef80af3..cfafce732 100644 --- a/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py +++ b/certbot-ci/certbot_integration_tests/certbot_tests/test_main.py @@ -544,7 +544,8 @@ def test_revoke_multiple_lineages(context): 'revoke', '--cert-path', join(context.config_dir, 'live', cert1, 'cert.pem') ]) - assert 'Not deleting revoked certs due to overlapping archive dirs' in output + with open(join(context.workspace, 'logs', 'letsencrypt.log'), 'r') as f: + assert 'Not deleting revoked certs due to overlapping archive dirs' in f.read() def test_wildcard_certificates(context): diff --git a/certbot/certbot/_internal/cert_manager.py b/certbot/certbot/_internal/cert_manager.py index 5dc909330..5c1c515c2 100644 --- a/certbot/certbot/_internal/cert_manager.py +++ b/certbot/certbot/_internal/cert_manager.py @@ -100,8 +100,8 @@ def delete(config): return for certname in certnames: storage.delete_files(config, certname) - disp.notification("Deleted all files relating to certificate {0}." - .format(certname), pause=False) + display_util.notify("Deleted all files relating to certificate {0}." + .format(certname)) ################### # Public Helpers diff --git a/certbot/certbot/_internal/eff.py b/certbot/certbot/_internal/eff.py index ae8690821..5fbbd302a 100644 --- a/certbot/certbot/_internal/eff.py +++ b/certbot/certbot/_internal/eff.py @@ -7,6 +7,7 @@ import zope.component from acme.magic_typing import Optional # pylint: disable=unused-import from certbot import interfaces +from certbot.display import util as display_util from certbot._internal import constants from certbot._internal.account import Account # pylint: disable=unused-import from certbot._internal.account import AccountFileStorage @@ -133,5 +134,4 @@ def _report_failure(reason=None): msg.append(' because ') msg.append(reason) msg.append('. You can try again later by visiting https://act.eff.org.') - reporter = zope.component.getUtility(interfaces.IReporter) - reporter.add_message(''.join(msg), reporter.LOW_PRIORITY) + display_util.notify(''.join(msg)) diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 71b85c4cb..e02737c4c 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -163,8 +163,7 @@ def _handle_subset_cert_request(config, domains, cert): cli_flag="--expand", force_interactive=True): return "renew", cert - reporter_util = zope.component.getUtility(interfaces.IReporter) - reporter_util.add_message( + display_util.notify( "To obtain a new certificate that contains these names without " "replacing your existing certificate for {0}, you must use the " "--duplicate option.{br}{br}" @@ -172,8 +171,7 @@ def _handle_subset_cert_request(config, domains, cert): existing, sys.argv[0], " ".join(sys.argv[1:]), br=os.linesep - ), - reporter_util.HIGH_PRIORITY) + )) raise errors.Error(USER_CANCELLED) @@ -542,7 +540,6 @@ def _delete_if_appropriate(config): archive dir is found for the specified lineage, etc ... """ display = zope.component.getUtility(interfaces.IDisplay) - reporter_util = zope.component.getUtility(interfaces.IReporter) attempt_deletion = config.delete_after_revoke if attempt_deletion is None: @@ -552,7 +549,6 @@ def _delete_if_appropriate(config): force_interactive=True, default=True) if not attempt_deletion: - reporter_util.add_message("Not deleting revoked certs.", reporter_util.LOW_PRIORITY) return # config.cert_path must have been set @@ -570,9 +566,8 @@ def _delete_if_appropriate(config): cert_manager.match_and_check_overlaps(config, [lambda x: archive_dir], lambda x: x.archive_dir, lambda x: x) except errors.OverlappingMatchFound: - msg = ('Not deleting revoked certs due to overlapping archive dirs. More than ' - 'one lineage is using {0}'.format(archive_dir)) - reporter_util.add_message(''.join(msg), reporter_util.MEDIUM_PRIORITY) + logger.warning("Not deleting revoked certs due to overlapping archive dirs. More than " + "one certificate is using %s", archive_dir) return except Exception as e: msg = ('config.default_archive_dir: {0}, config.live_dir: {1}, archive_dir: {2},' @@ -625,7 +620,6 @@ def unregister(config, unused_plugins): """ account_storage = account.AccountFileStorage(config) accounts = account_storage.find_all() - reporter_util = zope.component.getUtility(interfaces.IReporter) if not accounts: return "Could not find existing account to deactivate." @@ -647,7 +641,7 @@ def unregister(config, unused_plugins): # delete local account files account_files.delete(config.account) - reporter_util.add_message("Account deactivated.", reporter_util.MEDIUM_PRIORITY) + display_util.notify("Account deactivated.") return None @@ -698,8 +692,6 @@ def update_account(config, unused_plugins): # exist or not. account_storage = account.AccountFileStorage(config) accounts = account_storage.find_all() - reporter_util = zope.component.getUtility(interfaces.IReporter) - add_msg = lambda m: reporter_util.add_message(m, reporter_util.MEDIUM_PRIORITY) if not accounts: return "Could not find an existing account to update." @@ -724,10 +716,11 @@ def update_account(config, unused_plugins): account_storage.update_regr(acc, cb_client.acme) if config.email is None: - add_msg("Any contact information associated with this account has been removed.") + display_util.notify("Any contact information associated " + "with this account has been removed.") else: eff.prepare_subscription(config, acc) - add_msg("Your e-mail address was updated to {0}.".format(config.email)) + display_util.notify("Your e-mail address was updated to {0}.".format(config.email)) return None diff --git a/certbot/certbot/display/ops.py b/certbot/certbot/display/ops.py index 7a72e336b..fa08d142b 100644 --- a/certbot/certbot/display/ops.py +++ b/certbot/certbot/display/ops.py @@ -262,17 +262,15 @@ def success_renewal(domains): def success_revocation(cert_path): - """Display a box confirming a certificate has been revoked. + """Display a message confirming a certificate has been revoked. :param list cert_path: path to certificate which was revoked. """ - z_util(interfaces.IDisplay).notification( + display_util.notify( "Congratulations! You have successfully revoked the certificate " - "that was located at {0}{1}{1}".format( - cert_path, - os.linesep), - pause=False) + "that was located at {0}.".format(cert_path) + ) def _gen_https_names(domains): diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index 4b56944dc..a551e4400 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -114,16 +114,20 @@ class DeleteTest(storage_test.BaseRenewableCertTest): cert_manager.delete(self.config) @test_util.patch_get_utility() + @mock.patch('certbot.display.util.notify') @mock.patch('certbot._internal.cert_manager.lineage_for_certname') @mock.patch('certbot._internal.storage.delete_files') def test_delete_from_config_yes(self, mock_delete_files, mock_lineage_for_certname, - mock_util): + mock_notify, mock_util): """Test delete""" mock_lineage_for_certname.return_value = self.test_rc mock_util().yesno.return_value = True self.config.certname = "example.org" self._call() mock_delete_files.assert_called_once_with(self.config, "example.org") + mock_notify.assert_called_once_with( + "Deleted all files relating to certificate example.org." + ) @test_util.patch_get_utility() @mock.patch('certbot._internal.cert_manager.lineage_for_certname') diff --git a/certbot/tests/display/ops_test.py b/certbot/tests/display/ops_test.py index bdc6472bf..cdbcf1937 100644 --- a/certbot/tests/display/ops_test.py +++ b/certbot/tests/display/ops_test.py @@ -384,16 +384,14 @@ class SuccessRevocationTest(unittest.TestCase): success_revocation(path) @test_util.patch_get_utility("certbot.display.ops.z_util") - def test_success_revocation(self, mock_util): - mock_util().notification.return_value = None + @mock.patch("certbot.display.util.notify") + def test_success_revocation(self, mock_notify, unused_mock_util): path = "/path/to/cert.pem" self._call(path) - mock_util().notification.assert_called_once_with( + mock_notify.assert_called_once_with( "Congratulations! You have successfully revoked the certificate " - "that was located at {0}{1}{1}".format( - path, - os.linesep), pause=False) - self.assertTrue(path in mock_util().notification.call_args[0][0]) + "that was located at {0}.".format(path) + ) class ValidatorTests(unittest.TestCase): diff --git a/certbot/tests/eff_test.py b/certbot/tests/eff_test.py index 00e479d77..9c8d2565e 100644 --- a/certbot/tests/eff_test.py +++ b/certbot/tests/eff_test.py @@ -43,11 +43,12 @@ class PrepareSubscriptionTest(SubscriptionTest): prepare_subscription(self.config, self.account) @test_util.patch_get_utility() - def test_failure(self, mock_get_utility): + @mock.patch("certbot._internal.eff.display_util.notify") + def test_failure(self, mock_notify, mock_get_utility): self.config.email = None self.config.eff_email = True self._call() - actual = mock_get_utility().add_message.call_args[0][0] + actual = mock_notify.call_args[0][0] expected_part = "because you didn't provide an e-mail address" self.assertTrue(expected_part in actual) self.assertIsNone(self.account.meta.register_to_eff) @@ -121,6 +122,10 @@ class SubscribeTest(unittest.TestCase): self.json = {'status': True} self.response = mock.Mock(ok=True) self.response.json.return_value = self.json + self.mock_notify = mock.patch("certbot._internal.eff.display_util.notify").start() + + def tearDown(self): + self.mock_notify.stop() @mock.patch('certbot._internal.eff.requests.post') def _call(self, mock_post): @@ -139,42 +144,38 @@ class SubscribeTest(unittest.TestCase): self.assertFalse(data is None) self.assertEqual(data.get('email'), self.email) - @test_util.patch_get_utility() - def test_bad_status(self, mock_get_utility): + def test_bad_status(self): self.json['status'] = False self._call() - actual = self._get_reported_message(mock_get_utility) + actual = self._get_reported_message() expected_part = 'because your e-mail address appears to be invalid.' self.assertTrue(expected_part in actual) - @test_util.patch_get_utility() - def test_not_ok(self, mock_get_utility): + def test_not_ok(self): self.response.ok = False self.response.raise_for_status.side_effect = requests.exceptions.HTTPError self._call() - actual = self._get_reported_message(mock_get_utility) + actual = self._get_reported_message() unexpected_part = 'because' self.assertFalse(unexpected_part in actual) - @test_util.patch_get_utility() - def test_response_not_json(self, mock_get_utility): + def test_response_not_json(self): self.response.json.side_effect = ValueError() self._call() - actual = self._get_reported_message(mock_get_utility) + actual = self._get_reported_message() expected_part = 'problem' self.assertTrue(expected_part in actual) - @test_util.patch_get_utility() - def test_response_json_missing_status_element(self, mock_get_utility): + def test_response_json_missing_status_element(self): self.json.clear() self._call() - actual = self._get_reported_message(mock_get_utility) + actual = self._get_reported_message() expected_part = 'problem' self.assertTrue(expected_part in actual) - def _get_reported_message(self, mock_get_utility): - self.assertTrue(mock_get_utility().add_message.called) - return mock_get_utility().add_message.call_args[0][0] + def _get_reported_message(self): + self.assertTrue(self.mock_notify.called) + return self.mock_notify.call_args[0][0] @test_util.patch_get_utility() def test_subscribe(self, mock_get_utility): diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 2eb2b3e58..63307009d 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -339,23 +339,23 @@ class DeleteIfAppropriateTest(test_util.ConfigTestCase): from certbot._internal.main import _delete_if_appropriate _delete_if_appropriate(mock_config) - def _test_delete_opt_out_common(self, mock_get_utility): + def _test_delete_opt_out_common(self): with mock.patch('certbot._internal.cert_manager.delete') as mock_delete: self._call(self.config) mock_delete.assert_not_called() - self.assertTrue(mock_get_utility().add_message.called) @test_util.patch_get_utility() - def test_delete_flag_opt_out(self, mock_get_utility): + def test_delete_flag_opt_out(self, unused_mock_get_utility): self.config.delete_after_revoke = False - self._test_delete_opt_out_common(mock_get_utility) + self._test_delete_opt_out_common() @test_util.patch_get_utility() def test_delete_prompt_opt_out(self, mock_get_utility): util_mock = mock_get_utility() util_mock.yesno.return_value = False - self._test_delete_opt_out_common(mock_get_utility) + self._test_delete_opt_out_common() + @mock.patch("certbot._internal.main.logger.warning") @mock.patch('certbot._internal.storage.renewal_file_for_certname') @mock.patch('certbot._internal.cert_manager.delete') @mock.patch('certbot._internal.cert_manager.match_and_check_overlaps') @@ -365,7 +365,7 @@ class DeleteIfAppropriateTest(test_util.ConfigTestCase): def test_overlapping_archive_dirs(self, mock_get_utility, mock_cert_path_to_lineage, mock_archive, mock_match_and_check_overlaps, mock_delete, - mock_renewal_file_for_certname): + mock_renewal_file_for_certname, mock_warning): # pylint: disable = unused-argument config = self.config config.cert_path = "/some/reasonable/path" @@ -374,6 +374,7 @@ class DeleteIfAppropriateTest(test_util.ConfigTestCase): mock_match_and_check_overlaps.side_effect = errors.OverlappingMatchFound() self._call(config) mock_delete.assert_not_called() + self.assertEqual(mock_warning.call_count, 1) @mock.patch('certbot._internal.storage.renewal_file_for_certname') @mock.patch('certbot._internal.cert_manager.match_and_check_overlaps') @@ -1448,9 +1449,10 @@ class MainTest(test_util.ConfigTestCase): # ensure we didn't try to subscribe (no email to subscribe with) self.assertFalse(mock_prepare.called) + @mock.patch("certbot._internal.main.display_util.notify") @mock.patch('certbot._internal.main.display_ops.get_email') @test_util.patch_get_utility() - def test_update_account_with_email(self, mock_utility, mock_email): + def test_update_account_with_email(self, mock_utility, mock_email, mock_notify): email = "user@example.com" mock_email.return_value = email with mock.patch('certbot._internal.eff.prepare_subscription') as mock_prepare: @@ -1475,7 +1477,7 @@ class MainTest(test_util.ConfigTestCase): # and we saved the updated registration on disk self.assertTrue(mocked_storage.update_regr.called) self.assertTrue( - email in mock_utility().add_message.call_args[0][0]) + email in mock_notify.call_args[0][0]) self.assertTrue(mock_prepare.called) @mock.patch('certbot._internal.plugins.selection.choose_configurator_plugins') @@ -1517,7 +1519,8 @@ class UnregisterTest(unittest.TestCase): res = main.unregister(config, unused_plugins) self.assertEqual(res, "Deactivation aborted.") - def test_unregister(self): + @mock.patch("certbot._internal.main.display_util.notify") + def test_unregister(self, mock_notify): mocked_storage = mock.MagicMock() mocked_storage.find_all.return_value = ["an account"] @@ -1533,9 +1536,7 @@ class UnregisterTest(unittest.TestCase): res = main.unregister(config, unused_plugins) self.assertTrue(res is None) - self.assertTrue(cb_client.acme.deactivate_registration.called) - m = "Account deactivated." - self.assertTrue(m in self.mocks['get_utility']().add_message.call_args[0][0]) + mock_notify.assert_called_once_with("Account deactivated.") def test_unregister_no_account(self): mocked_storage = mock.MagicMock()