manifestv2, appresources: Implement a more generic 'apply' mecanism with possible rollback

src/app.py

@@ -546,6 +546,7 @@ def app_upgrade(app=[], url=None, file=None, force=False, no_safety_backup=False
 
         if manifest["packaging_format"] >= 2:
             if no_safety_backup:
+                # FIXME: i18n
                 logger.warning("Skipping the creation of a backup prior to the upgrade.")
             else:
                 # FIXME: i18n
@@ -570,8 +571,17 @@ def app_upgrade(app=[], url=None, file=None, force=False, no_safety_backup=False
 
         _assert_system_is_sane_for_app(manifest, "pre")
 
+        # We'll check that the app didn't brutally edit some system configuration
+        manually_modified_files_before_install = manually_modified_files()
+
         app_setting_path = os.path.join(APPS_SETTING_PATH, app_instance_name)
 
+        # Attempt to patch legacy helpers ...
+        _patch_legacy_helpers(extracted_app_folder)
+
+        # Apply dirty patch to make php5 apps compatible with php7
+        _patch_legacy_php_versions(extracted_app_folder)
+
         # Prepare env. var. to pass to script
         env_dict = _make_environment_for_app_script(
             app_instance_name, workdir=extracted_app_folder, action="upgrade"
@@ -582,20 +592,19 @@ def app_upgrade(app=[], url=None, file=None, force=False, no_safety_backup=False
         if manifest["packaging_format"] < 2:
             env_dict["NO_BACKUP_UPGRADE"] = "1" if no_safety_backup else "0"
 
-        # We'll check that the app didn't brutally edit some system configuration
-        manually_modified_files_before_install = manually_modified_files()
-
-        # Attempt to patch legacy helpers ...
-        _patch_legacy_helpers(extracted_app_folder)
-
-        # Apply dirty patch to make php5 apps compatible with php7
-        _patch_legacy_php_versions(extracted_app_folder)
-
         # Start register change on system
         related_to = [("app", app_instance_name)]
         operation_logger = OperationLogger("app_upgrade", related_to, env=env_dict)
         operation_logger.start()
 
+        if manifest["packaging_format"] >= 2:
+            from yunohost.utils.resources import AppResourceManager
+            try:
+                AppResourceManager(app_instance_name, wanted=manifest, current=app_dict["manifest"]).apply(rollback_if_failure=True)
+            except Exception:
+                # FIXME : improve error handling ....
+                raise
+
         # Execute the app upgrade script
         upgrade_failed = True
         try:
@@ -880,10 +889,9 @@ def app_install(
     if packaging_format >= 2:
         from yunohost.utils.resources import AppResourceManager
         try:
-            AppResourceManager(app_instance_name, wanted=manifest, current={}).apply()
+            AppResourceManager(app_instance_name, wanted=manifest, current={}).apply(rollback_if_failure=True)
         except Exception:
             # FIXME : improve error handling ....
-            AppResourceManager(app_instance_name, wanted={}, current=manifest).apply()
             raise
     else:
         # Initialize the main permission for the app
@@ -997,7 +1005,7 @@ def app_install(
             if packaging_format >= 2:
                 from yunohost.utils.resources import AppResourceManager
                 try:
-                    AppResourceManager(app_instance_name, wanted={}, current=manifest).apply()
+                    AppResourceManager(app_instance_name, wanted={}, current=manifest).apply(rollback_if_failure=False)
                 except Exception:
                     # FIXME : improve error handling ....
                     raise
@@ -1109,7 +1117,7 @@ def app_remove(operation_logger, app, purge=False):
     if packaging_format >= 2:
         try:
             from yunohost.utils.resources import AppResourceManager
-            AppResourceManager(app, wanted={}, current=manifest).apply()
+            AppResourceManager(app, wanted={}, current=manifest).apply(rollback_if_failure=False)
         except Exception:
             # FIXME : improve error handling ....
             raise

src/backup.py

@@ -49,6 +49,7 @@ from yunohost.app import (
     _is_installed,
     _make_environment_for_app_script,
     _make_tmp_workdir_for_app,
+    _get_manifest_of_app,
 )
 from yunohost.hook import (
     hook_list,
@@ -1512,6 +1513,15 @@ class RestoreManager:
         operation_logger.extra["env"] = env_dict
         operation_logger.flush()
 
+        manifest = _get_manifest_of_app(app_dir_in_archive)
+        if manifest["packaging_format"] >= 2:
+            from yunohost.utils.resources import AppResourceManager
+            try:
+                AppResourceManager(app_instance_name, wanted=manifest, current={}).apply(rollback_if_failure=True)
+            except Exception:
+                # FIXME : improve error handling ....
+                raise
+
         # Execute the app install script
         restore_failed = True
         try:

src/tests/test_app_resources.py

@@ -0,0 +1,115 @@
+import os
+import pytest
+
+from yunohost.utils.resources import AppResource, AppResourceManager, AppResourceClassesByType
+
+dummyfile = "/tmp/dummyappresource-testapp"
+
+
+class DummyAppResource(AppResource):
+
+    type = "dummy"
+
+    default_properties = {
+        "file": "/tmp/dummyappresource-__APP__",
+        "content": "foo",
+    }
+
+    def provision_or_update(self, context):
+
+        open(self.file, "w").write(self.content)
+
+        if self.content == "forbiddenvalue":
+            raise Exception("Emeged you used the forbidden value!1!£&")
+
+    def deprovision(self, context):
+
+        os.system(f"rm -f {self.file}")
+
+
+AppResourceClassesByType["dummy"] = DummyAppResource
+
+
+def setup_function(function):
+
+    clean()
+
+
+def teardown_function(function):
+
+    clean()
+
+
+def clean():
+
+    os.system(f"rm -f {dummyfile}")
+
+
+def test_provision_dummy():
+
+    current = {"resources": {}}
+    wanted = {"resources": {"dummy": {}}}
+
+    assert not os.path.exists(dummyfile)
+    AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False)
+    assert open(dummyfile).read().strip() == "foo"
+
+
+def test_deprovision_dummy():
+
+    current = {"resources": {"dummy": {}}}
+    wanted = {"resources": {}}
+
+    open(dummyfile, "w").write("foo")
+
+    assert open(dummyfile).read().strip() == "foo"
+    AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False)
+    assert not os.path.exists(dummyfile)
+
+
+def test_provision_dummy_nondefaultvalue():
+
+    current = {"resources": {}}
+    wanted = {"resources": {"dummy": {"content": "bar"}}}
+
+    assert not os.path.exists(dummyfile)
+    AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False)
+    assert open(dummyfile).read().strip() == "bar"
+
+
+def test_update_dummy():
+
+    current = {"resources": {"dummy": {}}}
+    wanted = {"resources": {"dummy": {"content": "bar"}}}
+
+    open(dummyfile, "w").write("foo")
+
+    assert open(dummyfile).read().strip() == "foo"
+    AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False)
+    assert open(dummyfile).read().strip() == "bar"
+
+
+def test_update_dummy_fail():
+
+    current = {"resources": {"dummy": {}}}
+    wanted = {"resources": {"dummy": {"content": "forbiddenvalue"}}}
+
+    open(dummyfile, "w").write("foo")
+
+    assert open(dummyfile).read().strip() == "foo"
+    with pytest.raises(Exception):
+        AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=False)
+    assert open(dummyfile).read().strip() == "forbiddenvalue"
+
+
+def test_update_dummy_failwithrollback():
+
+    current = {"resources": {"dummy": {}}}
+    wanted = {"resources": {"dummy": {"content": "forbiddenvalue"}}}
+
+    open(dummyfile, "w").write("foo")
+
+    assert open(dummyfile).read().strip() == "foo"
+    with pytest.raises(Exception):
+        AppResourceManager("testapp", current=current, wanted=wanted).apply(rollback_if_failure=True)
+    assert open(dummyfile).read().strip() == "foo"

src/utils/resources.py

@@ -41,30 +41,77 @@ class AppResourceManager:
     def __init__(self, app: str, current: Dict, wanted: Dict):
 
         self.app = app
-        self.current = current.get("resources", {})
+        self.current = current
-        self.wanted = wanted.get("resources", {})
+        self.wanted = wanted
 
-        # c.f. the permission ressources where we need the app label >_>
+    def apply(self, rollback_if_failure, **context):
-        self.wanted_manifest = wanted
 
-    def apply(self, **context):
+        todos = list(self.compute_todos())
+        completed = []
+        rollback = False
+        exception = None
 
-        for name, infos in reversed(self.current.items()):
+        for todo, name, old, new in todos:
-            if name not in self.wanted.keys():
+            try:
-                resource = AppResourceClassesByType[name](infos, self.app, self)
+                if todo == "deprovision":
-                # FIXME : i18n, better info strings
+                    # FIXME : i18n, better info strings
-                logger.info(f"Deprovisionning {name} ...")
+                    logger.info(f"Deprovisionning {name} ...")
-                resource.deprovision(context=context)
+                    old.deprovision(context=context)
-
+                elif todo == "provision":
-        for name, infos in self.wanted.items():
+                    logger.info(f"Provisionning {name} ...")
-            resource = AppResourceClassesByType[name](infos, self.app, self)
+                    new.provision_or_update(context=context)
-            if name not in self.current.keys():
+                elif todo == "update":
-                # FIXME : i18n, better info strings
+                    logger.info(f"Updating {name} ...")
-                logger.info(f"Provisionning {name} ...")
+                    new.provision_or_update(context=context)
+            except Exception as e:
+                exception = e
+                # FIXME: better error handling ? display stacktrace ?
+                logger.warning(f"Failed to {todo} for {name} : {e}")
+                if rollback_if_failure:
+                    rollback = True
+                    completed.append((todo, name, old, new))
+                    break
+                else:
+                    pass
             else:
-                # FIXME : i18n, better info strings
+                completed.append((todo, name, old, new))
-                logger.info(f"Updating {name} ...")
+
-            resource.provision_or_update(context=context)
+        if rollback:
+            for todo, name, old, new in completed:
+                try:
+                    # (NB. here we want to undo the todo)
+                    if todo == "deprovision":
+                        # FIXME : i18n, better info strings
+                        logger.info(f"Reprovisionning {name} ...")
+                        old.provision_or_update(context=context)
+                    elif todo == "provision":
+                        logger.info(f"Deprovisionning {name} ...")
+                        new.deprovision(context=context)
+                    elif todo == "update":
+                        logger.info(f"Reverting {name} ...")
+                        old.provision_or_update(context=context)
+                except Exception as e:
+                    # FIXME: better error handling ? display stacktrace ?
+                    logger.error(f"Failed to rollback {name} : {e}")
+
+        if exception:
+            raise exception
+
+    def compute_todos(self):
+
+        for name, infos in reversed(self.current["resources"].items()):
+            if name not in self.wanted["resources"].keys():
+                resource = AppResourceClassesByType[name](infos, self.app, self)
+                yield ("deprovision", name, resource, None)
+
+        for name, infos in self.wanted["resources"].items():
+            wanted_resource = AppResourceClassesByType[name](infos, self.app, self)
+            if name not in self.current["resources"].keys():
+                yield ("provision", name, None, wanted_resource)
+            else:
+                infos_ = self.current["resources"][name]
+                current_resource = AppResourceClassesByType[name](infos_, self.app, self)
+                yield ("update", name, current_resource, wanted_resource)
 
 
 class AppResource:
@@ -179,7 +226,7 @@ class PermissionsResource(AppResource):
         )
 
         # Delete legacy is_public setting if not already done
-        self.delete_setting(f"is_public")
+        self.delete_setting("is_public")
 
         existing_perms = user_permission_list(short=True, apps=[self.app])["permissions"]
         for perm in existing_perms:
@@ -195,8 +242,8 @@ class PermissionsResource(AppResource):
                 permission_create(
                     f"{self.app}.{perm}",
                     allowed=init_allowed,
-                    # This is why the ugly hack with self.manager and wanted_manifest exists >_>
+                    # This is why the ugly hack with self.manager exists >_>
-                    label=self.manager.wanted_manifest["name"] if perm == "main" else perm,
+                    label=self.manager.wanted["name"] if perm == "main" else perm,
                     url=infos["url"],
                     additional_urls=infos["additional_urls"],
                     auth_header=infos["auth_header"],
@@ -499,7 +546,7 @@ class AptDependenciesAppResource(AppResource):
                          "ynh_remove_app_dependencies")
 
 
-class PortAppResource(AppResource):
+class PortResource(AppResource):
     """
         is_provisioned -> port setting exists and is not the port used by another app (ie not in another app setting)
         is_available   -> true
@@ -520,7 +567,7 @@ class PortAppResource(AppResource):
 
     default_properties = {
         "default": 1000,
-        "type": "internal",    # FIXME : implement logic for exposed port (allow/disallow in firewall ?)
+        "expose": False,    # FIXME : implement logic for exposed port (allow/disallow in firewall ?)
     }
 
     def _port_is_used(self, port):