From: Georges Discry <georges@discry.be>
Date: Fri, 15 Sep 2023 20:01:26 +0000 (+0200)
Subject: Warn when a setting is in the wrong section
X-Git-Tag: v17~3
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=74ba4b160f1fa3bbf4291b2585fe5db6a50becbb;p=thirdparty%2Fmkosi.git

Warn when a setting is in the wrong section

Except for the `[Match]` section, a setting can be set in any of the
valid sections because the actual section is ignored. This fuzziness
works because each setting is used in only one section so there is not
conflict.

Instead of silently parsing such a file, a warning is now displayed with
a hint on how to fix it. In the future, this could become an error.
---

diff --git a/.github/mkosi.conf.d/10-common.conf b/.github/mkosi.conf.d/10-common.conf
index e612e0c44..132accf37 100644
--- a/.github/mkosi.conf.d/10-common.conf
+++ b/.github/mkosi.conf.d/10-common.conf
@@ -1,13 +1,13 @@
 [Output]
 CacheDirectory=mkosi.cache
+
+[Content]
+Autologin=yes
+BiosBootloader=grub
 KernelCommandLine=console=ttyS0
                   systemd.unit=mkosi-check-and-shutdown.service
                   systemd.log_target=console
                   systemd.default_standard_output=journal+console
 
-[Content]
-BiosBootloader=grub
-
 [Host]
-Autologin=yes
 QemuVsock=yes
diff --git a/mkosi/config.py b/mkosi/config.py
index 13c30fca7..daa088dd0 100644
--- a/mkosi/config.py
+++ b/mkosi/config.py
@@ -1976,12 +1976,15 @@ def parse_config(argv: Sequence[str] = ()) -> tuple[MkosiArgs, tuple[MkosiConfig
         if path.exists():
             logging.debug(f"Including configuration file {Path.cwd() / path}")
 
-            for _, k, v in parse_ini(path, only_sections=["Distribution", "Output", "Content", "Validation", "Host", "Preset"]):
+            for section, k, v in parse_ini(path, only_sections=["Distribution", "Output", "Content", "Validation", "Host", "Preset"]):
                 ns = defaults if k.startswith("@") else namespace
 
                 if not (s := settings_lookup_by_name.get(k.removeprefix("@"))):
                     die(f"Unknown setting {k}")
 
+                if section != s.section:
+                    logging.warning(f"Setting {k} should be configured in [{s.section}], not [{section}].")
+
                 setattr(ns, s.dest, s.parse(v, getattr(ns, s.dest, None)))
 
         if extras:
diff --git a/tests/test_config.py b/tests/test_config.py
index 082759c7e..7f4608a27 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -2,6 +2,7 @@
 
 import argparse
 import itertools
+import logging
 import operator
 from pathlib import Path
 from typing import Optional
@@ -148,6 +149,8 @@ def test_parse_config(tmp_path: Path) -> None:
         """\
         [Content]
         Packages=
+
+        [Output]
         ImageId=
         """
     )
@@ -362,6 +365,8 @@ def test_match_imageid(tmp_path: Path, image1: str, image2: str) -> None:
             f"""\
             [Distribution]
             Distribution=fedora
+
+            [Output]
             ImageId={image1}
             """
         )
@@ -442,6 +447,8 @@ def test_match_imageversion(tmp_path: Path, op: str, version: str) -> None:
             """\
             [Distribution]
             Distribution=fedora
+
+            [Output]
             ImageId=testimage
             ImageVersion=123
             """
@@ -523,3 +530,41 @@ def test_package_manager_tree(tmp_path: Path, skel: Optional[Path], pkgmngr: Opt
 
         assert conf.skeleton_trees == skel_expected
         assert conf.package_manager_trees == pkgmngr_expected
+
+
+@pytest.mark.parametrize(
+    "sections,args,warning_count",
+    [
+        (["Output"], [], 0),
+        (["Content"], [], 1),
+        (["Content", "Output"], [], 1),
+        (["Output", "Content"], [], 1),
+        (["Output", "Content", "Distribution"], [], 2),
+        (["Content"], ["--image-id=testimage"], 1),
+    ],
+)
+def test_wrong_section_warning(
+    tmp_path: Path,
+    caplog: pytest.LogCaptureFixture,
+    sections: list[str],
+    args: list[str],
+    warning_count: int,
+) -> None:
+    with chdir(tmp_path):
+        # Create a config with ImageId in the wrong section,
+        # and sometimes in the correct section
+        Path("mkosi.conf").write_text(
+            "\n".join(
+                f"""\
+                [{section}]
+                ImageId=testimage
+                """
+                for section in sections
+            )
+        )
+
+        with caplog.at_level(logging.WARNING):
+            # Parse the config, with --image-id sometimes given on the command line
+            parse_config(args)
+
+        assert len(caplog.records) == warning_count