launchpad-reviewers team mailing list archive

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

Jürgen Gmach has proposed merging ~jugmac00/lpcraft:add-more-hooks into lpcraft:main.

Commit message:
Add more hooks

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

This MP is WIP.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~jugmac00/lpcraft:add-more-hooks into lpcraft:main.

diff --git a/docs/plugins.rst b/docs/plugins.rst
index 4c644d6..ca26449 100644
--- a/docs/plugins.rst
+++ b/docs/plugins.rst
@@ -20,3 +20,8 @@ Plugin discovery
 
 As currently only builtin plugins are supported,
 you need to define a plugin in ``lpcraft/plugins/<plugin>``.
+
+
+.. comments
+
+    XXX jugmac00 2021-12-17: render all available hooks via plugin
diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py
index 3c53eef..6985d7c 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -168,12 +168,30 @@ def _run_job(
     host_architecture = get_host_architecture()
     if host_architecture not in job.architectures:
         return
-    if job.run is None:
+    pm = get_plugin_manager(job)
+    run_from_plugin = pm.hook.lpcraft_execute_run()
+    run_from_configuration = job.run
+    if run_from_configuration is not None:
+        run_commands = run_from_plugin + [run_from_configuration]
+    else:
+        run_commands = run_from_plugin
+
+    if len(run_commands) == 0:
         raise CommandError(
             f"Job {job_name!r} for {job.series}/{host_architecture} "
             f"does not set 'run'"
         )
 
+    if len(run_commands) > 1:
+        raise CommandError(
+            f"Job {job_name!r} for {job.series}/{host_architecture} "
+            f"sets more than one 'run' command. "
+            f"Maybe you have set a run command both in the configuration "
+            f"and in a plugin?"
+        )
+
+    run_command = run_commands[0]
+
     cwd = Path.cwd()
     remote_cwd = env.get_managed_environment_project_path()
 
@@ -186,16 +204,15 @@ def _run_job(
         series=job.series,
         architecture=host_architecture,
     ) as instance:
-        if job.snaps:
-            for snap in job.snaps:
-                emit.progress(f"Running `snap install {snap}`")
-                install_from_store(
-                    executor=instance,
-                    snap_name=snap,
-                    channel="stable",
-                    classic=True,
-                )
-        pm = get_plugin_manager(job)
+        snaps = list(itertools.chain(*pm.hook.lpcraft_install_snaps()))
+        for snap in snaps:
+            emit.progress(f"Running `snap install {snap}`")
+            install_from_store(
+                executor=instance,
+                snap_name=snap,
+                channel="stable",
+                classic=True,
+            )
         packages = list(itertools.chain(*pm.hook.lpcraft_install_packages()))
         if packages:
             packages_cmd = ["apt", "install", "-y"] + packages
@@ -208,11 +225,11 @@ def _run_job(
                     stdout=stream,
                     stderr=stream,
                 )
-        run_cmd = ["bash", "--noprofile", "--norc", "-ec", job.run]
+        full_run_cmd = ["bash", "--noprofile", "--norc", "-ec", run_command]
         emit.progress("Running the job")
-        with emit.open_stream(f"Running {run_cmd}") as stream:
+        with emit.open_stream(f"Running {full_run_cmd}") as stream:
             proc = instance.execute_run(
-                run_cmd,
+                full_run_cmd,
                 cwd=remote_cwd,
                 env=job.environment,
                 stdout=stream,
diff --git a/lpcraft/plugin/hookspecs.py b/lpcraft/plugin/hookspecs.py
index f16f909..2219e31 100644
--- a/lpcraft/plugin/hookspecs.py
+++ b/lpcraft/plugin/hookspecs.py
@@ -11,3 +11,13 @@ hookspec = pluggy.HookspecMarker("lpcraft")
 @hookspec  # type: ignore
 def lpcraft_install_packages() -> list[str]:
     """system packages to be installed"""
+
+
+@hookspec  # type: ignore
+def lpcraft_install_snaps() -> list[str]:
+    """snaps to be installed"""
+
+
+@hookspec  # type: ignore
+def lpcraft_execute_run() -> str:
+    """command to be executed"""
diff --git a/lpcraft/plugin/lib.py b/lpcraft/plugin/lib.py
index b22be5a..ced3cdd 100644
--- a/lpcraft/plugin/lib.py
+++ b/lpcraft/plugin/lib.py
@@ -23,3 +23,9 @@ class InternalPlugins:
         if self.config.packages:
             return self.config.packages
         return []
+
+    @hookimpl  # type: ignore
+    def lpcraft_install_snaps(self) -> list[str]:
+        if self.config.snaps:
+            return self.config.snaps
+        return []
diff --git a/lpcraft/plugin/tests/test_builtin_plugins.py b/lpcraft/plugin/tests/test_plugins.py
similarity index 82%
rename from lpcraft/plugin/tests/test_builtin_plugins.py
rename to lpcraft/plugin/tests/test_plugins.py
index 79b0a1d..d72a50c 100644
--- a/lpcraft/plugin/tests/test_builtin_plugins.py
+++ b/lpcraft/plugin/tests/test_plugins.py
@@ -9,12 +9,12 @@ from unittest.mock import ANY, Mock, call, patch
 from craft_providers.lxd import LXC, launch
 
 from lpcraft.commands.tests import CommandBaseTestCase
-from lpcraft.errors import YAMLError
+from lpcraft.errors import CommandError, YAMLError
 from lpcraft.providers._lxd import LXDProvider, _LXDLauncher
 from lpcraft.providers.tests import FakeLXDInstaller
 
 
-class TestBuiltinPlugins(CommandBaseTestCase):
+class TestPlugins(CommandBaseTestCase):
     def makeLXDProvider(
         self,
         is_ready=True,
@@ -39,9 +39,6 @@ class TestBuiltinPlugins(CommandBaseTestCase):
         mock_get_provider.return_value = provider
         execute_run = launcher.return_value.execute_run
         execute_run.return_value = subprocess.CompletedProcess([], 0)
-        # XXX jugmac00 2021-12-16: the current implementation of the `tox`
-        # plugin does not provide a run command, so we need to specify it
-        # here in the configuration file
         config = dedent(
             """
             pipeline:
@@ -52,7 +49,6 @@ class TestBuiltinPlugins(CommandBaseTestCase):
                     series: focal
                     architectures: amd64
                     packages: [nginx, apache2]
-                    run: tox
                     plugin: tox
             """
         )
@@ -110,3 +106,45 @@ class TestBuiltinPlugins(CommandBaseTestCase):
 
         self.assertEqual(1, result.exit_code)
         self.assertEqual([YAMLError("Unknown plugin")], result.errors)
+
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_defining_two_run_commands_errors(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+        # defining a run command both in the configuration file and
+        # in a plugin results in an error
+        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
+                    packages: [nginx, apache2]
+                    run: tox
+                    plugin: tox
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        result = self.run_command("run")
+
+        self.assertEqual(1, result.exit_code)
+        self.assertEqual(
+            result.errors,
+            [
+                CommandError(
+                    "Job 'test' for focal/amd64 sets more than one 'run' "
+                    "command. Maybe you have set a run command both in the "
+                    "configuration and in a plugin?"
+                )
+            ],
+        )
diff --git a/lpcraft/plugins/tox.py b/lpcraft/plugins/tox.py
index 2952371..03b3b9a 100644
--- a/lpcraft/plugins/tox.py
+++ b/lpcraft/plugins/tox.py
@@ -15,3 +15,7 @@ class ToxPlugin:
     @hookimpl  # type: ignore
     def lpcraft_install_packages(self) -> list[str]:
         return ["tox"]
+
+    @hookimpl  # type: ignore
+    def lpcraft_execute_run(self) -> str:
+        return "tox"