From cb441606d52fe3fd67fe8b3f2fd1672cd645ba49 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 23 Jun 2016 16:14:34 -0700 Subject: [PATCH 1/5] Explictly state assumptions made by certbot --- certbot/interfaces.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/certbot/interfaces.py b/certbot/interfaces.py index 37835462e..a6f0b9735 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -180,6 +180,9 @@ class IAuthenticator(IPlugin): def cleanup(achalls): """Revert changes and shutdown after challenges complete. + This method should be able to revert all changes made by + perform, even if perform exited abnormally. + :param list achalls: Non-empty (guaranteed) list of :class:`~certbot.achallenges.AnnotatedChallenge` instances, a subset of those previously passed to :func:`perform`. @@ -304,8 +307,11 @@ class IInstaller(IPlugin): Both title and temporary are needed because a save may be intended to be permanent, but the save is not ready to be a full - checkpoint. If an exception is raised, it is assumed a new - checkpoint was not created. + checkpoint. + + It is assumed that at most one checkpoint is finalized by this + method. Additionally, if an exception is raised, it is assumed a + new checkpoint was not finalized. :param str title: The title of the save. If a title is given, the configuration will be saved as a new checkpoint and put in a From 38feb37d3ee0f9b0db689b0db2a2a9df1facd207 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 23 Jun 2016 17:03:01 -0700 Subject: [PATCH 2/5] Further document reverter --- certbot/reverter.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/certbot/reverter.py b/certbot/reverter.py index f8140d60d..c7eab1690 100644 --- a/certbot/reverter.py +++ b/certbot/reverter.py @@ -24,6 +24,39 @@ logger = logging.getLogger(__name__) class Reverter(object): """Reverter Class - save and revert configuration checkpoints. + This class can be used by the plugins, especially installers, to + undo changes made to the user's system. Modifications to files and + commands to do undo actions taken by the plugin should be registered + with this class before the action is taken. + + Once a change has been registered with this class, there are three + states the change can be in. First, the change can be a temporary + change. This should be used for changes that will soon be reverted, + such as config changes for the purpose of solving a challenge. + Changes are added to this state through calls to + :func:`~add_to_temp_checkpoint` and reverted when + :func:`~revert_temporary_config` or :func:`~recovery_routine` is + called. + + The second state a change can be in is in progress. These changes + are not temporary, however, they also have not been finalized in a + checkpoint. A change must become in progress before it can be + finalized. Changes are added to this state through calls to + :func:`~add_to_checkpoint` and reverted when + :func:`~recovery_routine` is called. + + The last state a change can be in is finalized in a checkpoint. A + change is put into this state by first becoming an in progress + change and then calling :func:`~finalize_checkpoint`. Changes + in this state can be reverted through calls to + :func:`~rollback_checkpoints`. + + As a final note, creating new files and registering undo commands + are handled specially and use the methods + :func:`~register_file_creation` and :func:`~register_undo_command` + respectively. Both of these methods can be used to create either + temporary or in progress changes. + .. note:: Consider moving everything over to CSV format. :param config: Configuration. From 7e5e016982a0852f4151c434890102d733eedeba Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 23 Jun 2016 17:10:03 -0700 Subject: [PATCH 3/5] Document that only save should make checkpoints --- certbot/interfaces.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/certbot/interfaces.py b/certbot/interfaces.py index a6f0b9735..2bc25723c 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -241,6 +241,11 @@ class IInstaller(IPlugin): Represents any server that an X509 certificate can be placed. + It is assumed that :func:`save` is the only method that finalizes a + checkpoint. This is important to ensure that checkpoints are + restored in a consistent manner if requested by the user or in case + of an error. + """ def get_all_names(): From 7aa6becc5bffa19e85108b0286b08aa29aaf50aa Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 23 Jun 2016 17:16:58 -0700 Subject: [PATCH 4/5] Suggest reverter for installers --- certbot/interfaces.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/certbot/interfaces.py b/certbot/interfaces.py index 2bc25723c..e4e62e0a2 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -246,6 +246,9 @@ class IInstaller(IPlugin): restored in a consistent manner if requested by the user or in case of an error. + Using :class:`certbot.reverter.Reverter` to implement checkpoints, + rollback, and recovery can dramatically simplify plugin development. + """ def get_all_names(): From 02c61cffb713ef3609b2dffd8e2465a95cd4fcfe Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 23 Jun 2016 17:17:17 -0700 Subject: [PATCH 5/5] Consistent capitialization with installer --- certbot/reverter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/reverter.py b/certbot/reverter.py index c7eab1690..6dde05077 100644 --- a/certbot/reverter.py +++ b/certbot/reverter.py @@ -24,7 +24,7 @@ logger = logging.getLogger(__name__) class Reverter(object): """Reverter Class - save and revert configuration checkpoints. - This class can be used by the plugins, especially installers, to + This class can be used by the plugins, especially Installers, to undo changes made to the user's system. Modifications to files and commands to do undo actions taken by the plugin should be registered with this class before the action is taken.