launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+413280@xxxxxxxxxxxxxxxxxx>]

Jürgen Gmach has proposed merging ~jugmac00/lpcraft:use-pluggy-for-plugins into lpcraft:main.

Commit message:
Introduce pluggy to manage internal and external plugins

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jugmac00/lpcraft/+git/lpcraft/+merge/413280

- pluggy has not been type annotated yet
- the documentation will be extended once all hooks are defined/created; e.g. tox uses a sphinx plugin to auto-document the hooks (this will be a new MP)

The internal hook `lpcraft_install_packages` needs access to the job configuration, the external plugins do not.

This "mismatch" could be solved by:
- creating two different hooks, e.g. `_lpcraft_install_packages` which takes a job configuration as argument, and `lpcraft_install_packages` which does not, and then combining the result; that is what `tox` does
- using a class as namespace, which I did, so the internal hook can access the job configuration via `self.config`
- just pass the job configuration to all hooks, and ignore the information where it is not needed; that is what pytest does a lot; I did not use this approach as the signature/interface of the hook would be more complex than needed
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/lpcraft:use-pluggy-for-plugins into lpcraft:main.

diff --git a/.mypy.ini b/.mypy.ini
index 3b71bdb..446a374 100644
--- a/.mypy.ini
+++ b/.mypy.ini
@@ -7,7 +7,7 @@ disallow_subclassing_any = false
 disallow_untyped_calls = false
 disallow_untyped_defs = false
 
-[mypy-fixtures.*,systemfixtures.*,testtools.*]
+[mypy-fixtures.*,systemfixtures.*,testtools.*,pluggy.*]
 ignore_missing_imports = true
 
 [mypy-craft_cli.*]
diff --git a/docs/index.rst b/docs/index.rst
index 8d171d8..c37c2fc 100644
--- a/docs/index.rst
+++ b/docs/index.rst
@@ -46,4 +46,5 @@ Example configuration
 
     self
     configuration
+    plugins
     CONTRIBUTING
diff --git a/docs/plugins.rst b/docs/plugins.rst
new file mode 100644
index 0000000..2669cc3
--- /dev/null
+++ b/docs/plugins.rst
@@ -0,0 +1,37 @@
+Extending lpcraft
+=================
+
+lpcraft uses `pluggy <https://pluggy.readthedocs.io/>`_ to customize the
+default behaviour. For example the following code snippet would extend the
+packages which need to be installed into an environment by additional
+requirements:
+
+.. code-block:: python
+
+    from lpcraft.plugin import hookimpl
+
+
+    @hookimpl
+    def lpcraft_install_packages():
+        return ["tox"]
+
+You can define such hooks in a package alongside lpcraft.
+
+
+Discovery
+---------
+
+lpcraft uses `setuptools' <https://setuptools.pypa.io/en/latest/index.html>`_
+`entry_points <https://setuptools.pypa.io/en/latest/userguide/entry_point.html>`_
+for plugin discovery.
+
+You need to add a configuration similar to the following to your `setup.py`:
+
+.. code-block:: python
+
+    setup(
+        ...
+        install_requires="lpcraft",
+        entry_points={"lpcraft": ["entry_name = entry_point"]},
+        ...
+    )
diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py
index 685ab70..07f9a1c 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -3,6 +3,7 @@
 
 import fnmatch
 import io
+import itertools
 import json
 import os
 from argparse import Namespace
@@ -17,6 +18,7 @@ from dotenv import dotenv_values
 from lpcraft import env
 from lpcraft.config import Config, Output
 from lpcraft.errors import CommandError
+from lpcraft.plugin.manager import get_plugin_manager
 from lpcraft.providers import get_provider
 from lpcraft.utils import get_host_architecture
 
@@ -201,8 +203,12 @@ def run(args: Namespace) -> int:
                             channel="stable",
                             classic=True,
                         )
-                if job.packages:
-                    packages_cmd = ["apt", "install", "-y"] + job.packages
+                pm = get_plugin_manager(job)
+                packages = list(
+                    itertools.chain(*pm.hook.lpcraft_install_packages())
+                )
+                if packages:
+                    packages_cmd = ["apt", "install", "-y"] + packages
                     emit.progress("Installing system packages")
                     with emit.open_stream(f"Running {packages_cmd}") as stream:
                         proc = instance.execute_run(
diff --git a/lpcraft/plugin/__init__.py b/lpcraft/plugin/__init__.py
new file mode 100644
index 0000000..b2e74c1
--- /dev/null
+++ b/lpcraft/plugin/__init__.py
@@ -0,0 +1,3 @@
+import pluggy
+
+hookimpl = pluggy.HookimplMarker("lpcraft")
diff --git a/lpcraft/plugin/hookspecs.py b/lpcraft/plugin/hookspecs.py
new file mode 100644
index 0000000..7e9d4c8
--- /dev/null
+++ b/lpcraft/plugin/hookspecs.py
@@ -0,0 +1,10 @@
+from __future__ import annotations
+
+import pluggy
+
+hookspec = pluggy.HookspecMarker("lpcraft")
+
+
+@hookspec  # type: ignore
+def lpcraft_install_packages() -> list[str]:
+    """system packages to be installed"""
diff --git a/lpcraft/plugin/lib.py b/lpcraft/plugin/lib.py
new file mode 100644
index 0000000..3bd1931
--- /dev/null
+++ b/lpcraft/plugin/lib.py
@@ -0,0 +1,15 @@
+from __future__ import annotations
+
+from lpcraft.config import Job
+from lpcraft.plugin import hookimpl
+
+
+class HookImplementations:
+    def __init__(self, config: Job) -> None:
+        self.config = config
+
+    @hookimpl  # type: ignore
+    def lpcraft_install_packages(self) -> list[str]:
+        if self.config.packages:
+            return self.config.packages
+        return []
diff --git a/lpcraft/plugin/manager.py b/lpcraft/plugin/manager.py
new file mode 100644
index 0000000..8a1213c
--- /dev/null
+++ b/lpcraft/plugin/manager.py
@@ -0,0 +1,15 @@
+import pluggy
+
+from lpcraft.config import Job
+from lpcraft.plugin import hookspecs
+from lpcraft.plugin.lib import HookImplementations
+
+
+def get_plugin_manager(job: Job) -> pluggy.PluginManager:
+    pm = pluggy.PluginManager("lpcraft")
+    pm.add_hookspecs(hookspecs)
+    # load internal plugins
+    pm.register(HookImplementations(job))
+    # load external plugins
+    pm.load_setuptools_entrypoints("lpcraft")
+    return pm
diff --git a/lpcraft/plugin/testing/lp_tox/lp_tox.py b/lpcraft/plugin/testing/lp_tox/lp_tox.py
new file mode 100644
index 0000000..f5613b6
--- /dev/null
+++ b/lpcraft/plugin/testing/lp_tox/lp_tox.py
@@ -0,0 +1,8 @@
+from __future__ import annotations
+
+from lpcraft.plugin import hookimpl
+
+
+@hookimpl  # type: ignore
+def lpcraft_install_packages() -> list[str]:
+    return ["tox"]
diff --git a/lpcraft/plugin/tests/__init__.py b/lpcraft/plugin/tests/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/lpcraft/plugin/tests/__init__.py
diff --git a/lpcraft/plugin/tests/test_external_plugin.py b/lpcraft/plugin/tests/test_external_plugin.py
new file mode 100644
index 0000000..aa7c521
--- /dev/null
+++ b/lpcraft/plugin/tests/test_external_plugin.py
@@ -0,0 +1,85 @@
+import subprocess
+import sys
+from pathlib import Path, PosixPath
+from textwrap import dedent
+from unittest.mock import ANY, Mock, call, patch
+
+from craft_providers.lxd import LXC, launch
+
+from lpcraft.commands.tests import CommandBaseTestCase
+from lpcraft.providers._lxd import LXDProvider, _LXDLauncher
+from lpcraft.providers.tests import FakeLXDInstaller
+
+
+class TestLoadExternalPlugin(CommandBaseTestCase):
+    def setUp(self) -> None:
+        super().setUp()
+        sys.path.append(
+            str(Path(__file__).parent.parent / "testing" / "lp_tox")
+        )
+
+    def tearDown(self) -> None:
+        super().tearDown()
+        sys.path.pop()
+
+    def makeLXDProvider(
+        self,
+        is_ready=True,
+        lxd_launcher=_LXDLauncher,
+    ):
+        lxc = Mock(spec=LXC)
+        lxc.remote_list.return_value = {}
+        lxd_installer = FakeLXDInstaller(is_ready=is_ready)
+        return LXDProvider(
+            lxc=lxc,
+            lxd_installer=lxd_installer,
+            lxd_launcher=lxd_launcher,
+        )
+
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_load_external_plugin(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+        launcher = Mock(spec=launch)
+        provider = self.makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        execute_run = launcher.return_value.execute_run
+        execute_run.return_value = subprocess.CompletedProcess([], 0)
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: tox
+                    packages: [nginx, apache2]
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        self.run_command("run")
+
+        # the requirement to install `tox` is added by the external plugin
+        self.assertEqual(
+            [
+                call(
+                    ["apt", "install", "-y", "tox", "nginx", "apache2"],
+                    cwd=PosixPath("/root/project"),
+                    env=None,
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+                call(
+                    ["bash", "--noprofile", "--norc", "-ec", "tox"],
+                    cwd=PosixPath("/root/project"),
+                    env=None,
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+            ],
+            execute_run.call_args_list,
+        )
diff --git a/setup.cfg b/setup.cfg
index 7f9e2d8..02c6816 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -25,6 +25,7 @@ install_requires =
     PyYAML
     craft-cli
     craft-providers
+    pluggy
     pydantic
     python-dotenv
 python_requires = >=3.8