Merge branch 'renew-no-dryrun-restarts' into ux

certbot/CHANGELOG.md

@@ -111,6 +111,7 @@ More details about these changes can be found on our GitHub repo.
 * Fixed the apache component on openSUSE Tumbleweed which no longer provides
   an apache2ctl symlink and uses apachectl instead.
 * Fixed a typo in `certbot/crypto_util.py` causing an error upon attempting `secp521r1` key generation
+* Installers (e.g. nginx, Apache) were being restarted unnecessarily after dry-run renewals.
 
 More details about these changes can be found on our GitHub repo.
 

certbot/certbot/_internal/main.py

@@ -1297,19 +1297,11 @@ def renew_cert(config, plugins, lineage):
 
     renewed_lineage = _get_and_save_cert(le_client, config, lineage=lineage)
 
-    notify = zope.component.getUtility(interfaces.IDisplay).notification
-    if installer is None:
-        notify("new certificate deployed without reload, fullchain is {0}".format(
-               lineage.fullchain), pause=False)
-    else:
+    if installer and not config.dry_run:
         # In case of a renewal, reload server to pick up new certificate.
-        # In principle we could have a configuration option to inhibit this
-        # from happening.
-        # Run deployer
         updater.run_renewal_deployer(config, renewed_lineage, installer)
-        installer.restart()
-        notify("new certificate deployed with reload of {0} server; fullchain is {1}".format(
-               config.installer, lineage.fullchain), pause=False)
+        display_util.notify(f"Reloading {config.installer} server after certificate renewal")
+        installer.restart() # type: ignore
 
 
 def certonly(config, plugins):

certbot/certbot/_internal/plugins/selection.py

@@ -2,10 +2,12 @@
 
 import logging
 
+from typing import Optional, Tuple
 import zope.component
 
 from certbot import errors
 from certbot import interfaces
+from certbot._internal.plugins import disco
 from certbot.compat import os
 from certbot.display import util as display_util
 
@@ -164,7 +166,9 @@ def record_chosen_plugins(config, plugins, auth, inst):
          config.authenticator, config.installer)
 
 
-def choose_configurator_plugins(config, plugins, verb):
+def choose_configurator_plugins(config: interfaces.IConfig, plugins: disco.PluginsRegistry,
+                                verb: str) -> Tuple[Optional[interfaces.IInstaller],
+                                                    Optional[interfaces.IAuthenticator]]:
     """
     Figure out which configurator we're going to use, modifies
     config.authenticator and config.installer strings to reflect that choice if
@@ -172,7 +176,7 @@ def choose_configurator_plugins(config, plugins, verb):
 
     :raises errors.PluginSelectionError if there was a problem
 
-    :returns: (an `IAuthenticator` or None, an `IInstaller` or None)
+    :returns: tuple of (`IInstaller` or None, `IAuthenticator` or None)
     :rtype: tuple
     """
 

certbot/tests/main_test.py

@@ -1496,6 +1496,24 @@ class MainTest(test_util.ConfigTestCase):
         # without installer
         self.assertIs(mock_run.called, False)
 
+    @mock.patch('certbot._internal.main.updater.run_renewal_deployer')
+    @mock.patch('certbot._internal.plugins.selection.choose_configurator_plugins')
+    @mock.patch('certbot._internal.main._init_le_client')
+    @mock.patch('certbot._internal.main._get_and_save_cert')
+    def test_renew_doesnt_restart_on_dryrun(self, mock_get_cert, mock_init, mock_choose,
+                                            mock_run_renewal_deployer):
+        """A dry-run renewal shouldn't try to restart the installer"""
+        self.config.dry_run = True
+        installer = mock.MagicMock()
+        mock_choose.return_value = (installer, mock.MagicMock())
+
+        main.renew_cert(self.config, None, None)
+
+        self.assertEqual(mock_init.call_count, 1)
+        self.assertEqual(mock_get_cert.call_count, 1)
+        installer.restart.assert_not_called()
+        mock_run_renewal_deployer.assert_not_called()
+
 
 class UnregisterTest(unittest.TestCase):
     def setUp(self):