Add more coverage for the saltfile mixin and ensure passed CLI options take priority

changelog/65358.fixed.md

@@ -0,0 +1 @@
+Ensure CLI options take priority over Saltfile options

salt/utils/parsers.py

@@ -454,6 +454,7 @@ class SaltfileMixIn(metaclass=MixInMeta):
             if value != default:
                 # The user passed an argument, we won't override it with the
                 # one from Saltfile, if any
+                cli_config.pop(option.dest)
                 continue
 
             # We reached this far! Set the Saltfile value on the option
@@ -477,6 +478,7 @@ class SaltfileMixIn(metaclass=MixInMeta):
                 if value != default:
                     # The user passed an argument, we won't override it with
                     # the one from Saltfile, if any
+                    cli_config.pop(option.dest)
                     continue
 
                 setattr(self.options, option.dest, cli_config[option.dest])

tests/pytests/unit/utils/parsers/test_saltfile_mixin.py

@@ -7,6 +7,7 @@ import shutil
 
 import pytest
 
+import salt.exceptions
 import salt.utils.parsers
 from tests.support.helpers import patched_environ
 from tests.support.mock import patch
@@ -52,6 +53,11 @@ class MockSaltfileParser(
             default=None,
             help="Write the output to the specified file.",
         )
+        group.add_option(
+            "--version-arg",
+            action="version",
+            help="Option to test no dest",
+        )
 
 
 @pytest.fixture
@@ -59,14 +65,6 @@ def parser():
     return MockSaltfileParser()
 
 
-# @pytest.fixture
-# def parser():
-#     # Mock this because we don't need it and it causes an error
-#     # if there is more than one test being run in this file
-#     with patch.object(salt.utils.parsers.LogLevelMixIn, "_LogLevelMixIn__setup_logging_routines"):
-#         yield salt.utils.parsers.SaltCallOptionParser()
-
-
 @pytest.fixture
 def saltfile(tmp_path):
     fp = tmp_path / "Saltfile"
@@ -74,42 +72,53 @@ def saltfile(tmp_path):
     return fp
 
 
-@pytest.fixture
-def base_opts():
-    # return ["--local", "test.ping"]
-    return []
-
-
-def test_saltfile_in_environment(parser, saltfile, base_opts):
+def test_saltfile_in_environment(parser, saltfile):
     """
     Test setting the SALT_SALTFILE environment variable
     """
     with patched_environ(SALT_SALTFILE=str(saltfile)):
-        parser.parse_args(base_opts)
+        parser.parse_args([])
         assert parser.options.saltfile == str(saltfile)
 
 
-def test_saltfile_option(parser, saltfile, base_opts):
+def test_saltfile_option(parser, saltfile):
     """
-    Test setting the SALT_SALTFILE environment variable
+    Test setting the saltfile via the CLI
     """
-    parser.parse_args(base_opts + ["--saltfile", str(saltfile)])
+    parser.parse_args(["--saltfile", str(saltfile)])
     assert parser.options.saltfile == str(saltfile)
 
 
-def test_saltfile_cwd(parser, saltfile, base_opts, tmp_path):
+def test_bad_saltfile_option(parser, saltfile, tmp_path):
     """
-    Test setting the SALT_SALTFILE environment variable
+    Test setting a bad saltfile via the CLI
+    """
+    with pytest.raises(SystemExit):
+        parser.parse_args(["--saltfile", str(tmp_path / "fake_dir")])
+
+
+def test_saltfile_cwd(parser, saltfile, tmp_path):
+    """
+    Test using a saltfile in the cwd
     """
     with patch("os.getcwd", return_value=str(tmp_path)) as cwd_mock:
-        parser.parse_args(base_opts)
+        parser.parse_args([])
         assert parser.options.saltfile == str(saltfile)
         cwd_mock.assert_called_once()
 
 
-def test_saltfile_user_home(parser, saltfile, base_opts, tmp_path):
+def test_saltfile_cwd_doesnt_exist(parser, saltfile, tmp_path):
     """
-    Test setting the SALT_SALTFILE environment variable
+    Test using a saltfile in the cwd that doesn't exist
+    """
+    with patch("os.getcwd", return_value=str(tmp_path / "fake_dir")) as cwd_mock:
+        parser.parse_args([])
+        assert parser.options.saltfile is None
+
+
+def test_saltfile_user_home(parser, saltfile, tmp_path):
+    """
+    Test using a saltfile in ~/.salt/
     """
     fake_dir = tmp_path / "fake_dir"
     fake_dir.mkdir()
@@ -119,7 +128,89 @@ def test_saltfile_user_home(parser, saltfile, base_opts, tmp_path):
             salt_subdir.mkdir()
             dest = str(salt_subdir / "Saltfile")
             shutil.copy(str(saltfile), dest)
-            parser.parse_args(base_opts)
+            parser.parse_args([])
             assert parser.options.saltfile == dest
             cwd_mock.assert_called_once()
             eu_mock.assert_called_with("~")
+
+
+def test_bad_saltfile(parser, saltfile):
+    """
+    Test a saltfile with bad configuration
+    """
+    contents = """
+    bad "yaml":
+  - this is: bad yaml
+  -       bad yaml=data:
+    - {"bad": yaml, "data": "yaml"}
+    """
+    saltfile.write_text(contents)
+    # It raises two errors, let's catch them both
+    with pytest.raises(SystemExit):
+        with pytest.raises(salt.exceptions.SaltConfigurationError):
+            parser.parse_args(["--saltfile", str(saltfile)])
+
+
+def test_saltfile_without_prog_name(parser, saltfile):
+    """
+    Test a saltfile with valid yaml but without the program name in it
+    """
+    contents = "good: yaml"
+    saltfile.write_text(contents)
+    # This should just run cleanly
+    parser.parse_args(["--saltfile", str(saltfile)])
+
+
+def test_saltfile(parser, saltfile):
+    """
+    Test a valid saltfile
+    """
+    contents = """
+    __main__.py:
+      log_level: debug
+      output: json
+    """
+    saltfile.write_text(contents)
+    parser.parse_args(["--saltfile", str(saltfile)])
+    print(parser.option_list)
+    assert parser.options.log_level == "debug"
+    assert parser.options.output == "json"
+
+
+def test_saltfile_unusual_option(parser, saltfile):
+    """
+    Test a valid saltfile
+    """
+    contents = """
+    __main__.py:
+      go: birds
+    """
+    saltfile.write_text(contents)
+    parser.parse_args(["--saltfile", str(saltfile)])
+    assert parser.options.go == "birds"
+
+
+def test_saltfile_cli_override(parser, saltfile):
+    """
+    Test a valid saltfile
+    """
+    contents = """
+    __main__.py:
+      log_level: debug
+      output: json
+      output_file: /fake/file
+    """
+    saltfile.write_text(contents)
+    parser.parse_args(
+        [
+            "--saltfile",
+            str(saltfile),
+            "--log-level",
+            "info",
+            "--out-file",
+            "/still/fake/file",
+        ]
+    )
+    assert parser.options.log_level == "info"
+    assert parser.options.output == "json"
+    assert parser.options.output_file == "/still/fake/file"