Repository: osbuild/osbuild
Pull request #1600 (2024-02-20 09:19:16)

tox,tools: add tools back to lintables
During the review of https://github.com/osbuild/osbuild/pull/1598 we noticed that tools is not checked by pylint (and the other linters).

This PR adds it back and fixes the issues that pylint found.

For the non-obvious changes I added comments in the commit message and I'm happy (of course) to change/adjust the changes as needed.

One issue that this PR may have is that it uses "tox-backticks" in tox.ini. AFAIK this is only available via pip but not packaged. If this is a problem the way to re-enable pylint (and friends) needs to be re-evaluated.


Patch:
diff --git a/stages/org.osbuild.grub2 b/stages/org.osbuild.grub2
index 72e6e1a0..5fb784fe 100755
--- a/stages/org.osbuild.grub2
+++ b/stages/org.osbuild.grub2
@@ -202,11 +202,24 @@ SCHEMA = """
         "description": "Default boot entry",
         "type": "string"
       },
+      "disable_recovery": {
+        "type": "boolean"
+      },
+      "disable_submenu": {
+        "type": "boolean"
+      },
+      "distributor": {
+        "description": "Name of the distributor",
+        "type": "string"
+      },
+      "terminal": {
+        "$ref": "#/definitions/terminal"
+      },
       "terminal_input": {
-         "$ref": "#/definitions/terminal"
+        "$ref": "#/definitions/terminal"
       },
       "terminal_output": {
-         "$ref": "#/definitions/terminal"
+        "$ref": "#/definitions/terminal"
       },
       "timeout": {
         "description": "Timeout in seconds",
@@ -214,6 +227,10 @@ SCHEMA = """
         "minimum": 0,
         "default": 0
       },
+      "timeout_style": {
+        "type": "string",
+        "enum": ["hidden", "menu", "countdown"]
+      },
       "serial": {
         "description": "The command to configure the serial console",
         "type": "string"
@@ -371,10 +388,15 @@ class GrubConfig:
         self.ignition = False
         self.greenboot = False
         self.kernel_opts = ""
+        self.disable_recovery = None
+        self.disable_submenu = None
+        self.distributor = None
         self.serial = ""
+        self.terminal = None
         self.terminal_input = None
         self.terminal_output = None
         self.timeout = 0
+        self.timeout_style = None
 
     @property
     def grubfs(self):
@@ -472,9 +494,22 @@ class GrubConfig:
             "GRUB_ENABLE_BLSCFG=true\n"
         )
 
+        if self.disable_recovery is not None:
+            data += f"GRUB_DISABLE_RECOVERY={str(self.disable_recovery).lower()}\n"
+
+        if self.disable_submenu is not None:
+            data += f"GRUB_DISABLE_SUBMENU={str(self.disable_submenu).lower()}\n"
+
+        if self.distributor:
+            data += f'GRUB_DISTRIBUTOR="{self.distributor}"\n'
+
         if self.serial:
             data += f'GRUB_SERIAL_COMMAND="{self.serial}"\n'
 
+        if self.terminal:
+            val = " ".join(self.terminal)
+            data += f'GRUB_TERMINAL="{val}"\n'
+
         if self.terminal_input:
             val = " ".join(self.terminal_input)
             data += f'GRUB_TERMINAL_INPUT="{val}"\n'
@@ -483,6 +518,9 @@ class GrubConfig:
             val = " ".join(self.terminal_output)
             data += f'GRUB_TERMINAL_OUTPUT="{val}"\n'
 
+        if self.timeout_style:
+            data += f'GRUB_TIMEOUT_STYLE={self.timeout_style}\n'
+
         if self.default_entry is not None:
             data += f'GRUB_DEFAULT={self.default_entry}\n'
 
@@ -525,10 +563,15 @@ def main(tree, options):
     config.ignition = ignition
     config.greenboot = options.get("greenboot", False)
     config.kernel_opts = kernel_opts
+    config.disable_recovery = cfg.get("disable_recovery")
+    config.disable_submenu = cfg.get("disable_submenu")
+    config.distributor = cfg.get("distributor")
     config.serial = cfg.get("serial")
+    config.terminal = cfg.get("terminal")
     config.terminal_input = cfg.get("terminal_input")
     config.terminal_output = cfg.get("terminal_output")
     config.timeout = cfg.get("timeout", 0)
+    config.timeout_style = cfg.get("timeout_style")
     config.default_entry = cfg.get("default")
 
     # Create the configuration file that determines how grub.cfg is generated.
diff --git a/stages/org.osbuild.grub2.legacy b/stages/org.osbuild.grub2.legacy
index 16e676ff..56b7e997 100755
--- a/stages/org.osbuild.grub2.legacy
+++ b/stages/org.osbuild.grub2.legacy
@@ -174,15 +174,26 @@ SCHEMA = """
         "description": "Additional kernel command line options",
         "type": "string"
       },
+      "disable_recovery": {
+        "type": "boolean",
+        "default": true
+      },
+      "disable_submenu": {
+        "type": "boolean",
+        "default": true
+      },
       "distributor": {
         "description": "Name of the distributor",
         "type": "string"
       },
+      "terminal": {
+        "$ref": "#/definitions/terminal"
+      },
       "terminal_input": {
-         "$ref": "#/definitions/terminal"
+        "$ref": "#/definitions/terminal"
       },
       "terminal_output": {
-         "$ref": "#/definitions/terminal"
+        "$ref": "#/definitions/terminal"
       },
       "timeout": {
         "description": "Timeout in seconds",
@@ -190,6 +201,11 @@ SCHEMA = """
         "minimum": 0,
         "default": 0
       },
+      "timeout_style": {
+        "type": "string",
+        "enum": ["hidden", "menu", "countdown"],
+        "default": "countdown"
+      },
       "serial": {
         "description": "The command to configure the serial console",
         "type": "string"
@@ -331,6 +347,7 @@ class GrubEntry:
         return entry
 
 
+# pylint: disable=too-many-instance-attributes
 class GrubConfig:
     def __init__(self, bios, rootfs, bootfs):
         self.bios = bios
@@ -338,11 +355,15 @@ class GrubConfig:
         self.bootfs = bootfs
         self.entries = []
         self.cmdline = ""
+        self.disable_recovery = True
+        self.disable_submenu = True
         self.distributor = ""
         self.serial = ""
+        self.terminal = None
         self.terminal_input = None
         self.terminal_output = None
         self.timeout = 0
+        self.timeout_style = "countdown"
 
     @property
     def grubfs(self):
@@ -432,9 +453,9 @@ class GrubConfig:
         data = (
             f"GRUB_TIMEOUT={self.timeout}\n"
             f'GRUB_CMDLINE_LINUX="{self.cmdline}"\n'
-            "GRUB_DISABLE_SUBMENU=true\n"
-            "GRUB_DISABLE_RECOVERY=true\n"
-            "GRUB_TIMEOUT_STYLE=countdown\n"
+            f"GRUB_DISABLE_SUBMENU={str(self.disable_submenu).lower()}\n"
+            f"GRUB_DISABLE_RECOVERY={str(self.disable_recovery).lower()}\n"
+            f"GRUB_TIMEOUT_STYLE={self.timeout_style}\n"
             "GRUB_DEFAULT=saved\n"
         )
 
@@ -444,6 +465,10 @@ class GrubConfig:
         if self.serial:
             data += f'GRUB_SERIAL_COMMAND="{self.serial}"\n'
 
+        if self.terminal:
+            val = " ".join(self.terminal)
+            data += f'GRUB_TERMINAL="{val}"\n'
+
         if self.terminal_input:
             val = " ".join(self.terminal_input)
             data += f'GRUB_TERMINAL_INPUT="{val}"\n'
@@ -467,11 +492,15 @@ def main(tree, options):
     cfg = options.get("config", {})
     config = GrubConfig(bios, root_fs, boot_fs)
     config.cmdline = cfg.get("cmdline", "")
+    config.disable_recovery = cfg.get("disable_recovery", True)
+    config.disable_submenu = cfg.get("disable_submenu", True)
     config.distributor = cfg.get("distributor")
     config.serial = cfg.get("serial")
+    config.terminal = cfg.get("terminal")
     config.terminal_input = cfg.get("terminal_input")
     config.terminal_output = cfg.get("terminal_output")
     config.timeout = cfg.get("timeout", 0)
+    config.timeout_style = cfg.get("timeout_style", "countdown")
     config.entries = [GrubEntry.from_json(e) for e in options["entries"]]
 
     # Create the configuration file that determines how grub.cfg is generated.
diff --git a/tools/check-snapshots b/tools/check-snapshots
index 969b1762..2b5fbe3b 100755
--- a/tools/check-snapshots
+++ b/tools/check-snapshots
@@ -23,7 +23,8 @@ def fetch_snapshots_api(url, timeout=SNAPSHOTS_TIMEOUT):
     start = time.time()
     try:
         r = requests.get(url, timeout=timeout)
-    except BaseException:
+    except BaseException as e:  # pylint: disable=broad-exception-caught
+        print(f"Problem downloading {url}: {e}")
         return None
     elapsed = time.time() - start
     if r.status_code != 200:
@@ -90,7 +91,8 @@ def check_baseurl(repo, snapshots):
     return invalid, newer
 
 
-def check_snapshot_urls(urls, snapshots, skip=["test/data/assemblers", "test/data/manifests", "test/data/stages"],
+# pylint: disable=too-many-branches
+def check_snapshot_urls(urls, snapshots, skip=("test/data/assemblers", "test/data/manifests", "test/data/stages"),
                         errors_only=False):
     """check the urls against the current list of snapshots
 
@@ -133,11 +135,11 @@ def check_snapshot_urls(urls, snapshots, skip=["test/data/assemblers", "test/dat
                     messages[f] = {"newer": [newer], "invalid": []}
 
     # Print the messages for each file
-    for f in messages:
+    for f, msgs in messages.items():
         print(f"{f}:")
-        for msg in messages[f]["invalid"]:
+        for msg in msgs["invalid"]:
             print(f"    ERROR: {msg}")
-        for msg in messages[f]["newer"]:
+        for msg in msgs["newer"]:
             print(f"    NEWER: {msg}")
 
     return ret
@@ -167,7 +169,7 @@ def main():
         try:
             with open(args.cache, encoding="utf8") as f:
                 snapshots = json.load(f)
-        except BaseException:
+        except FileNotFoundError:
             print(f"No snapshots cache found at {args.cache}")
             sys.exit(1)
     else:
diff --git a/tools/osbuild-depsolve-dnf5 b/tools/osbuild-depsolve-dnf5
index 0246aca5..903adfc1 100755
--- a/tools/osbuild-depsolve-dnf5
+++ b/tools/osbuild-depsolve-dnf5
@@ -11,6 +11,7 @@ import json
 import os
 import sys
 import tempfile
+import traceback
 from datetime import datetime
 
 import libdnf5 as dnf5
@@ -20,7 +21,7 @@ from libdnf5.common import QueryCmp_EQ as EQ
 from libdnf5.common import QueryCmp_GLOB as GLOB
 
 
-# XXX - Temporarily lifted from dnf.rpm module
+# XXX - Temporarily lifted from dnf.rpm module  # pylint: disable=fixme
 def _invert(dct):
     return {v: k for k in dct for v in dct[k]}
 
@@ -140,7 +141,7 @@ class Solver():
 
             self.base.get_repo_sack().update_and_load_enabled_repos(load_system=False)
         except RuntimeError as e:
-            raise RepoError(e)
+            raise RepoError(e) from e
 
     # pylint: disable=too-many-branches
     def _dnfrepo(self, desc, exclude_pkgs=None):
@@ -420,8 +421,7 @@ def solve(request, cache_dir):
                 "kind": "RepoError",
                 "reason": f"There was a problem reading a repository: {e}"
             }
-        except Exception as e:
-            import traceback
+        except Exception as e:  # pylint: disable=broad-exception-caught
             printe("error traceback")
             return None, {
                 "kind": type(e).__name__,
diff --git a/tools/osbuild-mpp b/tools/osbuild-mpp
index 5eab453e..56513038 100755
--- a/tools/osbuild-mpp
+++ b/tools/osbuild-mpp
@@ -692,7 +692,7 @@ class DepSolver:
             if not result.scheme:
                 path = basedir.joinpath(baseurl)
                 return path.resolve().as_uri()
-        except BaseException:  # pylint: disable=bare-except
+        except BaseException:  # pylint: disable=broad-exception-caught
             pass
 
         return baseurl
@@ -1008,7 +1008,7 @@ class Image:
                 try:
                     table.write_to(loopimage)
                 finally:
-                    subprocess.run(["losetup", "-d", loopimage])
+                    subprocess.run(["losetup", "-d", loopimage], check=True)
 
         return cls(size, table)
 
@@ -1215,7 +1215,7 @@ class ManifestFile:
             pipeline_name = self.get_pipeline_name(parent_node)
             self._process_stage(node, pipeline_name)
 
-    # pylint: disable=too-many-branches
+    # pylint: disable=too-many-branches,too-many-statements
     def _process_format(self, node):
         def _is_format(node):
             if not isinstance(node, dict):
@@ -1245,7 +1245,7 @@ class ManifestFile:
                 res = False
                 try:
                     res = eval(code, dict(local_vars))
-                except Exception as e:
+                except Exception as e:  # pylint: disable=broad-exception-caught
                     print(f"In {self.path}: Failed to evaluate mpp-if of:\n  {code}")
                     print(f"Error: {e}")
                     sys.exit(1)
@@ -1266,7 +1266,7 @@ class ManifestFile:
                 # Note, we copy local_vars here to avoid eval modifying it
                 try:
                     res = eval(code, dict(local_vars))
-                except Exception as e:
+                except Exception as e:  # pylint: disable=broad-exception-caught
                     print(f"In {self.path}: Failed to mpp-eval:\n  {code}")
                     print(f"Error: {e}")
                     sys.exit(1)
@@ -1286,7 +1286,7 @@ class ManifestFile:
             # Note, we copy local_vars here to avoid eval modifying it
             try:
                 res = eval(f'f\'\'\'{format_string}\'\'\'', dict(local_vars))
-            except Exception as e:
+            except Exception as e:  # pylint: disable=broad-exception-caught
                 print(f"In {self.path}: Failed to format string:\n  {format_string}")
                 print(f"Error: {e}")
                 sys.exit(1)
@@ -1373,7 +1373,7 @@ class ManifestFile:
     def _process_container(self, _stage):
         raise NotImplementedError()
 
-    def _process_ostree(self, _stage):
+    def _process_ostree_commits(self, _stage):
         raise NotImplementedError()
 
 
@@ -1788,7 +1788,7 @@ def main():
     args = parser.parse_args(sys.argv[1:])
 
     defaults = {
-        "arch": rpm.expandMacro("%{_arch}")
+        "arch": rpm.expandMacro("%{_arch}")  # pylint: disable=no-member
     }
 
     # Override variables from the main of imported files
diff --git a/tox.ini b/tox.ini
index 1067472f..731d042f 100644
--- a/tox.ini
+++ b/tox.ini
@@ -22,9 +22,10 @@ deps =
     pykickstart
     # required by pykickstart but not pulled in automatically :/
     requests
+    tox-backticks
 
 setenv =
-    LINTABLES = osbuild/ assemblers/* devices/* inputs/* mounts/* runners/* sources/* stages/*.* stages/test/*.py test/ tools/
+    LINTABLES = osbuild/ assemblers/* devices/* inputs/* mounts/* runners/* sources/* stages/*.* stages/test/*.py test/ `find ./tools ! -name "*.sh" -type f`
     TYPEABLES = osbuild
     TYPEABLES_STRICT = ./osbuild/main_cli.py
 


Test patch:
diff --git a/stages/test/test_grub2.py b/stages/test/test_grub2.py
index f3197baf..91a8d987 100644
--- a/stages/test/test_grub2.py
+++ b/stages/test/test_grub2.py
@@ -1,5 +1,9 @@
 #!/usr/bin/python3
 
+import os.path
+
+import pytest
+
 from osbuild.testutil import make_fake_tree
 
 STAGE_NAME = "org.osbuild.grub2"
@@ -26,3 +30,80 @@ def test_grub2_copy_efi_data(tmp_path, stage_module):
     stage_module.main(fake_tree, test_options)
     assert (fake_tree / "boot/efi/EFI/fedora/a.shim").exists()
     assert (fake_tree / "boot/efi/EFI/BOOT/BOOTX64.EFI").exists()
+
+
+# Test that the /etc/default/grub file is created with the correct content
+@pytest.mark.parametrize("test_data,kernel_opts,expected_conf", [
+    # default
+    ({}, "", """GRUB_CMDLINE_LINUX=""
+GRUB_TIMEOUT=0
+GRUB_ENABLE_BLSCFG=true
+"""),
+    # custom
+    ({
+        "default": "0",
+        "timeout": 10,
+        "terminal_input": ["console"],
+        "terminal_output": ["serial"],
+        "serial": "serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1",
+    }, "", """GRUB_CMDLINE_LINUX=""
+GRUB_TIMEOUT=10
+GRUB_ENABLE_BLSCFG=true
+GRUB_SERIAL_COMMAND="serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1"
+GRUB_TERMINAL_INPUT="console"
+GRUB_TERMINAL_OUTPUT="serial"
+GRUB_DEFAULT=0
+"""),
+    # custom (Azure)
+    ({
+        "default": "saved",
+        "disable_submenu": True,
+        "disable_recovery": True,
+        "distributor": "$(sed 's, release .*$,,g' /etc/system-release)",
+        "serial": "serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1",
+        "terminal": ["serial", "console"],
+        "terminal_output": ["console"],
+        "timeout": 10,
+        "timeout_style": "countdown",
+    },
+        "loglevel=3 crashkernel=auto console=tty1 console=ttyS0 earlyprintk=ttyS0 rootdelay=300",
+        """GRUB_CMDLINE_LINUX="loglevel=3 crashkernel=auto console=tty1 console=ttyS0 earlyprintk=ttyS0 rootdelay=300"
+GRUB_TIMEOUT=10
+GRUB_ENABLE_BLSCFG=true
+GRUB_DISABLE_RECOVERY=true
+GRUB_DISABLE_SUBMENU=true
+GRUB_DISTRIBUTOR="$(sed 's, release .*$,,g' /etc/system-release)"
+GRUB_SERIAL_COMMAND="serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1"
+GRUB_TERMINAL="serial console"
+GRUB_TERMINAL_OUTPUT="console"
+GRUB_TIMEOUT_STYLE=countdown
+GRUB_DEFAULT=saved
+"""),
+])
+def test_grub2_default_conf(tmp_path, stage_module, test_data, kernel_opts, expected_conf):
+    treedir = tmp_path / "tree"
+    confpath = treedir / "etc/default/grub"
+    confpath.parent.mkdir(parents=True, exist_ok=True)
+
+    options = {
+        "rootfs": {
+            "label": "root"
+        },
+        "entries": [
+            {
+                "id": "fff",
+                "kernel": "4.18",
+                "product": {
+                    "name": "Fedora",
+                    "version": "40"
+                }
+            }
+        ]
+    }
+
+    options["config"] = test_data
+    options["kernel_opts"] = kernel_opts
+    stage_module.main(treedir, options)
+
+    assert os.path.exists(confpath)
+    assert confpath.read_text() == expected_conf
diff --git a/stages/test/test_grub2_legacy.py b/stages/test/test_grub2_legacy.py
new file mode 100644
index 00000000..5e0b346c
--- /dev/null
+++ b/stages/test/test_grub2_legacy.py
@@ -0,0 +1,81 @@
+#!/usr/bin/python3
+
+import os.path
+
+import pytest
+
+STAGE_NAME = "org.osbuild.grub2.legacy"
+
+
+# Test that the /etc/default/grub file is created with the correct content
+@pytest.mark.parametrize("test_data,expected_conf", [
+    # default
+    ({}, """GRUB_TIMEOUT=0
+GRUB_CMDLINE_LINUX=""
+GRUB_DISABLE_SUBMENU=true
+GRUB_DISABLE_RECOVERY=true
+GRUB_TIMEOUT_STYLE=countdown
+GRUB_DEFAULT=saved
+"""),
+    # custom
+    ({
+        "disable_submenu": False,
+        "disable_recovery": False,
+        "timeout": 10,
+        "timeout_style": "hidden",
+    }, """GRUB_TIMEOUT=10
+GRUB_CMDLINE_LINUX=""
+GRUB_DISABLE_SUBMENU=false
+GRUB_DISABLE_RECOVERY=false
+GRUB_TIMEOUT_STYLE=hidden
+GRUB_DEFAULT=saved
+"""),
+    # custom (Azure)
+    ({
+        "cmdline": "loglevel=3 crashkernel=auto console=tty1 console=ttyS0 earlyprintk=ttyS0 rootdelay=300",
+        "disable_submenu": True,
+        "disable_recovery": True,
+        "distributor": "$(sed 's, release .*$,,g' /etc/system-release)",
+        "serial": "serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1",
+        "terminal": ["serial", "console"],
+        "terminal_output": ["console"],
+        "timeout": 10,
+        "timeout_style": "countdown",
+    }, """GRUB_TIMEOUT=10
+GRUB_CMDLINE_LINUX="loglevel=3 crashkernel=auto console=tty1 console=ttyS0 earlyprintk=ttyS0 rootdelay=300"
+GRUB_DISABLE_SUBMENU=true
+GRUB_DISABLE_RECOVERY=true
+GRUB_TIMEOUT_STYLE=countdown
+GRUB_DEFAULT=saved
+GRUB_DISTRIBUTOR="$(sed 's, release .*$,,g' /etc/system-release)"
+GRUB_SERIAL_COMMAND="serial --speed=115200 --unit=0 --word=8 --parity=no --stop=1"
+GRUB_TERMINAL="serial console"
+GRUB_TERMINAL_OUTPUT="console"
+"""),
+])
+def test_grub2_default_conf(tmp_path, stage_module, test_data, expected_conf):
+    treedir = tmp_path / "tree"
+    confpath = treedir / "etc/default/grub"
+    confpath.parent.mkdir(parents=True, exist_ok=True)
+
+    options = {
+        "rootfs": {
+            "label": "root"
+        },
+        "entries": [
+            {
+                "id": "fff",
+                "kernel": "4.18",
+                "product": {
+                    "name": "Fedora",
+                    "version": "40"
+                }
+            }
+        ]
+    }
+
+    options["config"] = test_data
+    stage_module.main(treedir, options)
+
+    assert os.path.exists(confpath)
+    assert confpath.read_text() == expected_conf
diff --git a/tools/gen-stage-test-diff b/tools/gen-stage-test-diff
index fb50eaf1..c302b5e7 100755
--- a/tools/gen-stage-test-diff
+++ b/tools/gen-stage-test-diff
@@ -40,7 +40,7 @@ def run_osbuild(output_directory, store, cache_max_size, libdir, manifest):
     except subprocess.CalledProcessError as err:
         raise RuntimeError(
             f"osbuild crashed when building {manifest}:\n\nstdout:\n{err.stdout}\n\nstderr:\n{err.stderr}"
-        )
+        ) from err
 
 
 epilog = """