Repository: osbuild/osbuild
Pull request #1476 (2023-11-28 08:20:58)

osbuild: add "mypy-strict" check
We have mypy right now which is super nice. However there are quite a few place where it's not used fully right now and where we lack type information. This PR start by adding a new `mypy-strict` tox filter that gradually builds up more and more files to be fully strict. I already see this uncovering potentially real issues in other files that should probably be addressed. But with this simple and humble start we can start addressing more and more of them.

supakeen: Do we plan to fix all the signatures to make `mypy-strict` pass here or do we want to mark it as non-required/expected to fail?
mvo5: > Do we plan to fix all the signatures to make `mypy-strict` pass here or do we want to mark it as non-required/expected to fail?

There are many missing/incomplete signatures today, running this over osbuild counts 594 errors. So it will take some time to do them all. The approach here allows to add one file at a time (and ensure we do not regress on the ones we have), maybe add one each time a bug is discovered or when a new change is needed? But yeah, *eventually* I would love to be fully covered. I also find it a nice task to get to know the code flows a bit better but that is probably just me :)

But maybe it requires a discussion in the team first? I see value in this but can see contra arguments of diminishing returns.
supakeen: I'm in favor. We've had these discussions previously where there was no clear benefit found but I'm happy to revisit and do this again, you can take a look at my previous attempt here: https://github.com/osbuild/osbuild/pull/1089/commits/ce612f423a073b90c66a4289b41955bfc860e16e
mvo5: > I'm in favor. We've had these discussions previously where there was no clear benefit found but I'm happy to revisit and do this again, you can take a look at my previous attempt here: [ce612f4](https://github.com/osbuild/osbuild/commit/ce612f423a073b90c66a4289b41955bfc860e16e)

Sorry for naging, what does it take to get this one in? Should I cherry pick your existing commit as part of this? Do it as a followup? 

Note that I'm fine either way. If it's considered not very useful I can close the PR.
github-actions[bot]: This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

Patch:
diff --git a/Makefile b/Makefile
index 2a8b9d87..0115300a 100644
--- a/Makefile
+++ b/Makefile
@@ -248,7 +248,7 @@ test-all:
 
 .PHONY: lint
 lint:
-	tox run-parallel -e ruff,pylint,autopep8,isort,mypy
+	tox run-parallel -e ruff,pylint,autopep8,isort,mypy,mypy-strict
 
 #
 # Building packages
diff --git a/osbuild/main_cli.py b/osbuild/main_cli.py
index 19a72579..172afe8d 100644
--- a/osbuild/main_cli.py
+++ b/osbuild/main_cli.py
@@ -10,16 +10,20 @@ import argparse
 import json
 import os
 import sys
+import typing
+from typing import List
 
 import osbuild
 import osbuild.meta
 import osbuild.monitor
+from osbuild.meta import ValidationResult
 from osbuild.objectstore import ObjectStore
+from osbuild.pipeline import Manifest
 from osbuild.util.parsing import parse_size
 from osbuild.util.term import fmt as vt
 
 
-def parse_manifest(path):
+def parse_manifest(path: str) -> dict:
     if path == "-":
         manifest = json.load(sys.stdin)
     else:
@@ -29,7 +33,7 @@ def parse_manifest(path):
     return manifest
 
 
-def show_validation(result, name):
+def show_validation(result: ValidationResult, name: str) -> None:
     if name == "-":
         name = "<stdin>"
 
@@ -47,7 +51,7 @@ def show_validation(result, name):
         print(f"  {error.message}\n")
 
 
-def export(name_or_id, output_directory, store, manifest):
+def export(name_or_id: str, output_directory: str, store: ObjectStore, manifest: Manifest) -> None:
     pipeline = manifest[name_or_id]
     obj = store.get(pipeline.id)
     dest = os.path.join(output_directory, name_or_id)
@@ -58,7 +62,8 @@ def export(name_or_id, output_directory, store, manifest):
     obj.export(dest, skip_preserve_owner=skip_preserve_owner)
 
 
-def parse_arguments(sys_argv):
+@typing.no_type_check  # see https://github.com/python/typeshed/issues/3107
+def parse_arguments(sys_argv: List[str]) -> argparse.Namespace:
     parser = argparse.ArgumentParser(prog="osbuild",
                                      description="Build operating system images")
 
@@ -103,7 +108,7 @@ def parse_arguments(sys_argv):
 
 
 # pylint: disable=too-many-branches,too-many-return-statements,too-many-statements
-def osbuild_cli():
+def osbuild_cli() -> int:
     args = parse_arguments(sys.argv)
     desc = parse_manifest(args.manifest_path)
 
diff --git a/stages/org.osbuild.mkfs.ext4 b/stages/org.osbuild.mkfs.ext4
index d88ae23a..697691ad 100755
--- a/stages/org.osbuild.mkfs.ext4
+++ b/stages/org.osbuild.mkfs.ext4
@@ -38,6 +38,14 @@ SCHEMA_2 = r"""
       "type": "string",
       "maxLength": 16
     },
+    "metadata_csum_seed": {
+      "description": "Enable metadata_csum_seed support",
+      "type": "boolean"
+    },
+    "orphan_file": {
+      "description": "Enable orphan_file support",
+      "type": "boolean"
+    },
     "verity": {
       "description": "Enable fs-verity support",
       "type": "boolean"
@@ -52,17 +60,18 @@ def main(devices, options):
 
     uuid = options["uuid"]
     label = options.get("label")
-    verity = options.get("verity")
     opts = []
 
     if label:
         opts = ["-L", label]
 
-    if verity is not None:
-        if verity:
-            opts += ["-O", "verity"]
-        else:
-            opts += ["-O", "^verity"]
+    for fsopt in ["verity", "orphan_file", "metadata_csum_seed"]:
+        val = options.get(fsopt)
+        if val is not None:
+            if val:
+                opts += ["-O", fsopt]
+            else:
+                opts += ["-O", f"^{fsopt}"]
 
     subprocess.run(["mkfs.ext4", "-U", uuid] + opts + [device],
                    encoding='utf8', check=True)
diff --git a/tox.ini b/tox.ini
index 5f30d50c..1067472f 100644
--- a/tox.ini
+++ b/tox.ini
@@ -7,7 +7,7 @@ env_list =
 labels =
     test = py{36,37,38,39,310,311}
     lint = ruff, isort, autopep8, pylint
-    type = mypy
+    type = mypy,mypy-strict
 
 [testenv]
 description = "run osbuild unit tests"
@@ -26,6 +26,7 @@ deps =
 setenv =
     LINTABLES = osbuild/ assemblers/* devices/* inputs/* mounts/* runners/* sources/* stages/*.* stages/test/*.py test/ tools/
     TYPEABLES = osbuild
+    TYPEABLES_STRICT = ./osbuild/main_cli.py
 
 passenv =
     TEST_CATEGORY
@@ -71,3 +72,10 @@ deps =
 
 commands =
     bash -c 'python -m mypy {env:TYPEABLES}'
+
+[testenv:mypy-strict]
+deps =
+    mypy==1.2.0
+
+commands =
+    bash -c 'python -m mypy --follow-imports=skip --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs {env:TYPEABLES_STRICT}'


Test patch:
diff --git a/osbuild/testutil/__init__.py b/osbuild/testutil/__init__.py
index 5b47219f..3d9e2bc0 100644
--- a/osbuild/testutil/__init__.py
+++ b/osbuild/testutil/__init__.py
@@ -3,6 +3,7 @@ Test related utilities
 """
 import os
 import pathlib
+import re
 import shutil
 
 
@@ -39,3 +40,20 @@ def make_fake_input_tree(tmpdir: pathlib.Path, fake_content: dict) -> str:
     basedir = tmpdir / "tree"
     make_fake_tree(basedir, fake_content)
     return os.fspath(basedir)
+
+
+def assert_jsonschema_error_contains(res, expected_err, expected_num_errs=None):
+    err_msgs = [e.as_dict()["message"] for e in res.errors]
+    if expected_num_errs is not None:
+        assert len(err_msgs) == expected_num_errs, \
+            f"expected exactly {expected_num_errs} errors in {[e.as_dict() for e in res.errors]}"
+    re_typ = getattr(re, 'Pattern', None)
+    # this can be removed once we no longer support py3.6 (re.Pattern is modern)
+    if not re_typ:
+        re_typ = getattr(re, '_pattern_type')
+    if isinstance(expected_err, re_typ):
+        finder = expected_err.search
+    else:
+        def finder(s): return expected_err in s  # pylint: disable=C0321
+    assert any(finder(err_msg)
+               for err_msg in err_msgs), f"{expected_err} not found in {err_msgs}"
diff --git a/stages/test/test_autotailor.py b/stages/test/test_autotailor.py
index c321391a..a42d0282 100644
--- a/stages/test/test_autotailor.py
+++ b/stages/test/test_autotailor.py
@@ -7,6 +7,7 @@ from unittest.mock import call, patch
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 from osbuild.testutil.imports import import_module_from_path
 
 TEST_INPUT = [
@@ -112,6 +113,4 @@ def test_schema_validation_oscap_autotailor(fake_input, test_data, expected_err)
     res = schema_validate_stage_oscap_autotailor(fake_input, test_data)
 
     assert res.valid is False
-    err_msgs = [e.as_dict()["message"] for e in res.errors]
-    assert len(res.errors) == 1, err_msgs
-    assert expected_err in err_msgs[0]
+    testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
diff --git a/stages/test/test_bootupd.py b/stages/test/test_bootupd.py
index 9df845d3..f9d3bfd3 100644
--- a/stages/test/test_bootupd.py
+++ b/stages/test/test_bootupd.py
@@ -6,6 +6,7 @@ from unittest.mock import call, patch
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 from osbuild.testutil.imports import import_module_from_path
 
 
@@ -54,9 +55,7 @@ def test_bootupd_schema_validation(test_data, expected_err):
         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, [e.as_dict() for e in res.errors]
-        err_msgs = [e.as_dict()["message"] for e in res.errors]
-        assert expected_err in err_msgs[0]
+        testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
 @pytest.mark.parametrize("test_options,expected_bootupd_opts", [
diff --git a/stages/test/test_containers_storage_conf.py b/stages/test/test_containers_storage_conf.py
index 7fcf030a..36f322c6 100644
--- a/stages/test/test_containers_storage_conf.py
+++ b/stages/test/test_containers_storage_conf.py
@@ -11,7 +11,7 @@ except ModuleNotFoundError:
     import pytoml as toml
 
 import osbuild.meta
-from osbuild.testutil import assert_dict_has
+from osbuild import testutil
 from osbuild.testutil.imports import import_module_from_path
 
 TEST_INPUT = [
@@ -56,7 +56,7 @@ def test_containers_storage_conf_integration(tmp_path, test_filename, test_stora
     assert conf is not None
 
     for (key, value) in expected:
-        assert_dict_has(conf, key, value)
+        testutil.assert_dict_has(conf, key, value)
 
 
 @pytest.mark.parametrize(
@@ -64,7 +64,7 @@ def test_containers_storage_conf_integration(tmp_path, test_filename, test_stora
     [
         # None, note that starting from jsonschema 4.21.0 the error changes
         # so we need a regexp here
-        ({}, {}, r"does not have enough properties|should be non-empty"),
+        ({}, {}, re.compile("does not have enough properties|should be non-empty")),
         # All options
         ({
             "filename": "/etc/containers/storage.conf",
@@ -120,6 +120,4 @@ def test_schema_validation_containers_storage_conf(test_data, storage_test_data,
         assert res.valid is True, f"err: {[e.as_dict() for e in res.errors]}"
     else:
         assert res.valid is False
-        err_msgs = [e.as_dict()["message"] for e in res.errors]
-        assert any(re.search(expected_err, err_msg)
-                   for err_msg in err_msgs), f"{expected_err} not found in {err_msgs}"
+        testutil.assert_jsonschema_error_contains(res, expected_err)
diff --git a/stages/test/test_erofs.py b/stages/test/test_erofs.py
index bcda4313..45ddfd2f 100644
--- a/stages/test/test_erofs.py
+++ b/stages/test/test_erofs.py
@@ -7,6 +7,7 @@ from unittest import mock
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 from osbuild.testutil import has_executable, make_fake_input_tree
 from osbuild.testutil.imports import import_module_from_path
 
@@ -109,6 +110,4 @@ def test_schema_validation_erofs(test_data, expected_err):
         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, [e.as_dict() for e in res.errors]
-        err_msgs = [e.as_dict()["message"] for e in res.errors]
-        assert expected_err in err_msgs[0]
+        testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
diff --git a/stages/test/test_kickstart.py b/stages/test/test_kickstart.py
index d2ca7340..4a7957e9 100644
--- a/stages/test/test_kickstart.py
+++ b/stages/test/test_kickstart.py
@@ -6,6 +6,7 @@ import subprocess
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 from osbuild.testutil import has_executable
 from osbuild.testutil.imports import import_module_from_path
 
@@ -365,6 +366,4 @@ def test_schema_validation_bad_apples(test_data, expected_err):
     res = schema_validate_kickstart_stage(test_data)
 
     assert res.valid is False
-    assert len(res.errors) == 1
-    err_msgs = [e.as_dict()["message"] for e in res.errors]
-    assert expected_err in err_msgs[0]
+    testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
diff --git a/stages/test/test_machine-id.py b/stages/test/test_machine-id.py
index efe98a75..b5ad2bda 100644
--- a/stages/test/test_machine-id.py
+++ b/stages/test/test_machine-id.py
@@ -7,6 +7,7 @@ import unittest.mock
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 from osbuild.testutil.imports import import_module_from_path
 
 
@@ -72,9 +73,7 @@ def test_machine_id_schema_validation(test_data, expected_err):
     res = schema.validate(test_input)
 
     assert res.valid is False
-    assert len(res.errors) == 1
-    err_msgs = [e.as_dict()["message"] for e in res.errors]
-    assert expected_err in err_msgs[0]
+    testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
 def test_machine_id_first_boot_unknown(tmp_path):
diff --git a/stages/test/test_mkfs_ext4.py b/stages/test/test_mkfs_ext4.py
index 0c0ea2f0..bb3433e4 100644
--- a/stages/test/test_mkfs_ext4.py
+++ b/stages/test/test_mkfs_ext4.py
@@ -8,6 +8,7 @@ from unittest import mock
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 from osbuild.testutil import has_executable
 from osbuild.testutil.imports import import_module_from_path
 
@@ -48,9 +49,7 @@ def test_schema_validation_mkfs_ext4(test_data, expected_err):
         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, [e.as_dict() for e in res.errors]
-        err_msgs = [e.as_dict()["message"] for e in res.errors]
-        assert expected_err in err_msgs[0]
+        testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
 @pytest.mark.skipif(not has_executable("mkfs.ext4"), reason="need mkfs.ext4")
@@ -82,6 +81,11 @@ def test_mkfs_ext4_integration(tmp_path):
     ({}, []),
     ({"verity": True}, ["-O", "verity"]),
     ({"verity": False}, ["-O", "^verity"]),
+    ({"orphan_file": True}, ["-O", "orphan_file"]),
+    ({"orphan_file": False}, ["-O", "^orphan_file"]),
+    ({"metadata_csum_seed": True}, ["-O", "metadata_csum_seed"]),
+    ({"metadata_csum_seed": False}, ["-O", "^metadata_csum_seed"]),
+    ({"verity": True, "orphan_file": True, "metadata_csum_seed": True}, ["-O", "verity", "-O", "orphan_file", "-O", "metadata_csum_seed"]),
 ])
 @mock.patch("subprocess.run")
 def test_mkfs_ext4_cmdline(mock_run, test_input, expected):
diff --git a/stages/test/test_ostree_post_copy.py b/stages/test/test_ostree_post_copy.py
index 2c349d02..8e7a1fbc 100644
--- a/stages/test/test_ostree_post_copy.py
+++ b/stages/test/test_ostree_post_copy.py
@@ -6,6 +6,7 @@ from unittest.mock import call, patch
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 from osbuild.testutil.imports import import_module_from_path
 
 
@@ -56,6 +57,4 @@ def test_schema_validation_ostree_post_copy(test_data, expected_err):
     res = schema_validate_stage_ostree_post_copy(test_data)
 
     assert res.valid is False
-    err_msgs = [e.as_dict()["message"] for e in res.errors]
-    assert len(res.errors) == 1, err_msgs
-    assert expected_err in err_msgs[0]
+    testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
diff --git a/stages/test/test_selinux.py b/stages/test/test_selinux.py
index 507d2e99..81360ce0 100644
--- a/stages/test/test_selinux.py
+++ b/stages/test/test_selinux.py
@@ -6,7 +6,7 @@ from unittest.mock import call, patch
 import pytest
 
 import osbuild.meta
-import osbuild.testutil
+from osbuild import testutil
 from osbuild.testutil.imports import import_module_from_path
 
 
@@ -42,18 +42,15 @@ def test_schema_validation_selinux(test_data, expected_err):
         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, [e.as_dict() for e in res.errors]
-        err_msgs = [e.as_dict()["message"] for e in res.errors]
-        assert expected_err in err_msgs[0]
+        testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
 def test_schema_validation_selinux_file_context_required():
     test_data = {}
     res = schema_validation_selinux(test_data, implicit_file_contexts=False)
     assert res.valid is False
-    assert len(res.errors) == 1, [e.as_dict() for e in res.errors]
-    err_msgs = [e.as_dict()["message"] for e in res.errors]
-    assert "'file_contexts' is a required property" in err_msgs[0]
+    expected_err = "'file_contexts' is a required property"
+    testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
 @patch("osbuild.util.selinux.setfiles")
diff --git a/stages/test/test_skopeo.py b/stages/test/test_skopeo.py
index 6e84ef62..907bba83 100644
--- a/stages/test/test_skopeo.py
+++ b/stages/test/test_skopeo.py
@@ -5,6 +5,7 @@ import os.path
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 
 
 @pytest.mark.parametrize("test_data,expected_err", [
@@ -37,6 +38,4 @@ def test_schema_validation_skopeo(test_data, expected_err):
         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, [e.as_dict() for e in res.errors]
-        err_msgs = [e.as_dict()["message"] for e in res.errors]
-        assert expected_err in err_msgs[0]
+        testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
diff --git a/stages/test/test_xz.py b/stages/test/test_xz.py
index c69a9c15..79bbf74b 100644
--- a/stages/test/test_xz.py
+++ b/stages/test/test_xz.py
@@ -7,6 +7,7 @@ from unittest import mock
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 from osbuild.testutil import has_executable, make_fake_input_tree
 from osbuild.testutil.imports import import_module_from_path
 
@@ -35,9 +36,7 @@ def test_schema_validation_xz(test_data, expected_err):
         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, [e.as_dict() for e in res.errors]
-        err_msgs = [e.as_dict()["message"] for e in res.errors]
-        assert expected_err in err_msgs[0]
+        testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
 @pytest.fixture(name="fake_input_tree")
diff --git a/stages/test/test_zstd.py b/stages/test/test_zstd.py
index 665601e9..ec702cd9 100644
--- a/stages/test/test_zstd.py
+++ b/stages/test/test_zstd.py
@@ -7,6 +7,7 @@ from unittest import mock
 import pytest
 
 import osbuild.meta
+from osbuild import testutil
 from osbuild.testutil import has_executable, make_fake_input_tree
 from osbuild.testutil.imports import import_module_from_path
 
@@ -35,9 +36,7 @@ def test_schema_validation_zstd(test_data, expected_err):
         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, [e.as_dict() for e in res.errors]
-        err_msgs = [e.as_dict()["message"] for e in res.errors]
-        assert expected_err in err_msgs[0]
+        testutil.assert_jsonschema_error_contains(res, expected_err, expected_num_errs=1)
 
 
 @pytest.fixture(name="fake_input_tree")
diff --git a/test/mod/test_testutil_jsonschema.py b/test/mod/test_testutil_jsonschema.py
new file mode 100644
index 00000000..53bc8104
--- /dev/null
+++ b/test/mod/test_testutil_jsonschema.py
@@ -0,0 +1,51 @@
+import re
+
+import pytest
+
+import osbuild.meta
+from osbuild import testutil
+
+fake_schema = {
+    "type": "object",
+    "required": ["name"],
+}
+
+
+@pytest.fixture(name="validation_error")
+def validation_error_fixture():
+    schema = osbuild.meta.Schema(fake_schema, "fake-schema")
+    res = schema.validate({"not": "name"})
+    assert res.valid is False
+    return res
+
+
+def test_assert_jsonschema_error_contains(validation_error):
+    expected_err = "'name' is a required property"
+    testutil.assert_jsonschema_error_contains(validation_error, expected_err)
+
+
+def test_assert_jsonschema_error_regex(validation_error):
+    expected_err = re.compile("'.*' is a required property")
+    testutil.assert_jsonschema_error_contains(validation_error, expected_err)
+
+
+def test_assert_jsonschema_error_not_contains(validation_error):
+    with pytest.raises(AssertionError, match=r'not-in-errs not found in \['):
+        testutil.assert_jsonschema_error_contains(validation_error, "not-in-errs")
+
+
+def test_assert_jsonschema_error_not_found_re(validation_error):
+    expected_err_re = re.compile("not-in-errs")
+    with pytest.raises(AssertionError, match=r"re.*not found in"):
+        testutil.assert_jsonschema_error_contains(validation_error, expected_err_re)
+
+
+def test_assert_jsonschema_error_num_errs(validation_error):
+    expected_err = "'name' is a required property"
+    testutil.assert_jsonschema_error_contains(validation_error, expected_err, expected_num_errs=1)
+
+
+def test_assert_jsonschema_error_num_errs_wrong(validation_error):
+    expected_err = "'name' is a required property"
+    with pytest.raises(AssertionError, match=r'expected exactly 99 errors in'):
+        testutil.assert_jsonschema_error_contains(validation_error, expected_err, expected_num_errs=99)