Re: [PATCH v7 11/21] dts: remote session docstring update

[Yoan Picchi <yoan.picchi@foss.arm.com>]

On 11/15/23 13:09, Juraj Linkeš wrote:
> Format according to the Google format and PEP257, with slight
> deviations.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>   dts/framework/remote_session/__init__.py      |  39 +++++-
>   .../remote_session/remote_session.py          | 128 +++++++++++++-----
>   dts/framework/remote_session/ssh_session.py   |  16 +--
>   3 files changed, 135 insertions(+), 48 deletions(-)
> 
> diff --git a/dts/framework/remote_session/__init__.py b/dts/framework/remote_session/__init__.py
> index 5e7ddb2b05..51a01d6b5e 100644
> --- a/dts/framework/remote_session/__init__.py
> +++ b/dts/framework/remote_session/__init__.py
> @@ -2,12 +2,14 @@
>   # Copyright(c) 2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2023 University of New Hampshire
>   
> -"""
> -The package provides modules for managing remote connections to a remote host (node),
> -differentiated by OS.
> -The package provides a factory function, create_session, that returns the appropriate
> -remote connection based on the passed configuration. The differences are in the
> -underlying transport protocol (e.g. SSH) and remote OS (e.g. Linux).
> +"""Remote interactive and non-interactive sessions.
> +
> +This package provides modules for managing remote connections to a remote host (node).
> +
> +The non-interactive sessions send commands and return their output and exit code.
> +
> +The interactive sessions open an interactive shell which is continuously open,
> +allowing it to send and receive data within that particular shell.
>   """
>   
>   # pylama:ignore=W0611
> @@ -26,10 +28,35 @@
>   def create_remote_session(
>       node_config: NodeConfiguration, name: str, logger: DTSLOG
>   ) -> RemoteSession:
> +    """Factory for non-interactive remote sessions.
> +
> +    The function returns an SSH session, but will be extended if support
> +    for other protocols is added.
> +
> +    Args:
> +        node_config: The test run configuration of the node to connect to.
> +        name: The name of the session.
> +        logger: The logger instance this session will use.
> +
> +    Returns:
> +        The SSH remote session.
> +    """
>       return SSHSession(node_config, name, logger)
>   
>   
>   def create_interactive_session(
>       node_config: NodeConfiguration, logger: DTSLOG
>   ) -> InteractiveRemoteSession:
> +    """Factory for interactive remote sessions.
> +
> +    The function returns an interactive SSH session, but will be extended if support
> +    for other protocols is added.
> +
> +    Args:
> +        node_config: The test run configuration of the node to connect to.
> +        logger: The logger instance this session will use.
> +
> +    Returns:
> +        The interactive SSH remote session.
> +    """
>       return InteractiveRemoteSession(node_config, logger)
> diff --git a/dts/framework/remote_session/remote_session.py b/dts/framework/remote_session/remote_session.py
> index 0647d93de4..629c2d7b9c 100644
> --- a/dts/framework/remote_session/remote_session.py
> +++ b/dts/framework/remote_session/remote_session.py
> @@ -3,6 +3,13 @@
>   # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2022-2023 University of New Hampshire
>   
> +"""Base remote session.
> +
> +This module contains the abstract base class for remote sessions and defines
> +the structure of the result of a command execution.
> +"""
> +
> +
>   import dataclasses
>   from abc import ABC, abstractmethod
>   from pathlib import PurePath
> @@ -15,8 +22,14 @@
>   
>   @dataclasses.dataclass(slots=True, frozen=True)
>   class CommandResult:
> -    """
> -    The result of remote execution of a command.
> +    """The result of remote execution of a command.
> +
> +    Attributes:
> +        name: The name of the session that executed the command.
> +        command: The executed command.
> +        stdout: The standard output the command produced.
> +        stderr: The standard error output the command produced.
> +        return_code: The return code the command exited with.
>       """
>   
>       name: str
> @@ -26,6 +39,7 @@ class CommandResult:
>       return_code: int
>   
>       def __str__(self) -> str:
> +        """Format the command outputs."""
>           return (
>               f"stdout: '{self.stdout}'\n"
>               f"stderr: '{self.stderr}'\n"
> @@ -34,13 +48,24 @@ def __str__(self) -> str:
>   
>   
>   class RemoteSession(ABC):
> -    """
> -    The base class for defining which methods must be implemented in order to connect
> -    to a remote host (node) and maintain a remote session. The derived classes are
> -    supposed to implement/use some underlying transport protocol (e.g. SSH) to
> -    implement the methods. On top of that, it provides some basic services common to
> -    all derived classes, such as keeping history and logging what's being executed
> -    on the remote node.
> +    """Non-interactive remote session.
> +
> +    The abstract methods must be implemented in order to connect to a remote host (node)
> +    and maintain a remote session.
> +    The subclasses must use (or implement) some underlying transport protocol (e.g. SSH)
> +    to implement the methods. On top of that, it provides some basic services common to all
> +    subclasses, such as keeping history and logging what's being executed on the remote node.
> +
> +    Attributes:
> +        name: The name of the session.
> +        hostname: The node's hostname. Could be an IP (possibly with port, separated by a colon)
> +            or a domain name.
> +        ip: The IP address of the node or a domain name, whichever was used in `hostname`.
> +        port: The port of the node, if given in `hostname`.
> +        username: The username used in the connection.
> +        password: The password used in the connection. Most frequently empty,
> +            as the use of passwords is discouraged.
> +        history: The executed commands during this session.
>       """
>   
>       name: str
> @@ -59,6 +84,16 @@ def __init__(
>           session_name: str,
>           logger: DTSLOG,
>       ):
> +        """Connect to the node during initialization.
> +
> +        Args:
> +            node_config: The test run configuration of the node to connect to.
> +            session_name: The name of the session.
> +            logger: The logger instance this session will use.
> +
> +        Raises:
> +            SSHConnectionError: If the connection to the node was not successful.
> +        """
>           self._node_config = node_config
>   
>           self.name = session_name
> @@ -79,8 +114,13 @@ def __init__(
>   
>       @abstractmethod
>       def _connect(self) -> None:
> -        """
> -        Create connection to assigned node.
> +        """Create a connection to the node.
> +
> +        The implementation must assign the established session to self.session.
> +
> +        The implementation must except all exceptions and convert them to an SSHConnectionError.
> +
> +        The implementation may optionally implement retry attempts.
>           """
>   
>       def send_command(
> @@ -90,11 +130,24 @@ def send_command(
>           verify: bool = False,
>           env: dict | None = None,
>       ) -> CommandResult:
> -        """
> -        Send a command to the connected node using optional env vars
> -        and return CommandResult.
> -        If verify is True, check the return code of the executed command
> -        and raise a RemoteCommandExecutionError if the command failed.
> +        """Send `command` to the connected node.
> +
> +        The :option:`--timeout` command line argument and the :envvar:`DTS_TIMEOUT`
> +        environment variable configure the timeout of command execution.
> +
> +        Args:
> +            command: The command to execute.
> +            timeout: Wait at most this long in seconds to execute `command`.
> +            verify: If :data:`True`, will check the exit code of `command`.
> +            env: A dictionary with environment variables to be used with `command` execution.
> +
> +        Raises:
> +            SSHSessionDeadError: If the session isn't alive when sending `command`.
> +            SSHTimeoutError: If `command` execution timed out.
> +            RemoteCommandExecutionError: If verify is :data:`True` and `command` execution failed.
> +
> +        Returns:
> +            The output of the command along with the return code.
>           """
>           self._logger.info(
>               f"Sending: '{command}'" + (f" with env vars: '{env}'" if env else "")
> @@ -115,29 +168,36 @@ def send_command(
>       def _send_command(
>           self, command: str, timeout: float, env: dict | None
>       ) -> CommandResult:
> -        """
> -        Use the underlying protocol to execute the command using optional env vars
> -        and return CommandResult.
> +        """Send a command to the connected node.
> +
> +        The implementation must execute the command remotely with `env` environment variables
> +        and return the result.
> +
> +        The implementation must except all exceptions and raise an SSHSessionDeadError if
> +        the session is not alive and an SSHTimeoutError if the command execution times out.

3 way "and". Needs comas or splitting the sentence.

>           """
>   
>       def close(self, force: bool = False) -> None:
> -        """
> -        Close the remote session and free all used resources.
> +        """Close the remote session and free all used resources.
> +
> +        Args:
> +            force: Force the closure of the connection. This may not clean up all resources.
>           """
>           self._logger.logger_exit()
>           self._close(force)
>   
>       @abstractmethod
>       def _close(self, force: bool = False) -> None:
> -        """
> -        Execute protocol specific steps needed to close the session properly.
> +        """Protocol specific steps needed to close the session properly.
> +
> +        Args:
> +            force: Force the closure of the connection. This may not clean up all resources.
> +                This doesn't have to be implemented in the overloaded method.
>           """
>   
>       @abstractmethod
>       def is_alive(self) -> bool:
> -        """
> -        Check whether the remote session is still responding.
> -        """
> +        """Check whether the remote session is still responding."""
>   
>       @abstractmethod
>       def copy_from(
> @@ -147,12 +207,12 @@ def copy_from(
>       ) -> None:
>           """Copy a file from the remote Node to the local filesystem.
>   
> -        Copy source_file from the remote Node associated with this remote
> -        session to destination_file on the local filesystem.
> +        Copy `source_file` from the remote Node associated with this remote session
> +        to `destination_file` on the local filesystem.
>   
>           Args:
> -            source_file: the file on the remote Node.
> -            destination_file: a file or directory path on the local filesystem.
> +            source_file: The file on the remote Node.
> +            destination_file: A file or directory path on the local filesystem.
>           """
>   
>       @abstractmethod
> @@ -163,10 +223,10 @@ def copy_to(
>       ) -> None:
>           """Copy a file from local filesystem to the remote Node.
>   
> -        Copy source_file from local filesystem to destination_file
> -        on the remote Node associated with this remote session.
> +        Copy `source_file` from local filesystem to `destination_file` on the remote Node
> +        associated with this remote session.
>   
>           Args:
> -            source_file: the file on the local filesystem.
> -            destination_file: a file or directory path on the remote Node.
> +            source_file: The file on the local filesystem.
> +            destination_file: A file or directory path on the remote Node.
>           """
> diff --git a/dts/framework/remote_session/ssh_session.py b/dts/framework/remote_session/ssh_session.py
> index cee11d14d6..7186490a9a 100644
> --- a/dts/framework/remote_session/ssh_session.py
> +++ b/dts/framework/remote_session/ssh_session.py
> @@ -1,6 +1,8 @@
>   # SPDX-License-Identifier: BSD-3-Clause
>   # Copyright(c) 2023 PANTHEON.tech s.r.o.
>   
> +"""SSH session remote session."""

Is the double "session" intended?

> +
>   import socket
>   import traceback
>   from pathlib import PurePath
> @@ -26,13 +28,8 @@
>   class SSHSession(RemoteSession):
>       """A persistent SSH connection to a remote Node.
>   
> -    The connection is implemented with the Fabric Python library.
> -
> -    Args:
> -        node_config: The configuration of the Node to connect to.
> -        session_name: The name of the session.
> -        logger: The logger used for logging.
> -            This should be passed from the parent OSSession.
> +    The connection is implemented with
> +    `the Fabric Python library <https://docs.fabfile.org/en/latest/>`_.
>   
>       Attributes:
>           session: The underlying Fabric SSH connection.
> @@ -80,6 +77,7 @@ def _connect(self) -> None:
>               raise SSHConnectionError(self.hostname, errors)
>   
>       def is_alive(self) -> bool:
> +        """Overrides :meth:`~.remote_session.RemoteSession.is_alive`."""
>           return self.session.is_connected
>   
>       def _send_command(
> @@ -89,7 +87,7 @@ def _send_command(
>   
>           Args:
>               command: The command to execute.
> -            timeout: Wait at most this many seconds for the execution to complete.
> +            timeout: Wait at most this long in seconds to execute the command.

Is the timeout actually to start running the command and not to wait for 
it to be completed?

>               env: Extra environment variables that will be used in command execution.
>   
>           Raises:
> @@ -118,6 +116,7 @@ def copy_from(
>           source_file: str | PurePath,
>           destination_file: str | PurePath,
>       ) -> None:
> +        """Overrides :meth:`~.remote_session.RemoteSession.copy_from`."""
>           self.session.get(str(destination_file), str(source_file))
>   
>       def copy_to(
> @@ -125,6 +124,7 @@ def copy_to(
>           source_file: str | PurePath,
>           destination_file: str | PurePath,
>       ) -> None:
> +        """Overrides :meth:`~.remote_session.RemoteSession.copy_to`."""
>           self.session.put(str(source_file), str(destination_file))
>   
>       def _close(self, force: bool = False) -> None: