Repository: osbuild/osbuild
Pull request #1650 (2024-03-07 12:06:30)

test: add unit-{type,path} test coverage
Small followup for https://github.com/osbuild/osbuild/pull/1620


Patch:
diff --git a/osbuild/util/containers.py b/osbuild/util/containers.py
index 0e0f801b..9b9f6a70 100644
--- a/osbuild/util/containers.py
+++ b/osbuild/util/containers.py
@@ -4,7 +4,7 @@ import subprocess
 import tempfile
 from contextlib import contextmanager
 
-from osbuild.util.mnt import MountGuard
+from osbuild.util.mnt import MountGuard, MountPermissions
 
 
 def is_manifest_list(data):
@@ -123,7 +123,7 @@ def containers_storage_source(image, image_filepath, container_format):
     with MountGuard() as mg:
         # NOTE: the ostree.deploy.container needs explicit `rw` access to
         # the containers-storage store even when bind mounted.
-        mg.mount(image_filepath, storage_path, rw=True)
+        mg.mount(image_filepath, storage_path, permissions=MountPermissions.READ_WRITE)
 
         image_id = image["checksum"].split(":")[1]
         image_source = f"{container_format}:[{driver}@{storage_path}+/run/containers/storage]{image_id}"
diff --git a/osbuild/util/mnt.py b/osbuild/util/mnt.py
index b5fefb89..41231e1a 100644
--- a/osbuild/util/mnt.py
+++ b/osbuild/util/mnt.py
@@ -2,13 +2,20 @@
 """
 
 import contextlib
+import enum
 import subprocess
+from typing import Optional
+
+
+class MountPermissions(enum.Enum):
+    READ_WRITE = "rw"
+    READ_ONLY = "ro"
 
 
 def mount(source, target, bind=True, ro=True, private=True, mode="0755"):
     options = []
     if ro:
-        options += ["ro"]
+        options += [MountPermissions.READ_ONLY.value]
     if mode:
         options += [mode]
 
@@ -44,14 +51,14 @@ class MountGuard(contextlib.AbstractContextManager):
     def __init__(self):
         self.mounts = []
 
-    def mount(self, source, target, bind=True, ro=False, rw=False, mode="0755"):
+    def mount(self, source, target, bind=True, permissions: Optional[MountPermissions] = None, mode="0755"):
         options = []
         if bind:
             options += ["bind"]
-        if ro:
-            options += ["ro"]
-        if rw:
-            options += ["rw"]
+        if permissions:
+            if permissions not in list(MountPermissions):
+                raise ValueError(f"unknown filesystem permissions: {permissions}")
+            options += [permissions.value]
         if mode:
             options += [mode]
 
diff --git a/stages/org.osbuild.bootupd.gen-metadata b/stages/org.osbuild.bootupd.gen-metadata
index 67ab2324..fe4072ec 100755
--- a/stages/org.osbuild.bootupd.gen-metadata
+++ b/stages/org.osbuild.bootupd.gen-metadata
@@ -15,7 +15,7 @@ import subprocess
 import sys
 
 import osbuild.api
-from osbuild.util.mnt import MountGuard
+from osbuild.util.mnt import MountGuard, MountPermissions
 
 SCHEMA_2 = r"""
 "options": {
@@ -29,7 +29,7 @@ def main(tree):
         for source in ("/dev", "/sys", "/proc"):
             target = os.path.join(tree, source.lstrip("/"))
             os.makedirs(target, exist_ok=True)
-            mounter.mount(source, target, ro=True)
+            mounter.mount(source, target, permissions=MountPermissions.READ_ONLY)
 
         # Using a non-default sysroot is not supported by the generate-update-metadata command, so we need to chroot
         # into the tree and run against /


Test patch:
diff --git a/stages/test/test_systemd_unit_create.py b/stages/test/test_systemd_unit_create.py
index 74e05f48..b0f251e0 100644
--- a/stages/test/test_systemd_unit_create.py
+++ b/stages/test/test_systemd_unit_create.py
@@ -46,9 +46,17 @@ def test_schema_validation(stage_schema, test_data, expected_err):
         testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
-def test_systemd_unit_create_simple(tmp_path, stage_module):
+@pytest.mark.parametrize("unit_type,unit_path,expected_prefix", [
+    ("system", "usr", "usr/lib/systemd/system"),
+    ("system", "etc", "etc/systemd/system"),
+    ("global", "usr", "usr/lib/systemd/user"),
+    ("global", "etc", "etc/systemd/user"),
+])
+def test_systemd_unit_create(tmp_path, stage_module, unit_type, unit_path, expected_prefix):
     options = {
         "filename": "create-directory.service",
+        "unit-type": unit_type,
+        "unit-path": unit_path,
         "config": {
             "Unit": {
                 "Description": "Create directory",
@@ -75,7 +83,7 @@ def test_systemd_unit_create_simple(tmp_path, stage_module):
             }
         }
     }
-    expected_unit_path = tmp_path / "usr/lib/systemd/system/create-directory.service"
+    expected_unit_path = tmp_path / expected_prefix / "create-directory.service"
     # should the stage create the dir?
     expected_unit_path.parent.mkdir(parents=True)
 
diff --git a/test/mod/test_util_mnt.py b/test/mod/test_util_mnt.py
index 76a17f12..3b706642 100644
--- a/test/mod/test_util_mnt.py
+++ b/test/mod/test_util_mnt.py
@@ -21,6 +21,14 @@ def test_mount_guard_failure_msg(tmp_path):
     assert "special device /dev/invalid-src does not exist" in str(e.value)
 
 
+@pytest.mark.skipif(os.getuid() != 0, reason="root only")
+def test_mount_guard_incorrect_permissions_msg(tmp_path):
+    with pytest.raises(ValueError) as e:
+        with MountGuard() as mg:
+            mg.mount("/dev/invalid-src", tmp_path, permissions="abc")
+    assert "unknown filesystem permissions" in str(e.value)
+
+
 # This needs a proper refactor so that FileSystemMountService just uses
 # a common mount helper.
 class FakeFileSystemMountService(FileSystemMountService):