cli: clean up certbot renew summary (#8503)

* cli: clean up `certbot renew` summary

- Unduplicate output which was being sent to both stdout and stderr
- Don't use IDisplay.notification to buffer output
- Remove big "DRY RUN" guards above and below, instead change language
  to "renewal" or "simulated renewal"
- Reword "Attempting to renew cert ... produced an unexpected error"
  to be more concise.

* add newline to docstring

Co-authored-by: ohemorange <ebportnoy@gmail.com>

Co-authored-by: ohemorange <ebportnoy@gmail.com>

certbot/certbot/_internal/renewal.py

@@ -19,6 +19,7 @@ import zope.component
 from acme.magic_typing import List
 from acme.magic_typing import Optional  # pylint: disable=unused-import
 from certbot import crypto_util
+from certbot.display import util as display_util
 from certbot import errors
 from certbot import interfaces
 from certbot import util
@@ -347,40 +348,42 @@ def report(msgs, category):
 
 def _renew_describe_results(config, renew_successes, renew_failures,
                             renew_skipped, parse_failures):
+    # type: (interfaces.IConfig, List[str], List[str], List[str], List[str]) -> None
+    """
+    Print a report to the terminal about the results of the renewal process.
 
-    out = []  # type: List[str]
+    :param interfaces.IConfig config: Configuration
-    notify = out.append
+    :param list renew_successes: list of fullchain paths which were renewed
-    disp = zope.component.getUtility(interfaces.IDisplay)
+    :param list renew_failures: list of fullchain paths which failed to be renewed
+    :param list renew_skipped: list of messages to print about skipped certificates
+    :param list parse_failures: list of renewal parameter paths which had erorrs
+    """
+    notify = display_util.notify
+    notify_error = logger.error
 
-    def notify_error(err):
+    notify('\n{}'.format(display_util.SIDE_FRAME))
-        """Notify and log errors."""
+
-        notify(str(err))
+    renewal_noun = "simulated renewal" if config.dry_run else "renewal"
-        logger.error(err)
 
-    if config.dry_run:
-        notify("** DRY RUN: simulating 'certbot renew' close to cert expiry")
-        notify("**          (The test certificates below have not been saved.)")
-    notify("")
     if renew_skipped:
         notify("The following certs are not due for renewal yet:")
         notify(report(renew_skipped, "skipped"))
     if not renew_successes and not renew_failures:
-        notify("No renewals were attempted.")
+        notify("No {renewal}s were attempted.".format(renewal=renewal_noun))
         if (config.pre_hook is not None or
                 config.renew_hook is not None or config.post_hook is not None):
             notify("No hooks were run.")
     elif renew_successes and not renew_failures:
-        notify("Congratulations, all renewals succeeded. The following certs "
+        notify("Congratulations, all {renewal}s succeeded: ".format(renewal=renewal_noun))
-               "have been renewed:")
         notify(report(renew_successes, "success"))
     elif renew_failures and not renew_successes:
-        notify_error("All renewal attempts failed. The following certs could "
+        notify_error("All %ss failed. The following certs could "
-               "not be renewed:")
+               "not be renewed:", renewal_noun)
         notify_error(report(renew_failures, "failure"))
     elif renew_failures and renew_successes:
-        notify("The following certs were successfully renewed:")
+        notify("The following {renewal}s succeeded:".format(renewal=renewal_noun))
         notify(report(renew_successes, "success") + "\n")
-        notify_error("The following certs could not be renewed:")
+        notify_error("The following %ss failed:", renewal_noun)
         notify_error(report(renew_failures, "failure"))
 
     if parse_failures:
@@ -388,11 +391,7 @@ def _renew_describe_results(config, renew_successes, renew_failures,
                "were invalid: ")
         notify(report(parse_failures, "parsefail"))
 
-    if config.dry_run:
+    notify(display_util.SIDE_FRAME)
-        notify("** DRY RUN: simulating 'certbot renew' close to cert expiry")
-        notify("**          (The test certificates above have not been saved.)")
-
-    disp.notification("\n".join(out), wrap=False)
 
 
 def handle_renewal_request(config):
@@ -482,9 +481,10 @@ def handle_renewal_request(config):
 
         except Exception as e:  # pylint: disable=broad-except
             # obtain_cert (presumably) encountered an unanticipated problem.
-            logger.warning("Attempting to renew cert (%s) from %s produced an "
+            logger.error(
-                           "unexpected error: %s. Skipping.", lineagename,
+                "Failed to renew cert %s with error: %s",
-                               renewal_file, e)
+                lineagename, e
+            )
             logger.debug("Traceback was:\n%s", traceback.format_exc())
             renew_failures.append(renewal_candidate.fullchain)
 

certbot/tests/renewal_test.py

@@ -163,5 +163,70 @@ class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase):
         self.assertEqual(self.config.server, constants.CLI_DEFAULTS['server'])
 
 
+class DescribeResultsTest(unittest.TestCase):
+    """Tests for certbot._internal.renewal._renew_describe_results."""
+    def setUp(self):
+        self.patchers = {
+            'log_error': mock.patch('certbot._internal.renewal.logger.error'),
+            'notify': mock.patch('certbot._internal.renewal.display_util.notify')}
+        self.mock_notify = self.patchers['notify'].start()
+        self.mock_error = self.patchers['log_error'].start()
+
+    def tearDown(self):
+        for patch in self.patchers.values():
+            patch.stop()
+
+    @classmethod
+    def _call(cls, *args, **kwargs):
+        from certbot._internal.renewal import _renew_describe_results
+        _renew_describe_results(*args, **kwargs)
+
+    def _assert_success_output(self, lines):
+        self.mock_notify.assert_has_calls([mock.call(l) for l in lines])
+
+    def test_no_renewal_attempts(self):
+        self._call(mock.MagicMock(dry_run=True), [], [], [], [])
+        self._assert_success_output(['No simulated renewals were attempted.'])
+
+    def test_successful_renewal(self):
+        self._call(mock.MagicMock(dry_run=False), ['good.pem'], None, None, None)
+        self._assert_success_output([
+            '\n- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -',
+            'Congratulations, all renewals succeeded: ',
+            '  good.pem (success)',
+            '- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -',
+        ])
+
+    def test_failed_renewal(self):
+        self._call(mock.MagicMock(dry_run=False), [], ['bad.pem'], [], [])
+        self._assert_success_output([
+            '\n- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -',
+            '- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -',
+        ])
+        self.mock_error.assert_has_calls([
+            mock.call('All %ss failed. The following certs could not be renewed:', 'renewal'),
+            mock.call('  bad.pem (failure)'),
+        ])
+
+    def test_all_renewal(self):
+        self._call(mock.MagicMock(dry_run=True),
+                   ['good.pem', 'good2.pem'], ['bad.pem', 'bad2.pem'],
+                   ['foo.pem expires on 123'], ['errored.conf'])
+        self._assert_success_output([
+            '\n- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -',
+            'The following certs are not due for renewal yet:',
+            '  foo.pem expires on 123 (skipped)',
+            'The following simulated renewals succeeded:',
+            '  good.pem (success)\n  good2.pem (success)\n',
+            '\nAdditionally, the following renewal configurations were invalid: ',
+            '  errored.conf (parsefail)',
+            '- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -',
+        ])
+        self.mock_error.assert_has_calls([
+            mock.call('The following %ss failed:', 'simulated renewal'),
+            mock.call('  bad.pem (failure)\n  bad2.pem (failure)'),
+        ])
+
+
 if __name__ == "__main__":
     unittest.main()  # pragma: no cover