Refactor post_hook storage during "renew"

certbot/hooks.py

@@ -9,7 +9,7 @@ from subprocess import Popen, PIPE
 from certbot import errors
 from certbot import util
 
-from certbot.plugins.util import path_surgery
+from certbot.plugins import util as plug_util
 
 logger = logging.getLogger(__name__)
 
@@ -23,7 +23,7 @@ def _prog(shell_cmd):
     """Extract the program run by a shell command"""
     cmd = util.which(shell_cmd)
     if not cmd:
-        path_surgery(shell_cmd)
+        plug_util.path_surgery(shell_cmd)
         cmd = util.which(shell_cmd)
 
     return os.path.basename(cmd) if cmd else None
@@ -55,33 +55,35 @@ def pre_hook(config):
 pre_hook.already = {}
 
 
-def post_hook(config, renew_final=False):
+def post_hook(config):
     """Run post hook if defined.
 
     If the verb is renew, we might have more certs to renew, so we wait until
-    we're called with renew_final=True before actually doing anything.
+    run_saved_post_hooks() is called.
     """
 
     cmd = config.post_hook
+    # In the "renew" case, we save these up to run at the end
     if config.verb == "renew":
-        if not renew_final:
+        if cmd and cmd not in post_hook.eventually:
-            if cmd and cmd not in post_hook.eventually:
+            post_hook.eventually.append(cmd)
-                post_hook.eventually.append(cmd)
+    # certonly / run
-        else:
+    elif cmd:
-            for cmd in post_hook.eventually:
+        logger.info("Running post-hook command: %s", cmd)
-                logger.info("Running post-hook command: %s", cmd)
+        _run_hook(cmd)
-                _run_hook(cmd)
-            if len(post_hook.eventually) == 0:
-                logger.info("No renewals attempted, so not running post-hook")
-    else: # certonly / run
-        if cmd:
-            logger.info("Running post-hook command: %s", cmd)
-            _run_hook(cmd)
 
 post_hook.eventually = []
 
+def run_saved_post_hooks():
+    """Run any post hooks that were saved up in the course of the 'renew' verb"""
+    for cmd in post_hook.eventually:
+        logger.info("Running post-hook command: %s", cmd)
+        _run_hook(cmd)
+    if len(post_hook.eventually) == 0:
+        logger.info("No renewals attempted, so not running post-hook")
+
 def renew_hook(config, domains, lineage_path):
-    "Run post-renewal hook if defined."
+    """Run post-renewal hook if defined."""
     if config.renew_hook:
         if not config.dry_run:
             os.environ["RENEWED_DOMAINS"] = " ".join(domains)

certbot/main.py

@@ -108,7 +108,7 @@ def _auth_from_available(le_client, config, domains=None, certname=None, lineage
             if lineage is False:
                 raise errors.Error("Certificate could not be obtained")
     finally:
-        hooks.post_hook(config, renew_final=False)
+        hooks.post_hook(config)
 
     if not config.dry_run and not config.verb == "renew":
         _report_new_cert(config, lineage.cert, lineage.fullchain)
@@ -634,7 +634,7 @@ def renew(config, unused_plugins):
     try:
         renewal.handle_renewal_request(config)
     finally:
-        hooks.post_hook(config, renew_final=True)
+        hooks.run_saved_post_hooks()
 
 
 def setup_log_file_handler(config, logfile, fmt):

certbot/tests/hook_test.py

@@ -27,9 +27,8 @@ class HookTest(unittest.TestCase):
         config = mock.MagicMock(pre_hook="explodinator", post_hook="", renew_hook="")
         self.assertRaises(errors.HookCommandNotFound, hooks.validate_hooks, config)
 
-
     @mock.patch('certbot.hooks.util.which')
-    @mock.patch('certbot.hooks.path_surgery')
+    @mock.patch('certbot.hooks.plug_util.path_surgery')
     def test_prog(self, mock_ps, mockwhich):
         mockwhich.return_value = "/very/very/funky"
         self.assertEqual(hooks._prog("funky"), "funky")
@@ -58,19 +57,30 @@ class HookTest(unittest.TestCase):
         config = mock.MagicMock(pre_hook="")
         self._test_a_hook(config, hooks.pre_hook, 0)
 
-    def test_post_hook(self):
+    def _test_renew_post_hooks(self, expected_count):
+        with mock.patch('certbot.hooks.logger.info') as mock_info:
+            with mock.patch('certbot.hooks._run_hook') as mock_run:
+                hooks.run_saved_post_hooks()
+                self.assertEqual(mock_run.call_count, expected_count)
+                if expected_count:
+                    self.assertEqual(mock_info.call_count, expected_count)
+                else:
+                    self.assertEqual(mock_info.call_count, 1)
+
+    def test_post_hooks(self):
         config = mock.MagicMock(post_hook="true", verb="splonk")
         self._test_a_hook(config, hooks.post_hook, 2)
+        self._test_renew_post_hooks(0)
 
         config = mock.MagicMock(post_hook="true", verb="renew")
         self._test_a_hook(config, hooks.post_hook, 0)
-        self._test_a_hook(config, hooks.post_hook, 2, renew_final=True)
+        self._test_renew_post_hooks(1)
         self._test_a_hook(config, hooks.post_hook, 0)
-        self._test_a_hook(config, hooks.post_hook, 2, renew_final=True)
+        self._test_renew_post_hooks(1)
 
         config = mock.MagicMock(post_hook="more_true", verb="renew")
         self._test_a_hook(config, hooks.post_hook, 0)
-        self._test_a_hook(config, hooks.post_hook, 4, renew_final=True)
+        self._test_renew_post_hooks(2)
 
     def test_renew_hook(self):
         with mock.patch.dict('os.environ', {}):