From 0ee05ed448cae41d37fc2edb2a267acb4cdbcebd Mon Sep 17 00:00:00 2001
From: Diego Antonio Hurtado Pimentel <diego-antonio.hurtado-pimentel@hpe.com>
Date: Sun, 3 Sep 2017 17:05:09 -0600
Subject: [PATCH 1/3] fix: dev: Several fixes in the usage of the connection
 argument.

This intentionally breaks compatibility with Python 2.7 since it uses
syntax introduced in PEP 3102.
---
 lib/topology/platforms/shell.py       | 108 +++++++++++++++-----------
 test/test_topology_platforms_shell.py |  35 ++++-----
 2 files changed, 78 insertions(+), 65 deletions(-)

diff --git a/lib/topology/platforms/shell.py b/lib/topology/platforms/shell.py
index 7f27eea..f4bf420 100644
--- a/lib/topology/platforms/shell.py
+++ b/lib/topology/platforms/shell.py
@@ -235,7 +235,7 @@ def is_connected(self, connection=None):
         """
 
     @abstractmethod
-    def connect(self, connection=None, *args, **kwargs):
+    def connect(self, *args, connection=None, **kwargs):
         """
         Creates a connection to the shell.
 
@@ -248,7 +248,7 @@ def connect(self, connection=None, *args, **kwargs):
         """
 
     @abstractmethod
-    def disconnect(self, connection=None, *args, **kwargs):
+    def disconnect(self, *args, connection=None, **kwargs):
         """
         Terminates a connection to the shell.
 
@@ -281,7 +281,7 @@ def execute(self, command, connection=None):
     def __call__(self, command, connection=None):
         return self.execute(command, connection=connection)
 
-    def _setup_shell(self, connection=None, *args, **kwargs):
+    def _setup_shell(self, *args, connection=None, **kwargs):
         """
         Method called by subclasses that will be triggered after matching the
         initial prompt.
@@ -365,13 +365,14 @@ class PExpectShell(BaseShell):
     """
 
     def __init__(
-            self, prompt,
-            initial_command=None, initial_prompt=None,
-            user=None, user_match='[uU]ser:',
-            password=None, password_match='[pP]assword:',
-            prefix=None, timeout=None, encoding='utf-8',
-            try_filter_echo=True, auto_connect=True,
-            spawn_args=None, errors='ignore', **kwargs):
+        self, prompt,
+        initial_command=None, initial_prompt=None,
+        user=None, user_match='[uU]ser:',
+        password=None, password_match='[pP]assword:',
+        prefix=None, timeout=None, encoding='utf-8',
+        try_filter_echo=True, auto_connect=True,
+        spawn_args=None, errors='ignore', **kwargs
+    ):
 
         self._connections = OrderedDict()
         self._default_connection = None
@@ -466,18 +467,18 @@ def send_command(
         # 1. Connection is missing
         # 2. Connection is disconnected
         if not self._auto_connect:
-            if not self.is_connected(connection):
-                raise DisconnectedError(connection)
+            if not self.is_connected(connection=connection):
+                raise DisconnectedError(connection=connection)
 
         # If auto connect is true, always reconnect unless:
         # 1. Connection already exists, and
         # 2. Connection is connected
         else:
             try:
-                if not self.is_connected(connection):
-                    self.connect(connection)
+                if not self.is_connected(connection=connection):
+                    self.connect(connection=connection)
             except NonExistingConnectionError:
-                self.connect(connection)
+                self.connect(connection=connection)
 
         spawn = self._get_connection(connection)
 
@@ -561,13 +562,15 @@ def is_connected(self, connection=None):
         spawn = self._get_connection(connection)
         return spawn.isalive()
 
-    def connect(self, connection=None, *args, **kwargs):
+    def connect(self, *args, connection=None, **kwargs):
         """
         See :meth:`BaseShell.connect` for more information.
         """
         connection = connection or self._default_connection or '0'
 
-        if connection in self._connections and self.is_connected(connection):
+        connection_is_present = connection in self._connections
+
+        if connection_is_present and self.is_connected(connection=connection):
             raise AlreadyConnectedError(connection)
 
         spawn = Spawn(
@@ -575,35 +578,46 @@ def connect(self, connection=None, *args, **kwargs):
             **self._spawn_args
         )
 
-        spawn.logfile_read = get_logger(
-            OrderedDict([
-                ('node_identifier', self._node_identifier),
-                ('shell_name', self._shell_name),
-                ('connection', connection)
-            ]),
-            category='pexpect_read',
-        )
+        # If the disconnect is called on a connection and then connect is
+        # called again on the same connection, this will be called twice,
+        # making each message from that connection to be logged twice.
+        if connection_is_present:
+            present_connection = self._connections[connection]
 
-        spawn.logfile_send = get_logger(
-            OrderedDict([
-                ('node_identifier', self._node_identifier),
-                ('shell_name', self._shell_name),
-                ('connection', connection)
-            ]),
-            category='pexpect_send',
-        )
+            spawn.logfile_read = present_connection.logfile_read
+            spawn.logfile_send = present_connection.logfile_send
+            spawn._connection_logger = present_connection._connection_logger
 
-        # Add a connection logger
-        # Note: self._node and self._name were added to this shell in the
-        #       node's call to its _register_shell method.
-        spawn._connection_logger = get_logger(
-            OrderedDict([
-                ('node_identifier', self._node_identifier),
-                ('shell_name', self._shell_name),
-                ('connection', connection)
-            ]),
-            category='connection'
-        )
+        else:
+            spawn.logfile_read = get_logger(
+                OrderedDict([
+                    ('node_identifier', self._node_identifier),
+                    ('shell_name', self._shell_name),
+                    ('connection', connection)
+                ]),
+                category='pexpect_read',
+            )
+
+            spawn.logfile_send = get_logger(
+                OrderedDict([
+                    ('node_identifier', self._node_identifier),
+                    ('shell_name', self._shell_name),
+                    ('connection', connection)
+                ]),
+                category='pexpect_send',
+            )
+
+            # Add a connection logger
+            # Note: self._node and self._name were added to this shell in the
+            #       node's call to its _register_shell method.
+            spawn._connection_logger = get_logger(
+                OrderedDict([
+                    ('node_identifier', self._node_identifier),
+                    ('shell_name', self._shell_name),
+                    ('connection', connection)
+                ]),
+                category='connection'
+            )
 
         self._connections[connection] = spawn
 
@@ -625,7 +639,7 @@ def expect_sendline(prompt, command):
             expect_sendline(self._initial_prompt, self._initial_command)
 
             # Setup shell before using it
-            self._setup_shell(connection)
+            self._setup_shell(*args, connection=connection, **kwargs)
 
             # Wait for command response to match the prompt
             spawn.expect(
@@ -641,7 +655,7 @@ def expect_sendline(prompt, command):
         if self.default_connection is None:
             self.default_connection = connection
 
-    def disconnect(self, connection=None, *args, **kwargs):
+    def disconnect(self, *args, connection=None, **kwargs):
         """
         See :meth:`BaseShell.disconnect` for more information.
         """
@@ -709,7 +723,7 @@ def __init__(
             **kwargs
         )
 
-    def _setup_shell(self, connection=None, *args, **kwargs):
+    def _setup_shell(self, *args, connection=None, **kwargs):
         """
         Overriden setup function that will disable the echo on the device on
         the shell and set a pexpect-safe prompt.
diff --git a/test/test_topology_platforms_shell.py b/test/test_topology_platforms_shell.py
index c73d1a7..dae3eef 100644
--- a/test/test_topology_platforms_shell.py
+++ b/test/test_topology_platforms_shell.py
@@ -331,7 +331,7 @@ def test_connect(spawn, shell):
     class SetupShellError(Exception):
         pass
 
-    def _setup_shell(self, connection=None):
+    def _setup_shell(*args, connection=None, **kwargs):
         raise SetupShellError
 
     shell._setup_shell = _setup_shell
@@ -344,30 +344,29 @@ def test_connect_disconnect_connect(spawn, shell):
     """
     Test that the connect - disconnect - connect use case works properly.
     """
-    for connection in [None, '1']:
+    for connection in ['0', '1']:
 
-        # Shell not created yet
+        # Connection not created yet
         with raises(NonExistingConnectionError):
             shell.is_connected(connection=connection)
 
         # First shell call and explicit reconnection case
-        for i in [1, 2]:
-            shell.connect(connection=connection)
-            assert shell.is_connected(connection=connection)
+        shell.connect(connection=connection)
 
-            shell.send_command('command'.format(i), connection=connection)
-            shell._connections[connection or '0'].sendline.assert_called_with(
-                'command'.format(i)
-            )
-            assert shell.is_connected(connection=connection)
+        assert shell.is_connected(connection=connection)
 
-            shell.disconnect(connection=connection)
-            assert not shell.is_connected(connection=connection)
+        shell.send_command('command 0', connection=connection)
+
+        shell._connections[connection].sendline.assert_called_with('command 0')
+
+        shell.disconnect(connection=connection)
 
-        # Second case, automatic reconnect
         assert not shell.is_connected(connection=connection)
-        shell.send_command('a call to'.format(i), connection=connection)
-        shell._connections[connection or '0'].sendline.assert_called_with(
-            'a call to'
-        )
+
+        # Second case, automatic reconnect
+
+        shell.send_command('command 1', connection=connection)
+
+        shell._connections[connection].sendline.assert_called_with('command 1')
+
         assert shell.is_connected(connection=connection)

From e7bbdc9e788b73e9b63706d206a384f1fc5e78db Mon Sep 17 00:00:00 2001
From: Diego Antonio Hurtado Pimentel <diego-antonio.hurtado-pimentel@hpe.com>
Date: Mon, 4 Sep 2017 16:28:07 -0600
Subject: [PATCH 2/3] fix: dev: Removing support for Python 2.7.

---
 requirements.py27.txt                 |  1 -
 test/test_topology_platforms_shell.py | 18 ++++++------------
 tox.ini                               |  7 +------
 3 files changed, 7 insertions(+), 19 deletions(-)
 delete mode 100644 requirements.py27.txt

diff --git a/requirements.py27.txt b/requirements.py27.txt
deleted file mode 100644
index 14fe0d3..0000000
--- a/requirements.py27.txt
+++ /dev/null
@@ -1 +0,0 @@
-mock==2.0.0
diff --git a/test/test_topology_platforms_shell.py b/test/test_topology_platforms_shell.py
index dae3eef..4c9f092 100644
--- a/test/test_topology_platforms_shell.py
+++ b/test/test_topology_platforms_shell.py
@@ -22,18 +22,6 @@
 from __future__ import unicode_literals, absolute_import
 from __future__ import print_function, division
 
-# mock is located in unittest from Python 3.3 onwards, but as an external
-# package in Python 2.7, that is why the following is done:
-try:
-    from unittest.mock import patch, Mock, call
-
-    # Work around for http://bugs.python.org/issue25532
-    # Prevents infinite memory allocation
-    call.__wrapped__ = None
-
-except ImportError:
-    from mock import patch, Mock, call
-
 from pytest import fixture, raises
 
 from topology.platforms.shell import (
@@ -41,6 +29,12 @@
     DisconnectedError
 )
 
+from unittest.mock import patch, Mock, call
+
+# Work around for http://bugs.python.org/issue25532
+# Prevents infinite memory allocation
+call.__wrapped__ = None
+
 
 class Shell(PExpectBashShell):
     def __init__(self, prompt, **kwargs):
diff --git a/tox.ini b/tox.ini
index b1f68be..f149547 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,5 +1,5 @@
 [tox]
-envlist = py27, py3, coverage, doc
+envlist = py3, coverage, doc
 
 [testenv]
 passenv = http_proxy https_proxy
@@ -30,11 +30,6 @@ commands =
         {posargs:--topology-platform debug} \
         {toxinidir}/test {envsitepackagesdir}/topology
 
-[testenv:py27]
-deps =
-    -rrequirements.dev.txt
-    -rrequirements.py27.txt
-
 [testenv:doc]
 basepython = python3
 deps =

From 8a9af5df536daaaee635b4189e87bee2fda71848 Mon Sep 17 00:00:00 2001
From: Diego Antonio Hurtado Pimentel <diego-antonio.hurtado-pimentel@hpe.com>
Date: Mon, 4 Sep 2017 18:10:12 -0600
Subject: [PATCH 3/3] fix: dev: Refactoring _get_connection.

---
 lib/topology/platforms/shell.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/topology/platforms/shell.py b/lib/topology/platforms/shell.py
index f4bf420..bf2a40f 100644
--- a/lib/topology/platforms/shell.py
+++ b/lib/topology/platforms/shell.py
@@ -435,7 +435,7 @@ def _get_connect_command(self):
         :return: The command to be used when connecting to the shell.
         """
 
-    def _get_connection(self, connection):
+    def _get_connection(self, connection=None):
         """
         Get the pexpect object for the given connection name.
 
@@ -468,7 +468,7 @@ def send_command(
         # 2. Connection is disconnected
         if not self._auto_connect:
             if not self.is_connected(connection=connection):
-                raise DisconnectedError(connection=connection)
+                raise DisconnectedError(connection)
 
         # If auto connect is true, always reconnect unless:
         # 1. Connection already exists, and
@@ -480,7 +480,7 @@ def send_command(
             except NonExistingConnectionError:
                 self.connect(connection=connection)
 
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
 
         # Create possible expect matches
         if matches is None:
@@ -521,7 +521,7 @@ def get_response(self, connection=None, silent=False):
         See :meth:`BaseShell.get_response` for more information.
         """
         # Get connection
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
 
         # Convert binary representation to unicode using encoding
         text = spawn.before.decode(
@@ -559,7 +559,7 @@ def is_connected(self, connection=None):
         See :meth:`BaseShell.is_connected` for more information.
         """
         # Get connection
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
         return spawn.isalive()
 
     def connect(self, *args, connection=None, **kwargs):
@@ -660,7 +660,7 @@ def disconnect(self, *args, connection=None, **kwargs):
         See :meth:`BaseShell.disconnect` for more information.
         """
         # Get connection
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
         if not spawn.isalive():
             raise AlreadyDisconnectedError(connection)
         spawn.close()
@@ -728,7 +728,7 @@ def _setup_shell(self, *args, connection=None, **kwargs):
         Overriden setup function that will disable the echo on the device on
         the shell and set a pexpect-safe prompt.
         """
-        spawn = self._get_connection(connection)
+        spawn = self._get_connection(connection=connection)
 
         # Wait initial prompt
         spawn.expect(