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

Issue:
Specialized DockerExceptions
Hello again!

while I'm working on the integration of python on whales into my applications, I found the error management mostly thought for a human use.

Currently all errors raise a DockerException that contains the exact error output produced by the CLI. That's great when using the package by a human and provides enough information to debug the problem. But when the package is used to automate processes this approach is not very helpful and to be able to properly react, a parser of the exception is needed.

Just a practical example:

```
docker.service.inspect("my_service")
```

This command can fail for a number of reasons, for example:

the swarm is not initialized:

```
python_on_whales.utils.DockerException: The docker command executed was `/usr/local/bin/docker service inspect myservice`.
It returned with code 1
The content of stdout is '[]
'
The content of stderr is 'Status: Error response from daemon: This node is not a swarm manager. Use "docker swarm init" or "docker swarm join" to connect this node to swarm and try again., Code: 1
'
```

or the service does not exist:

```
python_on_whales.utils.DockerException: The docker command executed was `/usr/local/bin/docker service inspect myservice`.
It returned with code 1
The content of stdout is '[]
'
The content of stderr is 'Status: Error: no such service: myservice, Code: 1
'
```

Probably there are much more possible reasons, but two are enough for the example


To implement specific reactions to the different problems a parser of the exception is needed:

```
try:
    docker.service.inspect("my_service")
except python_on_whales.utils.DockerException as e:
    if "This node is not a swarm manager" is str(e):
        initialized_my_node()
    elif "no such service" in str(e):
        log.error("This service does not exist")
    else:
        log.critical("Unexpected error")
```

I think that raise specific exceptions (that would extend the parent DockerException, of course) would be very very useful for the user experience:


```
try:
    docker.service.inspect("my_service")
except python_on_whales.utils.SwarmNotInitialized:
        initialized_my_node()
except python_on_whales.utis.NoSuchService:
        log.error("This service does not exist")
except Exception:
        log.critical("Unexpected error")
```

That means, in a practical way, move the parsing directly into the package to centralized that operation and make it transparent to the final user.

I understand that is a very boring issue but I'm pretty sure that would greatly improve the adoption... and of course I'm 100% available to ease this task as much as I can


Discussion / hints:
This is indeed an issue that I noticed and i'm not very happy about the only solution possible in the short term (parsing the error message). That might be something we have to do in the short term as it's necessary to build more complicated things.

I see two other solution, more mid to long term:
1) Make the CLI output different exit codes depending on the error. We would have to ask the Docker team for that. So a change to the CLI.
2) Call the Docker engine API directly like docker-py does. It may not be practical for more complicated functions that are not available in docker-py, like docker compose or buildx functions.
Just few comments, in reversed order:

> Call the Docker engine API directly like docker-py does. It may not be practical for more complicated functions that are not available in docker-py, like docker compose or buildx functions.

Based on my understanding this approach would be out of the scope of this package that is explicitly built around the docker-cli, or I'm wrong? Of course communicating with API should be the more consistent way to interact with the daemon but it seems to be a major drift the would complicate a lot the implementation, but i agree that it IS a solution

> Make the CLI output different exit codes depending on the error. We would have to ask the Docker team for that. So a change to the CLI

A very interesting approach, but probably the slower: convincing the docker team to consider that, wait for the implementation, then work on this integration... maybe that can be considered a very long term possibility, With a valid support from docker and a little patience I think that this would be the more stable solution :)

> i'm not very happy about the only solution possible in the short term (parsing the error message). That might be something we have to do in the short term as it's necessary to build more complicated things.

No, of course it is not a very exciting solution, but seems to be the only _quick_ solution, if you think that this issue can make sense.

From my side: if you think that you are interested in this issue (in a short term perspective) I will be grateful and very available to help you, if I can. Otherwise I will implement (at least temporarily) some kind of parser into my code to be able to distinguish the different cases

Thank you



My plan is 
1) raise different exceptions for different errors quickly, so by parsing the error message. Currently, with python-on-whales, it's impossible to answer the question "does that image/container/network/volume exists?", by calling the engine api, we can do that. So we need to fix it quicky. 
I would like a function `exists()`. So that we can do `docker.container.exists("my_container_name")` or `my_container.exists()`. This function would return a bool. We should also have an exception for different objects that don't exists.

2) Ask the docker team for different error codes from the CLI. It's not only useful for python-on-whales, it may be also useful for people doing bash scripting. From a bash script, knowing if an image exists or not is only possible by parsing stderr. So not great. The CLI has some progress to do there.

I'm going to do the first step, but if you want to help and contribute, I'd be happy to review any pull request I can get :) 
I started parsing stderr to raise specific exceptions, you can follow the work here: https://github.com/gabrieldemarmiesse/python-on-whales/pull/210

As soon as you think that this preliminary work is stable I will start some tests, so that I will be able to report or directly fix with PRs eventual problems
The version is unreleased yet, but you can try it with `pip install git+https://github.com/gabrieldemarmiesse/python-on-whales.git`.

We now have a custom exception `python_on_whales.exceptions.NoSuchImage`. You can trigger it with `docker.push`, `docker.image.inspect` and `docker.save` among other. I'll continue to add exceptions.

This change should be backward compatible. Please notify me if this change breaks any existing code.
Verified:

- [x] backward compatibily
- [x] new NoSuchImage exception
- [x] new image exists function

:+1: 

Fix patch:
diff --git a/python_on_whales/components/container/cli_wrapper.py b/python_on_whales/components/container/cli_wrapper.py
index afd9d053..87744db7 100644
--- a/python_on_whales/components/container/cli_wrapper.py
+++ b/python_on_whales/components/container/cli_wrapper.py
@@ -25,6 +25,7 @@
     Mount,
     NetworkSettings,
 )
+from python_on_whales.exceptions import NoSuchContainer
 from python_on_whales.utils import (
     ValidPath,
     format_dict_for_cli,
@@ -323,6 +324,15 @@ def stop(self, time: Union[int, timedelta] = None) -> None:
         """
         return ContainerCLI(self.client_config).stop(self, time)
 
+    def exists(self) -> bool:
+        """Returns `True` if the docker container exists and `False` if it doesn't exists.
+
+        If it doesn't exists, it most likely mean that it was removed.
+
+         See the `docker.container.exists` command for information about the arguments.
+        """
+        return ContainerCLI(self.client_config).exists(self.id)
+
 
 ContainerPath = Tuple[Union[Container, str], ValidPath]
 ValidContainer = Union[Container, str]
@@ -740,6 +750,9 @@ def execute(
 
         # Returns:
             Optional[str]
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         full_cmd = self.docker_cmd + ["exec"]
 
@@ -775,6 +788,9 @@ def export(self, container: ValidContainer, output: ValidPath) -> None:
             container: The container to export.
             output: The path of the output tar archive. Returning a generator of bytes
                 is not yet implemented.
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         full_cmd = self.docker_cmd + ["container", "export"]
         full_cmd.add_simple_arg("--output", output)
@@ -805,6 +821,9 @@ def inspect(self, x: Union[str, List[str]]) -> Union[Container, List[Container]]
         # Returns:
             A `python_on_whales.Container` object or a list of those
             if a list of IDs was passed as input.
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
 
         if isinstance(x, list):
@@ -824,6 +843,10 @@ def kill(
         # Arguments
             containers: One or more containers to kill
             signal: The signal to send the container
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
+
         """
         containers = to_list(containers)
         if not containers:
@@ -863,6 +886,9 @@ def logs(
 
         # Returns
             `str`
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         full_cmd = self.docker_cmd + ["container", "logs"]
         full_cmd.add_flag("--details", details)
@@ -902,6 +928,9 @@ def pause(self, containers: Union[ValidContainer, List[ValidContainer]]):
 
         # Arguments
             containers: One or more containers to pause
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         containers = to_list(containers)
         if not containers:
@@ -932,6 +961,9 @@ def rename(self, container: ValidContainer, new_name: str) -> None:
         # Arguments
             container: The container to rename
             new_name: The new name of the container.
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         full_cmd = self.docker_cmd + ["container", "rename", str(container), new_name]
         run(full_cmd)
@@ -949,6 +981,9 @@ def restart(
             containers: One or more containers to restart
             time: Amount of to wait for stop before killing the container (default 10s).
                 If `int`, the unit is seconds.
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         containers = to_list(containers)
         if not containers:
@@ -980,6 +1015,9 @@ def remove(
             containers: One or more containers.
             force: Force the removal of a running container (uses SIGKILL)
             volumes: Remove anonymous volumes associated with the container
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         containers = to_list(containers)
         if not containers:
@@ -1495,6 +1533,9 @@ def stop(
         # Arguments
             containers: One or a list of containers.
             time: Seconds to wait for stop before killing a container (default 10)
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         containers = to_list(containers)
         if not containers:
@@ -1527,6 +1568,9 @@ def unpause(self, x: Union[ValidContainer, List[ValidContainer]]):
 
         # Arguments
             x: One or more containers (name, id or `python_on_whales.Container` object).
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         x = to_list(x)
         if not x:
@@ -1579,6 +1623,9 @@ def update(
                 to enable unlimited swap.
             pids_limit: Tune container pids limit (set `-1` for unlimited)
             restart: Restart policy to apply when a container exits (default "no")
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         x = to_list(x)
         if not x:
@@ -1648,6 +1695,9 @@ def wait(
         # [8, 10]
         docker.container.remove([cont_1, cont_2])
         ```
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchContainer` if the container does not exists.
         """
         if x == []:
             # nothing to do
@@ -1661,6 +1711,22 @@ def wait(
             full_cmd.append(x)
             return int(run(full_cmd))
 
+    def exists(self, x: str) -> bool:
+        """Returns `True` if the container exists. `False` otherwise.
+
+         It's just calling `docker.container.inspect(...)` and verifies that it doesn't throw
+         a `python_on_whales.exceptions.NoSuchContainer`.
+
+        # Returns
+            A `bool`
+        """
+        try:
+            self.inspect(x)
+        except NoSuchContainer:
+            return False
+        else:
+            return True
+
 
 class ContainerStats:
     def __init__(self, json_dict: Dict[str, Any]):
diff --git a/python_on_whales/components/image/cli_wrapper.py b/python_on_whales/components/image/cli_wrapper.py
index 754d5af0..cd5c2594 100644
--- a/python_on_whales/components/image/cli_wrapper.py
+++ b/python_on_whales/components/image/cli_wrapper.py
@@ -254,7 +254,7 @@ def inspect(self, x: Union[str, List[str]]) -> Union[Image, List[Image]]:
             return Image(self.client_config, x)
 
     def exists(self, x: str) -> bool:
-        """Verify that an image exists.
+        """Returns `True` if the image exists. `False` otherwise.
 
          It's just calling `docker.image.inspect(...)` and verifies that it doesn't throw
          a `python_on_whales.exceptions.NoSuchImage`.
diff --git a/python_on_whales/exceptions.py b/python_on_whales/exceptions.py
index 6e1d9737..7cd2d9aa 100644
--- a/python_on_whales/exceptions.py
+++ b/python_on_whales/exceptions.py
@@ -31,6 +31,10 @@ def __init__(
         super().__init__(error_msg)
 
 
+class NoSuchContainer(DockerException):
+    pass
+
+
 class NoSuchImage(DockerException):
     pass
 
diff --git a/python_on_whales/utils.py b/python_on_whales/utils.py
index a8096896..9b65b818 100644
--- a/python_on_whales/utils.py
+++ b/python_on_whales/utils.py
@@ -8,7 +8,12 @@
 
 import pydantic
 
-from python_on_whales.exceptions import DockerException, NoSuchImage, NoSuchService
+from python_on_whales.exceptions import (
+    DockerException,
+    NoSuchContainer,
+    NoSuchImage,
+    NoSuchService,
+)
 
 PROJECT_ROOT = Path(__file__).parents[1]
 
@@ -109,6 +114,13 @@ def run(
                     completed_process.stdout,
                     completed_process.stderr,
                 )
+            if "no such container" in completed_process.stderr.decode().lower():
+                raise NoSuchContainer(
+                    args,
+                    completed_process.returncode,
+                    completed_process.stdout,
+                    completed_process.stderr,
+                )
 
         raise DockerException(
             args,


Test patch:
diff --git a/tests/python_on_whales/components/test_container.py b/tests/python_on_whales/components/test_container.py
index a0d3d767..5e4fce48 100644
--- a/tests/python_on_whales/components/test_container.py
+++ b/tests/python_on_whales/components/test_container.py
@@ -14,7 +14,7 @@
     ContainerInspectResult,
     ContainerState,
 )
-from python_on_whales.exceptions import DockerException
+from python_on_whales.exceptions import DockerException, NoSuchContainer
 from python_on_whales.test_utils import get_all_jsons, random_name
 
 
@@ -452,3 +452,51 @@ def test_exec_change_directory():
         assert c.execute(["pwd"], workdir="/tmp") == "/tmp"
         assert c.execute(["pwd"], workdir="/etc") == "/etc"
         assert c.execute(["pwd"], workdir="/usr/lib") == "/usr/lib"
+
+
+@pytest.mark.parametrize(
+    "docker_function",
+    [
+        docker.container.remove,
+        docker.container.stop,
+        docker.container.diff,
+        docker.container.inspect,
+        docker.container.kill,
+        docker.container.pause,
+        docker.container.restart,
+        docker.container.start,
+        docker.container.commit,
+        docker.container.logs,
+        docker.container.unpause,
+        docker.container.wait,
+    ],
+)
+def test_functions_nosuchcontainer(docker_function):
+    with pytest.raises(NoSuchContainer):
+        docker_function("DOODODGOIHURHURI")
+
+
+def test_copy_nosuchcontainer():
+    with pytest.raises(NoSuchContainer):
+        docker.container.copy(("dodueizbgueirzhgueoz", "/dududada"), "/tmp/dudufeoz")
+
+
+def test_execute_nosuchcontainer():
+    with pytest.raises(NoSuchContainer):
+        docker.container.execute("dodueizbgueirzhgueoz", ["echo", "dudu"])
+
+
+def test_export_nosuchcontainer(tmp_path):
+    dest = tmp_path / "dodo.tar"
+    with pytest.raises(NoSuchContainer):
+        docker.container.export("some_random_container_that_does_not_exists", dest)
+
+
+def test_rename_nosuchcontainer():
+    with pytest.raises(NoSuchContainer):
+        docker.container.rename("dodueizbgueirzhgueoz", "new_name")
+
+
+def test_update_nosuchcontainer():
+    with pytest.raises(NoSuchContainer):
+        docker.container.update("grueighuri", cpus=4)