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

Issue:
docker container exec without tty: inconsistent behaviour between docker and python on whales
Hello, sorry for the long issue, as usual I tried to give as much as debug information as possible

I found that when `docker.container.execute(...)` is executed without a tty, the stderr is lost, while on docker it is returned combined to the stdout.
In the examples I will use both tty and interactive flags because the use of tty only is not implemented yet in python on whales, but the issue _should_ not be related to the interactive flag.

Let's image to have the following script into a container:

```
#!/usr/bin/python3
import sys
print("This is out", file=sys.stdout)
print("This is err", file=sys.stderr)
```

 `docker exec -it my_container /test.py` =>

```
This is out
This is err
```

I got exactly the same on python on whales:

```
>>> print(docker.container.execute("my_container", command=["python3", "/test.py"], interactive=True, tty=True))
This is out
This is err
```

But without tty the output differs,
on docker I still get both the streams:

 `docker exec my_container /test.py` =>

```
This is out
This is err
```

But with python on whales the error stream is lost:

```
>>> print(docker.container.execute("my_container", command=["python3", "/test.py"]))
This is out
```

I edited here:
https://github.com/gabrieldemarmiesse/python-on-whales/blob/330519f699271b9783147cc36b013204f7712440/python_on_whales/components/container/cli_wrapper.py#L788

by adding `return_stderr=True`

`result = run(full_cmd, tty=tty)` => `result = run(full_cmd, tty=tty, return_stderr=True)`

and this way I can get both the streams as a tuple:

```
>>> print(docker.container.execute("my_container", command=["python3", "/test.py"]))
('This is out', 'This is err')
```

Can we consider to add the error stream to the output?

Anyway also this way I cannot combine them, I can only print the whole stdout and then the whole stderr. Maybe that in this case a stream would be useful? Maybe that the stream_stdout_and_stderr utility would do the trick? (ok, I'm taking a guess!)

Have a nice weekend and thank you! :sun_with_face: 


Discussion / hints:
P.S. the use case is the execution of commands on github actions. Since the runners there are headless the tty is not available and I'm getting lost all the error outputs :-\
When you do this: 
```
>>> print(docker.container.execute("my_container", command=["python3", "/test.py"], interactive=True, tty=True))
```

The print is actually useless. When tty works, your terminal is directly connected to the container input and output, bypassing python entirely.

The decision to not return stderr in the default usage was taken to allow users to run containers to easily get outputs that they could reuse in the rest of their program.

In docker.run(), the main way to get stderr is to use the stream option. When doing that, you can get stderr and stdout. The stream option wasn't implemented yet for `docker.execute()`. So I'll put the stream option on the backlog for execute.
Very clear, so I will wait for the introduction of the stream option :+1: 

Fix patch:
diff --git a/python_on_whales/components/container/cli_wrapper.py b/python_on_whales/components/container/cli_wrapper.py
index 6d886533..f4f2f936 100644
--- a/python_on_whales/components/container/cli_wrapper.py
+++ b/python_on_whales/components/container/cli_wrapper.py
@@ -215,7 +215,8 @@ def execute(
         tty: bool = False,
         user: Optional[str] = None,
         workdir: Optional[ValidPath] = None,
-    ):
+        stream: bool = False,
+    ) -> Union[None, str, Iterable[Tuple[str, bytes]]]:
         """Execute a command in this container
 
         See the [`docker.container.execute`](../sub-commands/container.md#execute)
@@ -232,6 +233,7 @@ def execute(
             tty,
             user,
             workdir,
+            stream,
         )
 
     def export(self, output: ValidPath) -> None:
@@ -733,7 +735,8 @@ def execute(
         tty: bool = False,
         user: Optional[str] = None,
         workdir: Optional[ValidPath] = None,
-    ) -> Optional[str]:
+        stream: bool = False,
+    ) -> Union[None, str, Iterable[Tuple[str, bytes]]]:
         """Execute a command inside a container
 
         Alias: `docker.execute(...)`
@@ -754,6 +757,7 @@ def execute(
                 to write on it.
             user: Username or UID, format: `"<name|uid>[:<group|gid>]"`
             workdir: Working directory inside the container
+            stream: Similar to `docker.run(..., stream=True)`.
 
         # Returns:
             Optional[str]
@@ -774,6 +778,24 @@ def execute(
                 "Currently, docker.container.execute(interactive=True) must have"
                 "tty=True. interactive=True and tty=False is not yet implemented."
             )
+
+        if interactive and stream:
+            raise ValueError(
+                "You can't set interactive=True and stream=True at the same"
+                "time. Their purpose are not compatible."
+            )
+
+        if tty and stream:
+            raise ValueError(
+                "You can't set interactive=True and stream=True at the same"
+                "time. Their purpose are not compatible."
+            )
+
+        if detach and stream:
+            raise ValueError(
+                "You can't detach and stream at the same time. It's not compatible."
+            )
+
         full_cmd.add_flag("--interactive", interactive)
         full_cmd.add_flag("--privileged", privileged)
         full_cmd.add_flag("--tty", tty)
@@ -784,12 +806,14 @@ def execute(
         full_cmd.append(container)
         for arg in to_list(command):
             full_cmd.append(arg)
-
-        result = run(full_cmd, tty=tty)
-        if detach:
-            return None
+        if stream:
+            return stream_stdout_and_stderr(full_cmd)
         else:
-            return result
+            result = run(full_cmd, tty=tty)
+            if detach:
+                return None
+            else:
+                return result
 
     def export(self, container: ValidContainer, output: ValidPath) -> None:
         """Export a container's filesystem as a tar archive


Test patch:
diff --git a/tests/python_on_whales/components/test_container.py b/tests/python_on_whales/components/test_container.py
index 3ee98433..00047cc8 100644
--- a/tests/python_on_whales/components/test_container.py
+++ b/tests/python_on_whales/components/test_container.py
@@ -284,6 +284,17 @@ def test_execute():
     docker.kill(my_container)
 
 
+def test_execute_stream():
+    my_container = docker.run(
+        "busybox:1", ["sleep", "infinity"], detach=True, remove=True
+    )
+    exec_result = list(
+        docker.execute(my_container, ["sh", "-c", ">&2 echo dodo"], stream=True)
+    )
+    assert exec_result == [("stderr", b"dodo\n")]
+    docker.kill(my_container)
+
+
 def test_diff():
     my_container = docker.run(
         "busybox:1", ["sleep", "infinity"], detach=True, remove=True