Make common Installer base class (#5055)

* Add installer class

* Add wrapped reverter methods to common.Installer.

* Use Installer class in Apache plugin

* Use Installer class in Nginx plugin

* Don't create reverter in Apache and Nginx plugins

certbot-apache/certbot_apache/augeas_configurator.py

@@ -3,7 +3,6 @@ import logging
 
 
 from certbot import errors
-from certbot import reverter
 from certbot.plugins import common
 
 from certbot_apache import constants
@@ -11,7 +10,7 @@ from certbot_apache import constants
 logger = logging.getLogger(__name__)
 
 
-class AugeasConfigurator(common.Plugin):
+class AugeasConfigurator(common.Installer):
     """Base Augeas Configurator class.
 
     :ivar config: Configuration.
@@ -33,11 +32,6 @@ class AugeasConfigurator(common.Plugin):
 
         self.save_notes = ""
 
-        # See if any temporary changes need to be recovered
-        # This needs to occur before VirtualHost objects are setup...
-        # because this will change the underlying configuration and potential
-        # vhosts
-        self.reverter = reverter.Reverter(self.config)
 
     def init_augeas(self):
         """ Initialize the actual Augeas instance """
@@ -50,6 +44,10 @@ class AugeasConfigurator(common.Plugin):
             flags=(augeas.Augeas.NONE |
                    augeas.Augeas.NO_MODL_AUTOLOAD |
                    augeas.Augeas.ENABLE_SPAN))
+        # See if any temporary changes need to be recovered
+        # This needs to occur before VirtualHost objects are setup...
+        # because this will change the underlying configuration and potential
+        # vhosts
         self.recovery_routine()
 
     def check_parsing_errors(self, lens):
@@ -124,17 +122,8 @@ class AugeasConfigurator(common.Plugin):
         if save_paths:
             for path in save_paths:
                 save_files.add(self.aug.get(path)[6:])
-
+            self.add_to_checkpoint(save_files,
-            try:
+                                   self.save_notes, temporary=temporary)
-                # Create Checkpoint
-                if temporary:
-                    self.reverter.add_to_temp_checkpoint(
-                        save_files, self.save_notes)
-                else:
-                    self.reverter.add_to_checkpoint(save_files,
-                                                    self.save_notes)
-            except errors.ReverterError as err:
-                raise errors.PluginError(str(err))
 
         self.aug.set("/augeas/save", save_state)
         self.save_notes = ""
@@ -147,10 +136,7 @@ class AugeasConfigurator(common.Plugin):
                 self.aug.remove("/files/"+sf)
             self.aug.load()
         if title and not temporary:
-            try:
+            self.finalize_checkpoint(title)
-                self.reverter.finalize_checkpoint(title)
-            except errors.ReverterError as err:
-                raise errors.PluginError(str(err))
 
     def _log_save_errors(self, ex_errs):
         """Log errors due to bad Augeas save.
@@ -175,10 +161,7 @@ class AugeasConfigurator(common.Plugin):
         :raises .errors.PluginError: If unable to recover the configuration
 
         """
-        try:
+        super(AugeasConfigurator, self).recovery_routine()
-            self.reverter.recovery_routine()
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))
         # Need to reload configuration after these changes take effect
         self.aug.load()
 
@@ -188,10 +171,7 @@ class AugeasConfigurator(common.Plugin):
         :raises .errors.PluginError: If unable to revert the challenge config.
 
         """
-        try:
+        self.revert_temporary_config()
-            self.reverter.revert_temporary_config()
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))
         self.aug.load()
 
     def rollback_checkpoints(self, rollback=1):
@@ -203,20 +183,5 @@ class AugeasConfigurator(common.Plugin):
             the function is unable to correctly revert the configuration
 
         """
-        try:
+        super(AugeasConfigurator, self).rollback_checkpoints(rollback)
-            self.reverter.rollback_checkpoints(rollback)
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))
         self.aug.load()
-
-    def view_config_changes(self):
-        """Show all of the configuration changes that have taken place.
-
-        :raises .errors.PluginError: If there is a problem while processing
-            the checkpoints directories.
-
-        """
-        try:
-            self.reverter.view_config_changes()
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))

certbot-nginx/certbot_nginx/configurator.py

@@ -19,7 +19,6 @@ from certbot import crypto_util
 from certbot import errors
 from certbot import interfaces
 from certbot import util
-from certbot import reverter
 
 from certbot.plugins import common
 
@@ -63,7 +62,7 @@ TEST_REDIRECT_COMMENT_BLOCK = [
 
 @zope.interface.implementer(interfaces.IAuthenticator, interfaces.IInstaller)
 @zope.interface.provider(interfaces.IPluginFactory)
-class NginxConfigurator(common.Plugin):
+class NginxConfigurator(common.Installer):
     # pylint: disable=too-many-instance-attributes,too-many-public-methods
     """Nginx configurator.
 
@@ -127,8 +126,6 @@ class NginxConfigurator(common.Plugin):
         self._enhance_func = {"redirect": self._enable_redirect,
                               "staple-ocsp": self._enable_ocsp_stapling}
 
-        # Set up reverter
-        self.reverter = reverter.Reverter(self.config)
         self.reverter.recovery_routine()
 
     @property
@@ -697,31 +694,13 @@ class NginxConfigurator(common.Plugin):
 
         """
         save_files = set(self.parser.parsed.keys())
-
+        self.add_to_checkpoint(save_files, self.save_notes, temporary)
-        try:  # TODO: make a common base for Apache and Nginx plugins
-            # Create Checkpoint
-            if temporary:
-                self.reverter.add_to_temp_checkpoint(
-                    save_files, self.save_notes)
-                # how many comments does it take
-            else:
-                self.reverter.add_to_checkpoint(save_files,
-                                            self.save_notes)
-                # to confuse a linter?
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))
-
         self.save_notes = ""
 
         # Change 'ext' to something else to not override existing conf files
         self.parser.filedump(ext='')
         if title and not temporary:
-            try:
+            self.finalize_checkpoint(title)
-                self.reverter.finalize_checkpoint(title)
-            except errors.ReverterError as err:
-                raise errors.PluginError(str(err))
-
-        return True
 
     def recovery_routine(self):
         """Revert all previously modified files.
@@ -731,10 +710,7 @@ class NginxConfigurator(common.Plugin):
         :raises .errors.PluginError: If unable to recover the configuration
 
         """
-        try:
+        super(NginxConfigurator, self).recovery_routine()
-            self.reverter.recovery_routine()
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))
         self.parser.load()
 
     def revert_challenge_config(self):
@@ -743,10 +719,7 @@ class NginxConfigurator(common.Plugin):
         :raises .errors.PluginError: If unable to revert the challenge config.
 
         """
-        try:
+        self.revert_temporary_config()
-            self.reverter.revert_temporary_config()
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))
         self.parser.load()
 
     def rollback_checkpoints(self, rollback=1):
@@ -758,24 +731,9 @@ class NginxConfigurator(common.Plugin):
             the function is unable to correctly revert the configuration
 
         """
-        try:
+        super(NginxConfigurator, self).rollback_checkpoints(rollback)
-            self.reverter.rollback_checkpoints(rollback)
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))
         self.parser.load()
 
-    def view_config_changes(self):
-        """Show all of the configuration changes that have taken place.
-
-        :raises .errors.PluginError: If there is a problem while processing
-            the checkpoints directories.
-
-        """
-        try:
-            self.reverter.view_config_changes()
-        except errors.ReverterError as err:
-            raise errors.PluginError(str(err))
-
     ###########################################################################
     # Challenges Section for IAuthenticator
     ###########################################################################

certbot/plugins/common.py

@@ -13,7 +13,9 @@ from acme.jose import util as jose_util
 
 from certbot import constants
 from certbot import crypto_util
+from certbot import errors
 from certbot import interfaces
+from certbot import reverter
 from certbot import util
 
 logger = logging.getLogger(__name__)
@@ -100,6 +102,102 @@ class Plugin(object):
 # other
 
 
+class Installer(Plugin):
+    """An installer base class with reverter methods defined.
+
+    Installer plugins do not have to inherit from this class.
+
+    """
+    def __init__(self, *args, **kwargs):
+        super(Installer, self).__init__(*args, **kwargs)
+        self.reverter = reverter.Reverter(self.config)
+
+    def add_to_checkpoint(self, save_files, save_notes, temporary=False):
+        """Add files to a checkpoint.
+
+        :param set save_files: set of filepaths to save
+        :param str save_notes: notes about changes during the save
+        :param bool temporary: True if the files should be added to a
+            temporary checkpoint rather than a permanent one. This is
+            usually used for changes that will soon be reverted.
+
+        :raises .errors.PluginError: when unable to add to checkpoint
+
+        """
+        if temporary:
+            checkpoint_func = self.reverter.add_to_temp_checkpoint
+        else:
+            checkpoint_func = self.reverter.add_to_checkpoint
+
+        try:
+            checkpoint_func(save_files, save_notes)
+        except errors.ReverterError as err:
+            raise errors.PluginError(str(err))
+
+    def finalize_checkpoint(self, title):
+        """Timestamp and save changes made through the reverter.
+
+        :param str title: Title describing checkpoint
+
+        :raises .errors.PluginError: when an error occurs
+
+        """
+        try:
+            self.reverter.finalize_checkpoint(title)
+        except errors.ReverterError as err:
+            raise errors.PluginError(str(err))
+
+    def recovery_routine(self):
+        """Revert all previously modified files.
+
+        Reverts all modified files that have not been saved as a checkpoint
+
+        :raises .errors.PluginError: If unable to recover the configuration
+
+        """
+        try:
+            self.reverter.recovery_routine()
+        except errors.ReverterError as err:
+            raise errors.PluginError(str(err))
+
+    def revert_temporary_config(self):
+        """Rollback temporary checkpoint.
+
+        :raises .errors.PluginError: when unable to revert config
+
+        """
+        try:
+            self.reverter.revert_temporary_config()
+        except errors.ReverterError as err:
+            raise errors.PluginError(str(err))
+
+    def rollback_checkpoints(self, rollback=1):
+        """Rollback saved checkpoints.
+
+        :param int rollback: Number of checkpoints to revert
+
+        :raises .errors.PluginError: If there is a problem with the input or
+            the function is unable to correctly revert the configuration
+
+        """
+        try:
+            self.reverter.rollback_checkpoints(rollback)
+        except errors.ReverterError as err:
+            raise errors.PluginError(str(err))
+
+    def view_config_changes(self):
+        """Show all of the configuration changes that have taken place.
+
+        :raises .errors.PluginError: If there is a problem while processing
+            the checkpoints directories.
+
+        """
+        try:
+            self.reverter.view_config_changes()
+        except errors.ReverterError as err:
+            raise errors.PluginError(str(err))
+
+
 class Addr(object):
     r"""Represents an virtual host address.
 

certbot/plugins/common_test.py

@@ -1,4 +1,5 @@
 """Tests for certbot.plugins.common."""
+import functools
 import os
 import shutil
 import tempfile
@@ -12,6 +13,7 @@ from acme import jose
 
 from certbot import achallenges
 from certbot import crypto_util
+from certbot import errors
 
 from certbot.tests import acme_util
 from certbot.tests import util as test_util
@@ -77,6 +79,91 @@ class PluginTest(unittest.TestCase):
             "--mock-foo-bar", dest="different_to_foo_bar", x=1, y=None)
 
 
+class InstallerTest(unittest.TestCase):
+    """Tests for certbot.plugins.common.Installer."""
+
+    def setUp(self):
+        from certbot.plugins.common import Installer
+
+        with mock.patch("certbot.plugins.common.reverter.Reverter"):
+            self.installer = Installer(config=mock.MagicMock(),
+                                       name="Installer")
+        self.reverter = self.installer.reverter
+
+    def test_add_to_real_checkpoint(self):
+        files = set(("foo.bar", "baz.qux",))
+        save_notes = "foo bar baz qux"
+        self._test_wrapped_method("add_to_checkpoint", files, save_notes)
+
+    def test_add_to_real_checkpoint2(self):
+        self._test_add_to_checkpoint_common(False)
+
+    def test_add_to_temporary_checkpoint(self):
+        self._test_add_to_checkpoint_common(True)
+
+    def _test_add_to_checkpoint_common(self, temporary):
+        files = set(("foo.bar", "baz.qux",))
+        save_notes = "foo bar baz qux"
+
+        installer_func = functools.partial(self.installer.add_to_checkpoint,
+                                           temporary=temporary)
+
+        if temporary:
+            reverter_func = self.reverter.add_to_temp_checkpoint
+        else:
+            reverter_func = self.reverter.add_to_checkpoint
+
+        self._test_adapted_method(
+            installer_func, reverter_func, files, save_notes)
+
+    def test_finalize_checkpoint(self):
+        self._test_wrapped_method("finalize_checkpoint", "foo")
+
+    def test_recovery_routine(self):
+        self._test_wrapped_method("recovery_routine")
+
+    def test_revert_temporary_config(self):
+        self._test_wrapped_method("revert_temporary_config")
+
+    def test_rollback_checkpoints(self):
+        self._test_wrapped_method("rollback_checkpoints", 42)
+
+    def test_view_config_changes(self):
+        self._test_wrapped_method("view_config_changes")
+
+    def _test_wrapped_method(self, name, *args, **kwargs):
+        """Test a wrapped reverter method.
+
+        :param str name: name of the method to test
+        :param tuple args: position arguments to method
+        :param dict kwargs: keyword arguments to method
+
+        """
+        installer_func = getattr(self.installer, name)
+        reverter_func = getattr(self.reverter, name)
+        self._test_adapted_method(
+            installer_func, reverter_func, *args, **kwargs)
+
+    def _test_adapted_method(self, installer_func,
+                             reverter_func, *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 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)
+
+
 class AddrTest(unittest.TestCase):
     """Tests for certbot.client.plugins.common.Addr."""