Repository: osbuild/osbuild
Pull request #2000 (2025-02-04 13:57:15)

stages: Where is mandatory in systemd.unit.create
`systemd.mount(5)` describes both `What` and `Where` as mandatory options for the `Mount` section of a unit.

achilleas-k: > Sure, but this breaks previous manifests that didn't define where right? Or did those fail to boot anyhow?

You're absolutely right that this is a backwards-incompatible change.  We used to have a systemd unit validator in the stage at some point iirc, but I remember removing it because it doesn't always work exactly right when resolving references to other units when used with `--root` (I should open an issue for that).  If we still had that, I'd say this is an acceptable change now because the stage would have failed anyway.  Without it, the stage succeeds but the mount unit is invalid, so I assume it would either fail to boot or fail to mount the unit.

So your point stands and we might want to talk about whether we should restrict the schema here or not.  We've done similar changes before, but I think it's good to examine each situation separately.  I'm fine with keeping it open and adding the restriction only to the validator in images for manifest generation.
achilleas-k: > I remember removing it because it doesn't always work exactly right when resolving references to other units when used with `--root`

f255fba09f2f0c879d92df37e3d9f8e31903720d

Patch:
diff --git a/stages/org.osbuild.systemd.unit.create.meta.json b/stages/org.osbuild.systemd.unit.create.meta.json
index 254bc0fa..f8cc3c58 100644
--- a/stages/org.osbuild.systemd.unit.create.meta.json
+++ b/stages/org.osbuild.systemd.unit.create.meta.json
@@ -265,7 +265,8 @@
             "type": "object",
             "description": "'Mount' configuration section of a unit file.",
             "required": [
-              "What"
+              "What",
+              "Where"
             ],
             "properties": {
               "What": {


Test patch:
diff --git a/stages/test/test_systemd_unit_create.py b/stages/test/test_systemd_unit_create.py
index a748e2f0..96ca747e 100644
--- a/stages/test/test_systemd_unit_create.py
+++ b/stages/test/test_systemd_unit_create.py
@@ -29,7 +29,7 @@ STAGE_NAME = "org.osbuild.systemd.unit.create"
             "filename": "foo.mount",
             "config": {
                 "Unit": {},
-                "Mount": {"What": ""},
+                "Mount": {"What": "", "Where": ""},
                 "Install": {},
             },
         },
@@ -39,24 +39,44 @@ STAGE_NAME = "org.osbuild.systemd.unit.create"
         {
             "filename": "foo.mount",
             "config": {
-                "Mount": {"What": ""},
+                "Mount": {"What": "", "Where": ""},
             },
         },
         "",
     ),
+
     # bad
+    # # No filename
     ({"config": {"Unit": {}, "Service": {}, "Install": {}}}, "'filename' is a required property"),
+
+    # # no config block
     ({"filename": "foo.service"}, "'config' is a required property"),
+
+    # # no Unit section in .service
     ({"filename": "foo.service", "config": {"Service": {}, "Install": {}}},
      "{'Service': {}, 'Install': {}} is not valid under any of the given schemas"),
+
+    # # no Service section in .service
     ({"filename": "foo.service", "config": {"Unit": {}, "Install": {}}},
      "{'Unit': {}, 'Install': {}} is not valid under any of the given schemas"),
+
+    # # no Install section in .service
     ({"filename": "foo.service", "config": {"Unit": {}, "Service": {}}},
      "{'Unit': {}, 'Service': {}} is not valid under any of the given schemas"),
-    ({"filename": "foo.service", "config": {"Service": {}, "Mount": {"What": ""}}},
-     "{'Service': {}, 'Mount': {'What': ''}} is not valid under any of the given schemas"),
-    ({"filename": "foo.service", "config": {"Unit": {}, "Service": {}, "Mount": {"What": ""}, "Install": {}}},
-     "{'Unit': {}, 'Service': {}, 'Mount': {'What': ''}, 'Install': {}} is not valid under any of the given schemas"),
+
+    # # .mount unit without Where
+    ({"filename": "foo.mount", "config": {"Unit": {}, "Mount": {"What": ""}, "Install": {}}},
+     "'Where' is a required property"),
+
+    # # .mount unit without What
+    ({"filename": "foo.mount", "config": {"Unit": {}, "Mount": {"Where": ""}, "Install": {}}},
+     "'What' is a required property"),
+
+    # # .service unit with Mount section
+    ({"filename": "foo.service", "config": {"Unit": {}, "Service": {},
+                                            "Mount": {"What": "", "Where": ""}, "Install": {}}},
+     "{'Unit': {}, 'Service': {}, 'Mount': {'What': '', 'Where': ''}, "
+     "'Install': {}} is not valid under any of the given schemas"),
 ])
 @pytest.mark.parametrize("stage_schema", ["1"], indirect=True)
 def test_schema_validation(stage_schema, test_data, expected_err):
@@ -90,7 +110,7 @@ def test_schema_validation(stage_schema, test_data, expected_err):
             "filename": "foo.mount",
             "config": {
                 "Unit": {},
-                "Mount": {"What": ""},
+                "Mount": {"What": "", "Where": ""},
                 "Install": {},
             },
         },
@@ -100,7 +120,7 @@ def test_schema_validation(stage_schema, test_data, expected_err):
         {
             "filename": "foo.mount",
             "config": {
-                "Mount": {"What": ""},
+                "Mount": {"What": "", "Where": ""},
             },
         },
         "",