Merge ~pelpsi/lpci:snaps-key-improved-to-specify-channel-and-classic-parameters into lpci:main

[Simone Pelosi]

Added support for channel and classic parameters

Snap keys now support channel and classic parameters.
Snap key new syntax <name>:<channel>:<classic>

LP: #1995101

[Jürgen Gmach (jugmac00)]

Using a colon as a delimiter looks like an elegant way to encode information.

As mentioned below, is this an official way?

If not, we should have a quick discussion about alternative ways at the standup, e.g. having separate keys for name, channel and classic.

Anyway, please rewrite the way you parse the snap string.

review: Needs Fixing

[Jürgen Gmach (jugmac00)]

It looks like that I missed to make you aware that we use the Pydantic library to parse the yaml input. In order to leverage that library, you need to create a new configuration class, see inline comments. Sorry for the extra work! Please reach out in case you need any help or a quick introduction to this topic.

review: Needs Fixing

[Jürgen Gmach (jugmac00)]

This looks good! Ideally, we could improve the error messages and make them more helpful for our users, see inline comments.

There is one thing we need to discuss. This change breaks backwards compatibility, ie configurations a la
```
snaps = [firefox, chromium]
```
will no longer work. In my last comment (see inline comments when you click on "show diff comments") I showed you a possible way to stay backwards compatible.

Did you try to do it? Have you encountered any problems? Or did you intentionally decide to break backwards compatibility?

[Jürgen Gmach (jugmac00)]

This looks really good now! Please have a look at my comments before merging.

And please remember... for lpcraft we do not have a merge bot, so after applying the suggested changes, you need to merge the branch yourself.

Imho it would make sense to squash your commits prior to merging.

Thanks for your patience!

review: Approve

diff --git a/NEWS.rst b/NEWS.rst
index d38f50c..67eb3af 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -2,6 +2,12 @@
 Version history
 ===============
 
+0.0.51 (2023-04-05)
+===================
+- Add support for snap's channel and confinement level.
+  See https://snapcraft.io/docs/channels and
+  https://snapcraft.io/docs/snap-confinement.
+
 0.0.50 (2023-03-20)
 ===================
 - Rebuild the Snap package to include updated system packages.
diff --git a/docs/configuration.rst b/docs/configuration.rst
index d25c96d..8655653 100644
--- a/docs/configuration.rst
+++ b/docs/configuration.rst
@@ -63,6 +63,7 @@ Job definitions
 
 ``snaps`` (optional)
     Snaps to install as dependencies of this job.
+    Also see the :ref:`snap-properties` section below.
 
 ``environment`` (optional)
     A mapping of environment variable names to values, to be set while
@@ -171,6 +172,26 @@ artifacts.  (This mirrors the output file structure created by ``lpcraft run
     to which the artifacts of the chosen job will be copied; the directory
     will be created if necessary.  Paths may not escape the build tree.
 
+.. _snap-properties:
+
+Snap properties
+-----------------
+
+``name``
+    The name of a snap will be installed..
+
+``channel`` (optional)
+    `Channel <https://snapcraft.io/docs/channels>`_
+    defining which release of a snap want to install.
+    Default value: ``latest/stable``.
+
+
+``classic`` (optional)
+    Classic boolean defining whether to use or not
+    `confiment=classic
+    <https://snapcraft.io/docs/snap-confinement>`_.
+    Default value: False.
+
 .. _package-repositories:
 
 Package-repositories properties
diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py
index c0e00d9..3a914df 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -543,12 +543,17 @@ def _run_job(
     ) as instance:
         snaps = list(itertools.chain(*pm.hook.lpcraft_install_snaps()))
         for snap in snaps:
-            emit.progress(f"Running `snap install {snap}`")
+            emit.progress(
+                "Running `snap install "
+                + f"{snap.name} "
+                + f"(channel={snap.channel}, "
+                + f"classic={snap.classic})`"
+            )
             install_from_store(
                 executor=instance,
-                snap_name=snap,
-                channel="latest/stable",
-                classic=True,
+                snap_name=snap.name,
+                channel=snap.channel,
+                classic=snap.classic,
             )
         packages = list(itertools.chain(*pm.hook.lpcraft_install_packages()))
         if packages:
diff --git a/lpcraft/commands/tests/test_run.py b/lpcraft/commands/tests/test_run.py
index 4466295..361661c 100644
--- a/lpcraft/commands/tests/test_run.py
+++ b/lpcraft/commands/tests/test_run.py
@@ -1956,7 +1956,7 @@ class TestRun(RunBaseTestCase):
     @responses.activate
     @patch("lpcraft.commands.run.get_provider")
     @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
-    def test_install_snaps(
+    def test_install_snaps_classic_parameter(
         self, mock_get_host_architecture, mock_get_provider
     ):
         def run_side_effect(
@@ -1985,7 +1985,11 @@ class TestRun(RunBaseTestCase):
                     series: focal
                     architectures: amd64
                     run: tox
-                    snaps: [chromium, firefox]
+                    snaps:
+                        - name: chromium
+                          classic: True
+                        - name: firefox
+                          classic: True
             """
         )
         Path(".launchpad.yaml").write_text(config)
@@ -2052,6 +2056,379 @@ class TestRun(RunBaseTestCase):
             execute_run.call_args_list,
         )
 
+    @responses.activate
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_install_snaps_provided_as_list_of_strings(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+        def run_side_effect(
+            command: List[str], **kwargs: Any
+        ) -> "subprocess.CompletedProcess[bytes]":
+            if command[0] == "curl":
+                response = {"result": {"revision": "1"}, "status-code": 200}
+                return subprocess.CompletedProcess(
+                    [], 0, stdout=json.dumps(response).encode()
+                )
+            else:
+                return subprocess.CompletedProcess([], 0)
+
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        execute_run = launcher.return_value.execute_run
+        execute_run.side_effect = run_side_effect
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: tox
+                    snaps: [chromium, firefox]
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+        result = self.run_command("run")
+        self.assertEqual(0, result.exit_code)
+        self.assertEqual(
+            [
+                call(
+                    [
+                        "snap",
+                        "install",
+                        "chromium",
+                        "--channel",
+                        "latest/stable",
+                    ],
+                    check=True,
+                    capture_output=True,
+                ),
+                call(
+                    [
+                        "curl",
+                        "--silent",
+                        "--unix-socket",
+                        "/run/snapd.socket",
+                        "http://localhost/v2/snaps/chromium",
+                    ],
+                    check=True,
+                    capture_output=True,
+                ),
+                call(
+                    [
+                        "snap",
+                        "install",
+                        "firefox",
+                        "--channel",
+                        "latest/stable",
+                    ],
+                    check=True,
+                    capture_output=True,
+                ),
+                call(
+                    [
+                        "curl",
+                        "--silent",
+                        "--unix-socket",
+                        "/run/snapd.socket",
+                        "http://localhost/v2/snaps/firefox",
+                    ],
+                    check=True,
+                    capture_output=True,
+                ),
+                call(
+                    ["bash", "--noprofile", "--norc", "-ec", "tox"],
+                    cwd=Path("/build/lpcraft/project"),
+                    env={},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+            ],
+            execute_run.call_args_list,
+        )
+
+    @responses.activate
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_install_snaps_wrong_array_format(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: tox
+                    snaps: [1, True, 3]
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+        result = self.run_command("run")
+        self.assertEqual(1, result.exit_code)
+        self.assertRegex(
+            str(result.errors[0]),
+            "You configured a Snap, "
+            + "but you used an unknown format. "
+            + "Please refer to the documentation for an "
+            + "overview of supported formats.",
+        )
+
+    @responses.activate
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_install_snaps_channel_parameter(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+        def run_side_effect(
+            command: List[str], **kwargs: Any
+        ) -> "subprocess.CompletedProcess[bytes]":
+            if command[0] == "curl":
+                response = {"result": {"revision": "1"}, "status-code": 200}
+                return subprocess.CompletedProcess(
+                    [], 0, stdout=json.dumps(response).encode()
+                )
+            else:
+                return subprocess.CompletedProcess([], 0)
+
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        execute_run = launcher.return_value.execute_run
+        execute_run.side_effect = run_side_effect
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: tox
+                    snaps:
+                        - name: black
+                          channel: 22/stable
+                        - firefox
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        result = self.run_command("run")
+
+        self.assertEqual(0, result.exit_code)
+        self.assertEqual(
+            [
+                call(
+                    [
+                        "snap",
+                        "install",
+                        "black",
+                        "--channel",
+                        "22/stable",
+                    ],
+                    check=True,
+                    capture_output=True,
+                ),
+                call(
+                    [
+                        "curl",
+                        "--silent",
+                        "--unix-socket",
+                        "/run/snapd.socket",
+                        "http://localhost/v2/snaps/black",
+                    ],
+                    check=True,
+                    capture_output=True,
+                ),
+                call(
+                    [
+                        "snap",
+                        "install",
+                        "firefox",
+                        "--channel",
+                        "latest/stable",
+                    ],
+                    check=True,
+                    capture_output=True,
+                ),
+                call(
+                    [
+                        "curl",
+                        "--silent",
+                        "--unix-socket",
+                        "/run/snapd.socket",
+                        "http://localhost/v2/snaps/firefox",
+                    ],
+                    check=True,
+                    capture_output=True,
+                ),
+                call(
+                    ["bash", "--noprofile", "--norc", "-ec", "tox"],
+                    cwd=Path("/build/lpcraft/project"),
+                    env={},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+            ],
+            execute_run.call_args_list,
+        )
+
+    @responses.activate
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_install_snaps_snap_name_missing(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: tox
+                    snaps:
+                        - classic: True
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        result = self.run_command("run")
+
+        self.assertEqual(1, result.exit_code)
+        self.assertRegex(
+            str(result.errors[0]),
+            "You configured a Snap " + "but you did not specify a name.",
+        )
+
+    @responses.activate
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_install_snaps_snap_channel_none(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: tox
+                    snaps:
+                        - name: chromium
+                          channel:
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        result = self.run_command("run")
+
+        self.assertEqual(1, result.exit_code)
+        self.assertRegex(
+            str(result.errors[0]),
+            "You configured a Snap `channel`, "
+            + "but you did not specify a value.",
+        )
+
+    @responses.activate
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_install_snaps_snap_classic_none(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: tox
+                    snaps:
+                        - name: chromium
+                          classic:
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        result = self.run_command("run")
+
+        self.assertEqual(1, result.exit_code)
+        self.assertRegex(
+            str(result.errors[0]),
+            "You configured a Snap `classic`, "
+            + "but you did not specify a value. "
+            + "Valid values would either be `True` or `False`.",
+        )
+
+    @responses.activate
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_install_snaps_classic_wrong_value(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        config = dedent(
+            """
+            pipeline:
+                - test
+
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: tox
+                    snaps:
+                        - name: chromium
+                          classic: wrong_value
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        result = self.run_command("run")
+
+        self.assertEqual(1, result.exit_code)
+        self.assertRegex(
+            str(result.errors[0]),
+            "You configured a Snap `classic`, "
+            + "but you did not specify a valid value. "
+            + "Valid values would either be `True` or `False`.",
+        )
+
     @patch("lpcraft.commands.run.get_provider")
     @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
     def test_install_system_packages(
diff --git a/lpcraft/config.py b/lpcraft/config.py
index 5b8d126..0fdb9b4 100644
--- a/lpcraft/config.py
+++ b/lpcraft/config.py
@@ -242,6 +242,33 @@ class PackageRepository(ModelConfigDefaults):
                     yield f"{format.value} {self.url!s} {suite.value} {' '.join(self.components)}"  # noqa: E501
 
 
+class Snap(ModelConfigDefaults):
+    """A snap definition."""
+
+    name: StrictStr
+    channel: Optional[StrictStr] = "latest/stable"
+    classic: Optional[bool] = False
+
+    @validator("channel")
+    def prevent_channel_none(cls, v: StrictStr) -> Any:
+        if v is None:
+            raise ValueError(
+                "You configured a Snap `channel`, "
+                + "but you did not specify a value."
+            )
+        return v
+
+    @validator("classic")
+    def prevent_classic_none(cls, v: bool) -> Any:
+        if v is None:
+            raise ValueError(
+                "You configured a Snap `classic`, "
+                + "but you did not specify a value. "
+                + "Valid values would either be `True` or `False`."
+            )
+        return v
+
+
 class Job(ModelConfigDefaults):
     """A job definition."""
 
@@ -257,7 +284,7 @@ class Job(ModelConfigDefaults):
     environment: Optional[Dict[str, Optional[str]]]
     output: Optional[Output]
     input: Optional[Input]
-    snaps: Optional[List[StrictStr]]
+    snaps: Optional[List[Snap]]
     packages: Optional[List[StrictStr]]
     package_repositories: List[PackageRepository] = []
     plugin: Optional[StrictStr]
@@ -271,6 +298,36 @@ class Job(ModelConfigDefaults):
             v = [v]
         return v
 
+    @pydantic.validator("snaps", pre=True)
+    def validate_snaps(cls, v: List[Any]) -> Any:
+        clean_values = []
+        for value in v:
+            # Backward compatibility, i.e. [chromium, firefox]
+            if type(value) is str:
+                clean_values.append({"name": value})
+            elif type(value) is dict:
+                if "name" not in value or value["name"] is None:
+                    raise ValueError(
+                        "You configured a Snap "
+                        + "but you did not specify a name."
+                    )
+                if "classic" in value and value["classic"] is not None:
+                    if type(value["classic"]) is not bool:
+                        raise ValueError(
+                            "You configured a Snap `classic`, "
+                            + "but you did not specify a valid value. "
+                            + "Valid values would either be `True` or `False`."
+                        )
+                clean_values.append(value)
+            else:
+                raise ValueError(
+                    "You configured a Snap, "
+                    + "but you used an unknown format. "
+                    + "Please refer to the documentation for an "
+                    + "overview of supported formats."
+                )
+        return clean_values
+
     @pydantic.validator("package_repositories")
     def validate_package_repositories(
         cls, v: List[PackageRepository], values: Dict[StrictStr, Any]
diff --git a/lpcraft/plugin/lib.py b/lpcraft/plugin/lib.py
index 2b22680..92510e6 100644
--- a/lpcraft/plugin/lib.py
+++ b/lpcraft/plugin/lib.py
@@ -10,7 +10,7 @@ The provided functionality can be extended by additional plugins.
 
 from __future__ import annotations
 
-from lpcraft.config import Job
+from lpcraft.config import Job, Snap
 from lpcraft.plugin import hookimpl
 
 
@@ -28,7 +28,7 @@ class InternalPlugins:
         return []
 
     @hookimpl  # type: ignore
-    def lpcraft_install_snaps(self) -> list[str]:
+    def lpcraft_install_snaps(self) -> list[Snap]:
         if self.config.snaps:
             return self.config.snaps
         return []
diff --git a/lpcraft/tests/test_config.py b/lpcraft/tests/test_config.py
index d64533a..a5b875a 100644
--- a/lpcraft/tests/test_config.py
+++ b/lpcraft/tests/test_config.py
@@ -26,6 +26,7 @@ from lpcraft.config import (
     PackageSuite,
     PackageType,
     PPAShortFormURL,
+    Snap,
     get_ppa_url_parts,
 )
 from lpcraft.errors import CommandError
@@ -370,12 +371,18 @@ class TestConfig(TestCase):
                     test:
                         series: focal
                         architectures: amd64
-                        snaps: [chromium, firefox]
+                        snaps: [name: chromium, name: firefox]
                 """
             )
         )
         config = Config.load(path)
-        self.assertEqual(["chromium", "firefox"], config.jobs["test"][0].snaps)
+        self.assertEqual(
+            [
+                Snap(name="chromium", channel="latest/stable", classic=False),
+                Snap(name="firefox", channel="latest/stable", classic=False),
+            ],
+            config.jobs["test"][0].snaps,
+        )
 
     def test_load_config_without_snaps(self):
         path = self.create_config(