Repository: gabrieldemarmiesse/python-on-whales (language: python)

Issue:
ssh argument not working as expected
I have been working on porting a bash script which I use to build some of my docker images to python, and your package has been a great help! I appreciate how easy and straightforward it is to port my docker run commands directly to python.

I just ran into an issue with the `ssh` arg though. Looks like when setting `ssh=default` in the `docker.build()` command, I see an error:
```
error: invalid empty ssh agent socket, make sure SSH_AUTH_SOCK is set
```

I checked, and that variable is set in my shell environment. Any suggestions? Am I using the argument incorrectly? The same bash script with `--ssh default` is working without problems.

Let me know if I can provide anything to help debug. Thanks!

```
    raise DockerException(
python_on_whales.utils.DockerException: The docker command executed was `/usr/bin/docker buildx build --build-arg FROM_TAG=gui-support --ssh default --file docker/ros/kinetic/autoyard-core/Dockerfile --tag ros-kinetic-autoyard-core:test --iidfile /tmp/tmpc9q55l8w/id_file.txt .`.
It returned with code 1

```

Logged the variable with `print("SSH socket {}".format(os.environ['SSH_AUTH_SOCK']))` and can see that the variable is printed correctly, but the docker build command still fails:
```
SSH socket /run/user/1001/keyring/ssh
error: invalid empty ssh agent socket, make sure SSH_AUTH_SOCK is set
```


Discussion / hints:
Looks like I might have a clue as to what the problem is. It looks like when the `env` dict passed to `subprocess` is not `None`, the generated subprocess might only have access to the the explicit variables defined in that dictionary.

From the subprocess [docs](https://docs.python.org/3/library/subprocess.html):
```
if env is not None, it must be a mapping that defines the environment variables for the new process; these are used instead of the default behavior of inheriting the current process’ environment. It is passed directly to Popen.
```

From what I can tell, in `utils.py` [Line 107](https://github.com/gabrieldemarmiesse/python-on-whales/blob/feca4f434ce575bfab6a636a5f2d576b71132ced/python_on_whales/utils.py#L107), sets the `DOCKER_CLI_EXPERIMENTAL` variable by creating an `env` dict. Since in the subsequent steps, we are not passing `None` as `env` to `subprocess`, the generated subprocess does not have access to other variables set in my current/parent shell process.

To test this theory, I commented out this line, and replaced it with `env = None`, and ran `docker.build` again, with `ssh=default` (after `export DOCKER_CLI_EXPERIMENTAL=enabled` and `export DOCKER_BUILDKIT=1`), and the argument works now, and my build proceeds as expected.

As far as I understand, it would be better to use `os.environ` to set the `DOCKER_CLI_EXPERIMENT` variable, and pass `env=None` to the subprocess command. I tried this out, by replacing line 107 with:
```
os.environ["DOCKER_CLI_EXPERIMENTAL"] = "enabled"
# env["DOCKER_CLI_EXPERIMENTAL"] = "enabled"
env = None
```

I unset the two variables I exported earlier, and looks like this change of using `os.environ` is allowing the subprocess access to my local environment variables, and my build is working with `ssh=default`.

Let me know if this makes sense, or if there is an easier way of doing this which I missed. Happy to open a PR if you agree with this approach/diagnosis.

Thanks!
Thank you for this great bug report! Reading the code with this in mind, I understand what is wrong. Is right way to fix it is to have a dict and put os.environ inside, then put the custom environment variables inside. Something like

```python
def run(..., env={}):
    subprocess_env = {}
    subprocess_env.update(os.environ)
    subprocess_env.update(env)
    subprocess.call(..., env=subprocess_env)
```

I noticed this bug before in the bake command and fixed it, but my fix wasn't very good because it worked only for bake. I'll make a fix today or tomorrow. Thanks again for making this package better!

Fix patch:
diff --git a/python_on_whales/components/buildx/cli_wrapper.py b/python_on_whales/components/buildx/cli_wrapper.py
index 722cf6f3..0fbdaa7e 100644
--- a/python_on_whales/components/buildx/cli_wrapper.py
+++ b/python_on_whales/components/buildx/cli_wrapper.py
@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 import json
-import os
 import tempfile
 from enum import Enum
 from pathlib import Path
@@ -170,8 +169,7 @@ def bake(
             full_cmd.add_simple_arg("--file", file)
         full_cmd.add_args_list("--set", format_dict_for_cli(set))
         targets = to_list(targets)
-        env = dict(os.environ)
-        env.update(variables)
+        env = dict(variables)
         if print:
             return json.loads(run(full_cmd + targets, env=env))
         else:
diff --git a/python_on_whales/components/stack/cli_wrapper.py b/python_on_whales/components/stack/cli_wrapper.py
index f7a7819a..79edc076 100644
--- a/python_on_whales/components/stack/cli_wrapper.py
+++ b/python_on_whales/components/stack/cli_wrapper.py
@@ -1,6 +1,5 @@
 from __future__ import annotations
 
-import os
 from pathlib import Path
 from typing import Dict, List, Optional, Union
 
@@ -81,8 +80,7 @@ def deploy(
         full_cmd.add_flag("--with-registry-auth", with_registry_auth)
         full_cmd.append(name)
 
-        env = dict(os.environ)
-        env.update(read_env_files([Path(x) for x in env_files]))
+        env = read_env_files([Path(x) for x in env_files])
         env.update(variables)
 
         run(full_cmd, capture_stdout=False, capture_stderr=False, env=env)
diff --git a/python_on_whales/utils.py b/python_on_whales/utils.py
index 23d9e15e..25da03e4 100644
--- a/python_on_whales/utils.py
+++ b/python_on_whales/utils.py
@@ -1,3 +1,4 @@
+import os
 import subprocess
 import warnings
 from pathlib import Path
@@ -102,11 +103,11 @@ def run(
     env: Dict[str, str] = {},
 ) -> Union[str, Tuple[str, str]]:
     args = [str(x) for x in args]
+    subprocess_env = dict(os.environ)
+    subprocess_env.update(env)
     if args[1] == "buildx":
         install_buildx_if_needed(args[0])
-        env["DOCKER_CLI_EXPERIMENTAL"] = "enabled"
-    if env == {}:
-        env = None
+        subprocess_env["DOCKER_CLI_EXPERIMENTAL"] = "enabled"
     if capture_stdout:
         stdout_dest = subprocess.PIPE
     else:
@@ -116,7 +117,7 @@ def run(
     else:
         stderr_dest = None
     completed_process = subprocess.run(
-        args, input=input, stdout=stdout_dest, stderr=stderr_dest, env=env
+        args, input=input, stdout=stdout_dest, stderr=stderr_dest, env=subprocess_env
     )
 
     if completed_process.returncode != 0:


Test patch:
diff --git a/tests/python_on_whales/test_utils.py b/tests/python_on_whales/test_utils.py
index a7e17bba..02768b03 100644
--- a/tests/python_on_whales/test_utils.py
+++ b/tests/python_on_whales/test_utils.py
@@ -3,3 +3,14 @@
 
 def test_project_root():
     assert (python_on_whales.utils.PROJECT_ROOT / "setup.py").exists()
+
+
+def test_environment_variables_propagation(monkeypatch):
+    monkeypatch.setenv("SOME_VARIABLE", "dododada")
+
+    stdout = python_on_whales.utils.run(
+        ["bash", "-c", "echo $SOME_VARIABLE && echo $OTHER_VARIABLE"],
+        capture_stdout=True,
+        env={"OTHER_VARIABLE": "dudu"},
+    )
+    assert stdout == "dododada\ndudu"