On Tue, Nov 21, 2023 at 4:36 PM Yoan Picchi <yoan.pic...@foss.arm.com> wrote:
>
> 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.lin...@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.
>
What about this?
The implementation must except all exceptions and raise:
* SSHSessionDeadError if the session is not alive,
* SSHTimeoutError if the command execution times out.
> > """
> >
> > 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?
>
Not really, I'll remove the first occurence.
> > +
> > 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?
>
It is to wait for it to be completed. The wording is a bit confusing,
what about:
Wait at most this long in seconds for the command execution to complete.
I'll change this in all places where timeout is documented.
> > 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:
>
