Fix regression: multiple values for keyword argument 'saltenv' (bsc#1…

…212844) (#590)

* fix passing wrong keyword arguments to cp.cache_file in pkg.installed with sources

* Drop `**kwargs` usage and be explicit about the supported keyword arguments.

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

* Add regression test for saltstack/salt#64118

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

* Add changelog file

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

---------

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>
Co-authored-by: Massimiliano Torromeo <massimiliano.torromeo@gmail.com>
Co-authored-by: Pedro Algarvio <palgarvio@vmware.com>

changelog/64118.fixed.md

@@ -0,0 +1 @@
+Stop passing `**kwargs` and be explicit about the keyword arguments to pass, namely, to `cp.cache_file` call in `salt.states.pkg`

salt/modules/win_pkg.py

@@ -1298,7 +1298,7 @@ def _failed_compile(prefix_msg, error_msg):
         successful_verbose[short_path_name] = []
 
 
-def _get_source_sum(source_hash, file_path, saltenv, **kwargs):
+def _get_source_sum(source_hash, file_path, saltenv, verify_ssl=True):
     """
     Extract the hash sum, whether it is in a remote hash file, or just a string.
     """
@@ -1315,7 +1315,7 @@ def _get_source_sum(source_hash, file_path, saltenv, **kwargs):
         # The source_hash is a file on a server
         try:
             cached_hash_file = __salt__["cp.cache_file"](
-                source_hash, saltenv, verify_ssl=kwargs.get("verify_ssl", True)
+                source_hash, saltenv=saltenv, verify_ssl=verify_ssl
             )
         except MinionError as exc:
             log.exception("Failed to cache %s", source_hash, exc_info=exc)
@@ -1671,7 +1671,7 @@ def install(name=None, refresh=False, pkgs=None, **kwargs):
                     try:
                         cached_file = __salt__["cp.cache_file"](
                             cache_file,
-                            saltenv,
+                            saltenv=saltenv,
                             verify_ssl=kwargs.get("verify_ssl", True),
                         )
                     except MinionError as exc:
@@ -1686,7 +1686,7 @@ def install(name=None, refresh=False, pkgs=None, **kwargs):
                     try:
                         cached_file = __salt__["cp.cache_file"](
                             cache_file,
-                            saltenv,
+                            saltenv=saltenv,
                             verify_ssl=kwargs.get("verify_ssl", True),
                         )
                     except MinionError as exc:
@@ -1706,7 +1706,9 @@ def install(name=None, refresh=False, pkgs=None, **kwargs):
                 # It's not cached. Cache it, mate.
                 try:
                     cached_pkg = __salt__["cp.cache_file"](
-                        installer, saltenv, verify_ssl=kwargs.get("verify_ssl", True)
+                        installer,
+                        saltenv=saltenv,
+                        verify_ssl=kwargs.get("verify_ssl", True),
                     )
                 except MinionError as exc:
                     msg = "Failed to cache {}".format(installer)
@@ -1730,7 +1732,7 @@ def install(name=None, refresh=False, pkgs=None, **kwargs):
                     try:
                         cached_pkg = __salt__["cp.cache_file"](
                             installer,
-                            saltenv,
+                            saltenv=saltenv,
                             verify_ssl=kwargs.get("verify_ssl", True),
                         )
                     except MinionError as exc:
@@ -1754,7 +1756,12 @@ def install(name=None, refresh=False, pkgs=None, **kwargs):
         # Compare the hash sums
         source_hash = pkginfo[version_num].get("source_hash", False)
         if source_hash:
-            source_sum = _get_source_sum(source_hash, cached_pkg, saltenv, **kwargs)
+            source_sum = _get_source_sum(
+                source_hash,
+                cached_pkg,
+                saltenv=saltenv,
+                verify_ssl=kwargs.get("verify_ssl", True),
+            )
             log.debug(
                 "pkg.install: Source %s hash: %s",
                 source_sum["hash_type"],
@@ -2126,7 +2133,7 @@ def remove(name=None, pkgs=None, **kwargs):
                     try:
                         cached_pkg = __salt__["cp.cache_file"](
                             uninstaller,
-                            saltenv,
+                            saltenv=saltenv,
                             verify_ssl=kwargs.get("verify_ssl", True),
                         )
                     except MinionError as exc:
@@ -2150,7 +2157,7 @@ def remove(name=None, pkgs=None, **kwargs):
                         try:
                             cached_pkg = __salt__["cp.cache_file"](
                                 uninstaller,
-                                saltenv,
+                                saltenv=saltenv,
                                 verify_ssl=kwargs.get("verify_ssl", True),
                             )
                         except MinionError as exc:

salt/states/pkg.py

@@ -760,7 +760,9 @@ def _find_install_targets(
             err = "Unable to cache {0}: {1}"
             try:
                 cached_path = __salt__["cp.cache_file"](
-                    version_string, saltenv=kwargs["saltenv"], **kwargs
+                    version_string,
+                    saltenv=kwargs["saltenv"],
+                    verify_ssl=kwargs.get("verify_ssl", True),
                 )
             except CommandExecutionError as exc:
                 problems.append(err.format(version_string, exc))

tests/pytests/unit/modules/test_win_pkg.py

@@ -262,7 +262,7 @@ def test_pkg_install_verify_ssl_false():
         result = win_pkg.install(name="nsis", version="3.02", verify_ssl=False)
         mock_cp.assert_called_once_with(
             "http://download.sourceforge.net/project/nsis/NSIS%203/3.02/nsis-3.02-setup.exe",
-            "base",
+            saltenv="base",
             verify_ssl=False,
         )
         assert expected == result

tests/pytests/unit/states/test_pkg.py

@@ -3,6 +3,7 @@
 import pytest
 
 import salt.modules.beacons as beaconmod
+import salt.modules.cp as cp
 import salt.modules.pkg_resource as pkg_resource
 import salt.modules.yumpkg as yumpkg
 import salt.states.beacon as beaconstate
@@ -15,27 +16,36 @@
 
 
 @pytest.fixture
-def configure_loader_modules():
+def configure_loader_modules(minion_opts):
     return {
+        cp: {
+            "__opts__": minion_opts,
+        },
         pkg: {
             "__env__": "base",
             "__salt__": {},
             "__grains__": {"os": "CentOS", "os_family": "RedHat"},
-            "__opts__": {"test": False, "cachedir": ""},
+            "__opts__": minion_opts,
             "__instance_id__": "",
             "__low__": {},
             "__utils__": {"state.gen_tag": state_utils.gen_tag},
         },
-        beaconstate: {"__salt__": {}, "__opts__": {}},
-        beaconmod: {"__salt__": {}, "__opts__": {}},
+        beaconstate: {
+            "__salt__": {},
+            "__opts__": minion_opts,
+        },
+        beaconmod: {
+            "__salt__": {},
+            "__opts__": minion_opts,
+        },
         pkg_resource: {
             "__salt__": {},
             "__grains__": {"os": "CentOS", "os_family": "RedHat"},
         },
         yumpkg: {
             "__salt__": {},
             "__grains__": {"osarch": "x86_64", "osmajorrelease": 7},
-            "__opts__": {},
+            "__opts__": minion_opts,
         },
     }
 
@@ -563,6 +573,32 @@ def test_installed_with_changes_test_true(list_pkgs):
             assert ret["changes"] == expected
 
 
+def test_installed_with_sources(list_pkgs, tmp_path):
+    """
+    Test pkg.installed with passing `sources`
+    """
+
+    list_pkgs = MagicMock(return_value=list_pkgs)
+    pkg_source = tmp_path / "pkga-package-0.3.0.deb"
+
+    with patch.dict(
+        pkg.__salt__,
+        {
+            "cp.cache_file": cp.cache_file,
+            "pkg.list_pkgs": list_pkgs,
+            "pkg_resource.pack_sources": pkg_resource.pack_sources,
+            "lowpkg.bin_pkg_info": MagicMock(),
+        },
+    ), patch("salt.fileclient.get_file_client", return_value=MagicMock()):
+        try:
+            ret = pkg.installed("install-pkgd", sources=[{"pkga": str(pkg_source)}])
+            assert ret["result"] is False
+        except TypeError as exc:
+            if "got multiple values for keyword argument 'saltenv'" in str(exc):
+                pytest.fail(f"TypeError should have not been raised: {exc}")
+            raise exc from None
+
+
 @pytest.mark.parametrize("action", ["removed", "purged"])
 def test_removed_purged_with_changes_test_true(list_pkgs, action):
     """