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

Issue:
Docker service logs
Hello,
I was trying to use docker service logs but it is not implemented yet

I made some tests with my local copy by (mostly) copying the implementation of docker.container.logs and it works fine included the follow and stream options

The only three differences that I found between container.logs and service.logs are:

1. service logs has some additional flags, but they are very easy to be included:
```
      --no-resolve     Do not map IDs to Names in output
      --no-task-ids    Do not include task IDs in output
      --no-trunc       Do not truncate output
      --raw            Do not neatly format logs
```

2. the --until option is only available for container.logs (that is very strange considering that the --since option is available on both... but this is another story)

3. service.logs has multiple targets. While container.logs only targets containers, service.logs works on both services and tasks. That can be tricky for the initial inspection:

```
self.inspect(service_or_task)
```

the inspect method already implemented in components/service/cli_wrapper.py works fine with services, but not with tasks:

logs("a_service") -> ok
logs("a_task") -> `python_on_whales.exceptions.NoSuchService The content of stderr is 'Status: Error: no such service: xyz.1.s1rsmzocziba8rbjpjo4vbw9w, Code: 1`

This would require some extra work, for example a method to guess if the target is a service or a task (a task should contains a `.{slot}.{id}` in the name) to properly validate it

In that case the raised exception should be a python_on_whales.exceptions.NoSuchServiceOrTask based on the corresponding error raised from docker:

```
The docker command executed was `/usr/local/bin/docker service logs --tail 1 invalid`.
It returned with code 1
The content of stdout can be found above the stacktrace (it wasn't captured).
The content of stderr is 'no such task or service: invalid
```

Final thought: the implementation of service.logs would basically duplicate some utilities and may be worth to move them in a utils package. For example the format_time_arg method from components/container and the task inspect from components/task




Discussion / hints:
Thank you for the bug report and the insights! I don't have the time to take care of it right now. But maybe in the next weeks or so. As usual, if you're willing to make a pull request, i'll gladly review it as it's a lot faster for me :) 

Fix patch:
diff --git a/python_on_whales/components/container/cli_wrapper.py b/python_on_whales/components/container/cli_wrapper.py
index 99b419eb..26e3791f 100644
--- a/python_on_whales/components/container/cli_wrapper.py
+++ b/python_on_whales/components/container/cli_wrapper.py
@@ -29,6 +29,7 @@
 from python_on_whales.utils import (
     ValidPath,
     format_dict_for_cli,
+    format_time_arg,
     removeprefix,
     run,
     stream_stdout_and_stderr,
@@ -870,12 +871,10 @@ def logs(
         follow: bool = False,
         stream: bool = False,
     ) -> Union[str, Iterable[Tuple[str, bytes]]]:
-        """Returns the logs of a container as a string.
+        """Returns the logs of a container as a string or an iterator.
 
         Alias: `docker.logs(...)`
 
-        The follow option is not yet implemented.
-
         # Arguments
             container: The container to get the logs of
             details: Show extra details provided to logs
@@ -1797,20 +1796,6 @@ def __repr__(self):
         return f"<{self.__class__} object, attributes are {attr}>"
 
 
-def format_time_for_docker(time_object: Union[datetime, timedelta]) -> str:
-    if isinstance(time_object, datetime):
-        return time_object.strftime("%Y-%m-%dT%H:%M:%S")
-    elif isinstance(time_object, timedelta):
-        return f"{time_object.total_seconds()}s"
-
-
-def format_time_arg(time_object):
-    if time_object is None:
-        return None
-    else:
-        return format_time_for_docker(time_object)
-
-
 def join_if_not_none(sequence: Optional[list]) -> Optional[str]:
     if sequence is None:
         return None
diff --git a/python_on_whales/components/service/cli_wrapper.py b/python_on_whales/components/service/cli_wrapper.py
index 3d2f7feb..ef57814c 100644
--- a/python_on_whales/components/service/cli_wrapper.py
+++ b/python_on_whales/components/service/cli_wrapper.py
@@ -18,7 +18,14 @@
     ServiceVersion,
 )
 from python_on_whales.exceptions import NoSuchService
-from python_on_whales.utils import ValidPath, format_dict_for_cli, run, to_list
+from python_on_whales.utils import (
+    ValidPath,
+    format_dict_for_cli,
+    format_time_arg,
+    run,
+    stream_stdout_and_stderr,
+    to_list,
+)
 
 
 class Service(ReloadableObjectFromJson):
@@ -261,9 +268,72 @@ def exists(self, x: str) -> bool:
         else:
             return True
 
-    def logs(self):
-        """Not yet implemented"""
-        raise NotImplementedError
+    def logs(
+        self,
+        service: ValidService,
+        details: bool = False,
+        since: Union[None, datetime, timedelta] = None,
+        tail: Optional[int] = None,
+        timestamps: bool = False,
+        follow: bool = False,
+        raw: bool = False,
+        task_ids: bool = True,
+        resolve: bool = True,
+        truncate: bool = True,
+        stream: bool = False,
+    ):
+        """Returns the logs of a service as a string or an iterator.
+
+        # Arguments
+            service: The service to get the logs of
+            details: Show extra details provided to logs
+            since: Use a datetime or timedelta to specify the lower
+                date limit for the logs.
+            tail: Number of lines to show from the end of the logs (default all)
+            timestamps: Put timestamps next to lines.
+            follow: If `False` (the default), the logs returned are the logs up to the time
+                of the function call. If `True`, the logs of the container up to the time the
+                service is stopped (removed) are displayed.
+                Which is why you must use the `stream` option if you use the `follow` option.
+                Without `stream`, only a `str` will be returned, possibly much later in the
+                future (maybe never if the service is never removed). So this option is not
+                possible (You'll get an error if you use follow and not stream).
+                With `stream`, you'll be able to read the logs in real time and stop
+                whenever you need.
+            stream: Similar to the `stream` argument of `docker.run()`.
+                This function will then returns and iterator that will yield a
+                tuple `(source, content)` with `source` being `"stderr"` or
+                `"stdout"`. `content` is the content of the line as bytes.
+                Take a look at [the user guide](https://gabrieldemarmiesse.github.io/python-on-whales/user_guide/docker_run/#stream-the-output)
+                to have an example of the output.
+
+        # Returns
+            `str` if `stream=False` (the default), `Iterable[Tuple[str, bytes]]`
+            if `stream=True`.
+
+        # Raises
+            `python_on_whales.exceptions.NoSuchService` if the service does not exists.
+        """
+        # first we verify that the service exists and raise an exception if not.
+        self.inspect(str(service))
+
+        full_cmd = self.docker_cmd + ["service", "logs"]
+        full_cmd.add_flag("--details", details)
+        full_cmd.add_simple_arg("--since", format_time_arg(since))
+        full_cmd.add_simple_arg("--tail", tail)
+        full_cmd.add_flag("--timestamps", timestamps)
+        full_cmd.add_flag("--follow", follow)
+        full_cmd.add_flag("--raw", raw)
+        full_cmd.add_flag("--no-task-ids", not task_ids)
+        full_cmd.add_flag("--no-resolve", not resolve)
+        full_cmd.add_flag("--no-trunc", not truncate)
+        full_cmd.append(service)
+
+        iterator = stream_stdout_and_stderr(full_cmd)
+        if stream:
+            return iterator
+        else:
+            return "".join(x[1].decode() for x in iterator)
 
     def list(self) -> List[Service]:
         """Returns the list of services
diff --git a/python_on_whales/utils.py b/python_on_whales/utils.py
index 2e1fb4d6..6fa0abec 100644
--- a/python_on_whales/utils.py
+++ b/python_on_whales/utils.py
@@ -1,5 +1,6 @@
 import os
 import subprocess
+from datetime import datetime, timedelta
 from pathlib import Path
 from queue import Queue
 from subprocess import PIPE, Popen
@@ -253,3 +254,17 @@ def all_fields_optional(cls):
         field.required = False
         field.allow_none = True
     return cls
+
+
+def format_time_arg(time_object):
+    if time_object is None:
+        return None
+    else:
+        return format_time_for_docker(time_object)
+
+
+def format_time_for_docker(time_object: Union[datetime, timedelta]) -> str:
+    if isinstance(time_object, datetime):
+        return time_object.strftime("%Y-%m-%dT%H:%M:%S")
+    elif isinstance(time_object, timedelta):
+        return f"{time_object.total_seconds()}s"


Test patch:
diff --git a/tests/python_on_whales/components/test_service.py b/tests/python_on_whales/components/test_service.py
index c9612835..83975fbf 100644
--- a/tests/python_on_whales/components/test_service.py
+++ b/tests/python_on_whales/components/test_service.py
@@ -25,6 +25,15 @@ def test_tasks():
     docker.service.remove(service)
 
 
+@pytest.mark.usefixtures("swarm_mode")
+def test_get_logs():
+    with docker.service.create(
+        "busybox", ["sh", "-c", "echo dodo && sleep infinity"]
+    ) as my_service:
+        assert my_service.ps()[0].desired_state == "running"
+        assert docker.service.logs(my_service).split("|")[-1].strip() == "dodo"
+
+
 @pytest.mark.usefixtures("swarm_mode")
 def test_service_scale():
     service = docker.service.create("busybox", ["sleep", "infinity"])