Repository: osbuild/osbuild
Pull request #1435 (2023-11-09 11:08:15)

stages(kickstart): implement "reboot" option
This commit implements the `reboot` option for kickstart files.

Note that there are two ways this can be enabled via the json.
Either via a boolean or by passing a dict with options.
```
{"reboot": true}
{"reboot": {"eject": true, "kexec": true}
```

Passing the empty dict
```
{"reboot": {}}
```
is not allowed by the schema.

mvo5: > There's a typo in the commit message:
> 
> > "clearpart" have have the same behavior
> 
> Other than that I think we can do something like requiring at least one property to be passed when `eject` is an object to alleviate the thinking about 'what does an empty dict mean'.

Great suggestion! I changed the code (and schema) this way now that the empty dict is not allowed.
achilleas-k: The `clearpart` test is failing.  It seems it's not technically wrong to have `clearpart` without an option:
```
$ cat test.ks
clearpart
$ ksvalidator test.ks

Checking kickstart file test.ks

$
```
achilleas-k: > requiring at least one property to be passed when `eject` is an object

I was a bit confused here.  Did you mean `reboot`?
mvo5: > The `clearpart` test is failing. It seems it's not technically wrong to have `clearpart` without an option:
> 
> ```
> $ cat test.ks
> clearpart
> $ ksvalidator test.ks
> 
> Checking kickstart file test.ks
> 
> $
> ```

Sorry! That was my bad, I pushed this a bit too hastily (right before a meeting, I really should have learned by now that doing anything in a rush is a bad idea).  

I fixed it now to make an empty `clearpart` an invalid option. Technically it's fine to have it without an option but the kickstart docs say: `Removes partitions from the system, prior to creation of new partitions. By default, no partitions are removed.`  which I read as "it won't do anything without an option" and requiring an option avoid the awkward question if an empty dict is valid or not.


> There's a typo in the commit message:
> 
> > "clearpart" have have the same behavior
> 
> Other than that I think we can do something like requiring at least one property to be passed when `eject` is an object to alleviate the thinking about 'what does an empty dict mean'.

Thanks, I fixed the typo (by removing this part of the commit message and split the commit in two as it should have been in the first place, sorry for that). Also thanks for the suggestion about requiring that a dict for `reboot` needs to have at least one option, I really like this !

Patch:
diff --git a/stages/org.osbuild.kickstart b/stages/org.osbuild.kickstart
index 82f949c1..3994aa04 100755
--- a/stages/org.osbuild.kickstart
+++ b/stages/org.osbuild.kickstart
@@ -139,6 +139,13 @@ SCHEMA = r"""
   "clearpart": {
     "description": "Removes partitions from the system, prior to creation of new partitions",
     "type": "object",
+    "anyOf": [
+      {"required": ["all"]},
+      {"required": ["drives"]},
+      {"required": ["list"]},
+      {"required": ["disklabel"]},
+      {"required": ["linux"]}
+    ],
     "properties": {
       "all": {
         "description": "Erases all partitions from the system",
@@ -170,6 +177,27 @@ SCHEMA = r"""
         "type": "boolean"
       }
     }
+  },
+  "reboot": {
+    "description": "Reboot after the installation is successfully completed",
+    "oneOf": [
+    {
+      "type": "boolean"
+    }, {
+      "type": "object",
+      "additionalProperties": false,
+      "anyOf": [{"required": ["eject"]}, {"required": ["kexec"]}],
+      "properties": {
+        "eject": {
+          "description": "Attempt to eject the installation media before rebooting",
+          "type": "boolean"
+        },
+        "kexec": {
+          "description": "Use this option to reboot into the new system using the kexec",
+          "type": "boolean"
+        }
+      }
+    }]
   }
 }
 """
@@ -259,6 +287,19 @@ def make_clearpart(options: Dict) -> str:
     return cmd
 
 
+def make_reboot(options):
+    reboot = options.get("reboot", None)
+    if not reboot:
+        return ""
+    cmd = "reboot"
+    if isinstance(reboot, dict):
+        if reboot.get("eject"):
+            cmd += " --eject"
+        if reboot.get("kexec"):
+            cmd += " --kexec"
+    return cmd
+
+
 def main(tree, options):
     path = options["path"].lstrip("/")
     ostree = options.get("ostree")
@@ -302,6 +343,10 @@ def main(tree, options):
     if clearpart:
         config += [clearpart]
 
+    reboot = make_reboot(options)
+    if reboot:
+        config += [reboot]
+
     target = os.path.join(tree, path)
     base = os.path.dirname(target)
     os.makedirs(base, exist_ok=True)


Test patch:
diff --git a/stages/test/test_kickstart.py b/stages/test/test_kickstart.py
index 1bd64ace..c149e54e 100644
--- a/stages/test/test_kickstart.py
+++ b/stages/test/test_kickstart.py
@@ -55,7 +55,6 @@ from osbuild.testutil.imports import import_module_from_path
      'sshkey --username someusr "ssh-rsa not-really-a-real-key"'
      ),
     ({"zerombr": "true"}, "zerombr"),
-    ({"clearpart": {}}, "clearpart"),
     ({"clearpart": {"all": True}}, "clearpart --all"),
     ({"clearpart": {"drives": ["hda", "hdb"]}}, "clearpart --drives=hda,hdb",),
     ({"clearpart": {"drives": ["hda"]}}, "clearpart --drives=hda"),
@@ -88,6 +87,13 @@ from osbuild.testutil.imports import import_module_from_path
       },
         "lang en_US.UTF-8\nkeyboard us\ntimezone UTC\nzerombr\nclearpart --all --drives=sd*|hd*|vda,/dev/vdc",
      ),
+    # no reboot for an empty dict
+    ({"reboot": True}, "reboot"),
+    ({"reboot": {"eject": False}}, "reboot"),
+    ({"reboot": {"eject": True}}, "reboot --eject"),
+    ({"reboot": {"kexec": False}}, "reboot"),
+    ({"reboot": {"kexec": True}}, "reboot --kexec"),
+    ({"reboot": {"eject": True, "kexec": True}}, "reboot --eject --kexec"),
 ])
 def test_kickstart(tmp_path, test_input, expected):
     ks_stage_path = os.path.join(os.path.dirname(__file__), "../org.osbuild.kickstart")
@@ -105,12 +111,14 @@ def test_kickstart(tmp_path, test_input, expected):
     assert ks_content == expected + "\n"
 
     # double check with pykickstart if the file looks valid
-    subprocess.check_call(["ksvalidator", ks_path])
+    if expected:
+        subprocess.check_call(["ksvalidator", ks_path])
 
 
 @pytest.mark.parametrize("test_data,expected_err", [
     # BAD pattern, ensure some obvious ways to write arbitrary
     # kickstart files will not work
+    ({"clearpart": {}}, "{} is not valid "),
     ({"clearpart": {"disklabel": r"\n%pre\necho p0wnd"}}, r"p0wnd' does not match"),
     ({"clearpart": {"drives": [" --spaces-dashes-not-allowed"]}}, "' --spaces-dashes-not-allowed' does not match"),
     ({"clearpart": {"drives": ["\n%pre not allowed"]}}, "not allowed' does not match"),
@@ -118,10 +126,16 @@ def test_kickstart(tmp_path, test_input, expected):
     ({"clearpart": {"list": ["\n%pre not allowed"]}}, "not allowed' does not match"),
     ({"clearpart": {"list": ["no,comma"]}}, "no,comma' does not match"),
     ({"clearpart": {"disklabel": "\n%pre not allowed"}}, "not allowed' does not match"),
+    # schema ensures reboot has at least one option set
+    ({"reboot": {}}, "{} is not valid under any of the given schemas"),
+    ({"reboot": "random-string"}, "'random-string' is not valid "),
+    ({"reboot": {"random": "option"}}, "{'random': 'option'} is not valid "),
     # GOOD pattern we want to keep working
     ({"clearpart": {"drives": ["sd*|hd*|vda", "/dev/vdc"]}}, ""),
     ({"clearpart": {"drives": ["disk/by-id/scsi-58095BEC5510947BE8C0360F604351918"]}}, ""),
     ({"clearpart": {"list": ["sda2", "sda3", "sdb1"]}}, ""),
+    ({"reboot": True}, ""),
+    ({"reboot": {"kexec": False}}, ""),
 ])
 def test_schema_validation_smoke(test_data, expected_err):
     name = "org.osbuild.kickstart"
@@ -139,7 +153,7 @@ def test_schema_validation_smoke(test_data, expected_err):
     res = schema.validate(test_input)
 
     if expected_err == "":
-        assert res.valid is True
+        assert res.valid is True, f"err: {[e.as_dict() for e in res.errors]}"
     else:
         assert res.valid is False
         assert len(res.errors) == 1