[master] Improve pip target override condition with VENV_PIP_TARGET environment variable

changelog/65562.fixed.md

@@ -0,0 +1 @@
+Improve the condition of overriding target for pip with VENV_PIP_TARGET environment variable.

salt/modules/pip.py

@@ -253,7 +253,6 @@ def _get_env_activate(bin_env):
 
 
 def _find_req(link):
-
     logger.info("_find_req -- link = %s", link)
 
     with salt.utils.files.fopen(link) as fh_link:
@@ -849,9 +848,11 @@ def install(
         cmd.extend(["--build", build])
 
     # Use VENV_PIP_TARGET environment variable value as target
-    # if set and no target specified on the function call
+    # if set and no target specified on the function call.
+    # Do not set target if bin_env specified, use default
+    # for specified binary environment or expect explicit target specification.
     target_env = os.environ.get("VENV_PIP_TARGET", None)
-    if target is None and target_env is not None:
+    if target is None and target_env is not None and bin_env is None:
         target = target_env
 
     if target:

tests/pytests/unit/modules/test_pip.py

@@ -1101,7 +1101,6 @@ def test_install_extra_args_arguments_recursion_error():
     pkg = "pep8"
     mock = MagicMock(return_value={"retcode": 0, "stdout": ""})
     with patch.dict(pip.__salt__, {"cmd.run_all": mock}):
-
         pytest.raises(
             TypeError,
             lambda: pip.install(
@@ -1522,7 +1521,7 @@ def test_list_upgrades_gt9(python_binary):
             {"latest_filetype": "wheel", "version": "1.4.1", "name": "appdirs", "latest_version": "1.4.3"},
             {"latest_filetype": "sdist", "version": "1.11.63", "name": "awscli", "latest_version": "1.12.1"}
             ]"""
-    mock = MagicMock(return_value={"retcode": 0, "stdout": "{}".format(eggs)})
+    mock = MagicMock(return_value={"retcode": 0, "stdout": f"{eggs}"})
     with patch.dict(pip.__salt__, {"cmd.run_all": mock}):
         with patch("salt.modules.pip.version", MagicMock(return_value="9.1.1")):
             ret = pip.list_upgrades()
@@ -1738,28 +1737,44 @@ def test_when_version_is_called_with_a_user_it_should_be_passed_to_undelying_run
         )
 
 
-def test_install_target_from_VENV_PIP_TARGET_in_resulting_command(python_binary):
+@pytest.mark.parametrize(
+    "bin_env,target,target_env,expected_target",
+    [
+        (None, None, None, None),
+        (None, "/tmp/foo", None, "/tmp/foo"),
+        (None, None, "/tmp/bar", "/tmp/bar"),
+        (None, "/tmp/foo", "/tmp/bar", "/tmp/foo"),
+        ("/tmp/venv", "/tmp/foo", None, "/tmp/foo"),
+        ("/tmp/venv", None, "/tmp/bar", None),
+        ("/tmp/venv", "/tmp/foo", "/tmp/bar", "/tmp/foo"),
+    ],
+)
+def test_install_target_from_VENV_PIP_TARGET_in_resulting_command(
+    python_binary, bin_env, target, target_env, expected_target
+):
     pkg = "pep8"
-    target = "/tmp/foo"
-    target_env = "/tmp/bar"
     mock = MagicMock(return_value={"retcode": 0, "stdout": ""})
     environment = os.environ.copy()
-    environment["VENV_PIP_TARGET"] = target_env
+    real_get_pip_bin = pip._get_pip_bin
+
+    def mock_get_pip_bin(bin_env):
+        if not bin_env:
+            return real_get_pip_bin(bin_env)
+        return [f"{bin_env}/bin/pip"]
+
+    if target_env is not None:
+        environment["VENV_PIP_TARGET"] = target_env
     with patch.dict(pip.__salt__, {"cmd.run_all": mock}), patch.object(
         os, "environ", environment
-    ):
-        pip.install(pkg)
-        expected = [*python_binary, "install", "--target", target_env, pkg]
-        mock.assert_called_with(
-            expected,
-            saltenv="base",
-            runas=None,
-            use_vt=False,
-            python_shell=False,
-        )
-        mock.reset_mock()
-        pip.install(pkg, target=target)
-        expected = [*python_binary, "install", "--target", target, pkg]
+    ), patch.object(pip, "_get_pip_bin", mock_get_pip_bin):
+        pip.install(pkg, bin_env=bin_env, target=target)
+        expected_binary = python_binary
+        if bin_env is not None:
+            expected_binary = [f"{bin_env}/bin/pip"]
+        if expected_target is not None:
+            expected = [*expected_binary, "install", "--target", expected_target, pkg]
+        else:
+            expected = [*expected_binary, "install", pkg]
         mock.assert_called_with(
             expected,
             saltenv="base",