Merge ~jugmac00/lpci:add-more-hooks into lpci:main

[Jürgen Gmach]

Add more hooks

-

[Colin Watson (cjwatson)]

Non-blocking, but I think this would be clearer and easier to unit-test if you pushed down the "combine plugin and configuration" logic for each of the various job properties to methods on `Job` (e.g. `Job.get_run`, or however you need to spell that to get it past pydantic), rather than open-coding it in `_run_job`.

review: Approve

[Jürgen Gmach (jugmac00)]

Thanks for the review.

I certainly noticed that inferring configuration options within the run method may not the best option.

Do you recall my MP review on https://code.launchpad.net/~cjwatson/lpcraft/+git/lpcraft/+merge/413081 where I mentioned that we need to build a configuration object?

`_run_job(job_name, job, provider, getattr(args, "output", None))`

I think this is related to your comment here.

I am not (yet) convinced that Job is the right place to put the logic to load additional data from the plugins. I especially do not like that after executing `Config.load(<config-file>)` we have a half finished config object, and then we must not forget to trigger some method(s) on Job to finish it.

Currently, `class Config` is more like a representation of a YAML file rather than a full blown configuration object.

Basically we have several sources of configuration options, which we need to combine.

We need some kind of configuration builder, which encapsulates all that logic:

def generate_configuration(...):
   a = load_yaml(...)
   b = load_config_from_plugins()
   c = load_config_from_cli()
   d = load_config_from_environment()
   # mix
   return configuration

And I totally agree - this would be way easier to unit test.

But I think that is something left for 2022 :-)

[Colin Watson (cjwatson)]

I'm not sure about every detail of this (e.g. to me `args.output` feels separate because it can't be set anywhere other than on the command line), but in broad terms I agree - we should certainly do some refactoring next year as time permits.

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..1805b05 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -168,12 +168,36 @@ 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)
+    # XXX jugmac00 2021-12-17: extract infering run_command
+    run_command = None
+
+    run_from_configuration = job.run
+    if run_from_configuration is not None:
+        run_command = run_from_configuration
+    else:
+        rv = pm.hook.lpcraft_execute_run()
+        run_command = rv and rv[0] or None
+
+    if not run_command:
         raise CommandError(
             f"Job {job_name!r} for {job.series}/{host_architecture} "
             f"does not set 'run'"
         )
 
+    # XXX jugmac00 2021-12-17: extract infering environment variables
+    rv = pm.hook.lpcraft_set_environment()
+    if rv:
+        # XXX jugmac00 2021-12-17: check for length or reduce?
+        env_from_plugin = rv[0]
+    else:
+        env_from_plugin = {}
+
+    env_from_configuration = job.environment
+    if env_from_configuration is not None:
+        env_from_plugin.update(env_from_configuration)
+    environment = env_from_plugin
+
     cwd = Path.cwd()
     remote_cwd = env.get_managed_environment_project_path()
 
@@ -186,16 +210,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
@@ -204,17 +227,17 @@ def _run_job(
                 proc = instance.execute_run(
                     packages_cmd,
                     cwd=remote_cwd,
-                    env=job.environment,
+                    env=environment,
                     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,
+                env=environment,
                 stdout=stream,
                 stderr=stream,
             )
diff --git a/lpcraft/commands/tests/test_run.py b/lpcraft/commands/tests/test_run.py
index e682273..e9a93ea 100644
--- a/lpcraft/commands/tests/test_run.py
+++ b/lpcraft/commands/tests/test_run.py
@@ -193,7 +193,7 @@ class TestRun(RunBaseTestCase):
                 call(
                     ["bash", "--noprofile", "--norc", "-ec", "tox"],
                     cwd=Path("/root/project"),
-                    env=None,
+                    env={},
                     stdout=ANY,
                     stderr=ANY,
                 )
@@ -281,7 +281,7 @@ class TestRun(RunBaseTestCase):
         execute_run.assert_called_once_with(
             ["bash", "--noprofile", "--norc", "-ec", "tox"],
             cwd=Path("/root/project"),
-            env=None,
+            env={},
             stdout=ANY,
             stderr=ANY,
         )
@@ -327,7 +327,7 @@ class TestRun(RunBaseTestCase):
                 call(
                     ["bash", "--noprofile", "--norc", "-ec", "tox"],
                     cwd=Path("/root/project"),
-                    env=None,
+                    env={},
                     stdout=ANY,
                     stderr=ANY,
                 ),
@@ -340,7 +340,7 @@ class TestRun(RunBaseTestCase):
                         "pyproject-build",
                     ],
                     cwd=Path("/root/project"),
-                    env=None,
+                    env={},
                     stdout=ANY,
                     stderr=ANY,
                 ),
@@ -392,14 +392,14 @@ class TestRun(RunBaseTestCase):
                 call(
                     ["bash", "--noprofile", "--norc", "-ec", "tox"],
                     cwd=Path("/root/project"),
-                    env=None,
+                    env={},
                     stdout=ANY,
                     stderr=ANY,
                 ),
                 call(
                     ["bash", "--noprofile", "--norc", "-ec", "tox"],
                     cwd=Path("/root/project"),
-                    env=None,
+                    env={},
                     stdout=ANY,
                     stderr=ANY,
                 ),
@@ -412,7 +412,7 @@ class TestRun(RunBaseTestCase):
                         "pyproject-build",
                     ],
                     cwd=Path("/root/project"),
-                    env=None,
+                    env={},
                     stdout=ANY,
                     stderr=ANY,
                 ),
@@ -956,9 +956,9 @@ class TestRun(RunBaseTestCase):
                 call(
                     ["bash", "--noprofile", "--norc", "-ec", "tox"],
                     cwd=Path("/root/project"),
-                    env=None,
-                    stdout=27,
-                    stderr=27,
+                    env={},
+                    stdout=ANY,
+                    stderr=ANY,
                 ),
             ],
             execute_run.call_args_list,
@@ -1003,14 +1003,14 @@ class TestRun(RunBaseTestCase):
                         "apache2",
                     ],
                     cwd=Path("/root/project"),
-                    env=None,
+                    env={},
                     stdout=ANY,
                     stderr=ANY,
                 ),
                 call(
                     ["bash", "--noprofile", "--norc", "-ec", "tox"],
                     cwd=Path("/root/project"),
-                    env=None,
+                    env={},
                     stdout=ANY,
                     stderr=ANY,
                 ),
@@ -1136,7 +1136,7 @@ class TestRunOne(RunBaseTestCase):
         execute_run.assert_called_once_with(
             ["bash", "--noprofile", "--norc", "-ec", "pyproject-build"],
             cwd=Path("/root/project"),
-            env=None,
+            env={},
             stdout=ANY,
             stderr=ANY,
         )
@@ -1184,7 +1184,7 @@ class TestRunOne(RunBaseTestCase):
         execute_run.assert_called_once_with(
             ["bash", "--noprofile", "--norc", "-ec", "tox"],
             cwd=Path("/root/project"),
-            env=None,
+            env={},
             stdout=ANY,
             stderr=ANY,
         )
diff --git a/lpcraft/config.py b/lpcraft/config.py
index ed6ae7f..98931f6 100644
--- a/lpcraft/config.py
+++ b/lpcraft/config.py
@@ -60,6 +60,10 @@ class Output(ModelConfigDefaults):
 class Job(ModelConfigDefaults):
     """A job definition."""
 
+    # XXX jugmac00 2012-12-17: working with Job's attributes could be
+    # simplified if they would not be optional, but rather return e.g.
+    # an empty list
+
     series: _Identifier
     architectures: List[_Identifier]
     run: Optional[StrictStr]
diff --git a/lpcraft/plugin/hookspecs.py b/lpcraft/plugin/hookspecs.py
index f16f909..e1232f4 100644
--- a/lpcraft/plugin/hookspecs.py
+++ b/lpcraft/plugin/hookspecs.py
@@ -11,3 +11,24 @@ 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"""
+    # Please note: when both a plugin and the configuration file are
+    # providing a `run` command, the one from the configuration file will be
+    # used
+
+
+@hookspec  # type: ignore
+def lpcraft_set_environment() -> dict[str, str | None]:
+    """environment variables to be set"""
+    # Please note: when there is the same environment variable provided by
+    # the plugin and the configuration file, the one in the configuration
+    # file will be taken into account
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 77%
rename from lpcraft/plugin/tests/test_builtin_plugins.py
rename to lpcraft/plugin/tests/test_plugins.py
index 79b0a1d..270ad7a 100644
--- a/lpcraft/plugin/tests/test_builtin_plugins.py
+++ b/lpcraft/plugin/tests/test_plugins.py
@@ -14,7 +14,7 @@ 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
             """
         )
@@ -65,14 +61,14 @@ class TestBuiltinPlugins(CommandBaseTestCase):
                 call(
                     ["apt", "install", "-y", "tox", "nginx", "apache2"],
                     cwd=PosixPath("/root/project"),
-                    env=None,
+                    env={"PLUGIN": "tox"},
                     stdout=ANY,
                     stderr=ANY,
                 ),
                 call(
                     ["bash", "--noprofile", "--norc", "-ec", "tox"],
                     cwd=PosixPath("/root/project"),
-                    env=None,
+                    env={"PLUGIN": "tox"},
                     stdout=ANY,
                     stderr=ANY,
                 ),
@@ -110,3 +106,51 @@ 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_run_command_from_configuration_takes_precedence(
+        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
+                    packages: [nginx, apache2]
+                    run: ls
+                    plugin: tox
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        self.run_command("run")
+
+        self.assertEqual(
+            [
+                call(
+                    ["apt", "install", "-y", "tox", "nginx", "apache2"],
+                    cwd=PosixPath("/root/project"),
+                    env={"PLUGIN": "tox"},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+                call(
+                    ["bash", "--noprofile", "--norc", "-ec", "ls"],
+                    cwd=PosixPath("/root/project"),
+                    env={"PLUGIN": "tox"},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+            ],
+            execute_run.call_args_list,
+        )
diff --git a/lpcraft/plugins/tox.py b/lpcraft/plugins/tox.py
index 2952371..00ba66c 100644
--- a/lpcraft/plugins/tox.py
+++ b/lpcraft/plugins/tox.py
@@ -15,3 +15,14 @@ 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"
+
+    @hookimpl  # type: ignore
+    def lpcraft_set_environment(self) -> dict[str, str | None]:
+        # XXX jugmac00 2021-12-17: this was added to raise coverage and is not
+        # necessary. Let's remove this once we have a plugin which actually
+        # needs to set environment variables.
+        return {"PLUGIN": "tox"}