Code dup #33

Ali-aqrabawi · 2019-05-18T15:56:44Z

Description

code refactoring:

added new module connection:
which has the protocol connection classes like ssh, telnet and serial.
this approach is needed to split the business logic from external libraries , so we can easily change libraries without changing our code. also this is needed to add different connection types.
added terminal_modes module:
objects that handle entering and exiting from different terminal modes like config_mode , enable_mode ... etc.
this approach is to eliminate code duplication in the Devices classes.

those are the major changes,
there are some changes on the device level, like adding methods, removing methods ... etc.

a new main method added to BaseDevice is _session_preparation , which will be called to prepare each device session.

new features:

telnet support.
textfsm.

selfuryon · 2019-05-18T16:00:47Z

Thank you for refactoring the code!
I'm going to test it this weekend and plan to merge it on Monday if all is fine!

Ali-aqrabawi · 2019-05-18T16:03:08Z

Thanks, please note that this pr has vt100 as term_mod, you can change it from constants.py module.

selfuryon · 2019-05-18T16:04:40Z

If I have any problems with that on Mikrotik I think I change it only for Mikrotik.
I test it separately. Thank you for the information!

selfuryon

Hello!
Some notes about the code

netdev/contants.py

netdev/_textfsm/_clitable.py

netdev/_textfsm/_terminal.py

netdev/connections/interface.py

netdev/connections/base.py

netdev/connections/ssh.py

netdev/vendors/devices/base.py

Ali-aqrabawi · 2019-05-18T19:46:47Z

@selfuryon please have a look at the updated pr.
you need to pip install textfsm before running the code, i could not find requirements.txt file to add it.

netdev/vendors/devices/base.py

selfuryon · 2019-05-18T20:03:37Z

@selfuryon please have a look at the updated pr.
you need to pip install textfsm before running the code, I could not find requirements.txt file to add it.

Yeah, I switched to use poetry for building the project and for dependency management. You can add it to pyproject.toml. This file is used not only poetry right now. You can find more information about that in PEP517 and PEP518

selfuryon · 2019-05-18T20:10:36Z

Thanks for new commits! I'll check it more deeply tomorrow!

…nection

selfuryon

Some notes.
Today I'm going to perform full testing for all devices what I have.

selfuryon · 2019-05-19T12:25:35Z

netdev/connections/ssh.py

+        self._conn.close()
+        await self._conn.wait_closed()
+
+    def send(self, cmd):


We have some differences here.
Code here is def send(self, cmd), but in parent class is async def send(self, cmd). We need to do the same for removing the ambiguous.
I think that we can leave it a regular function like asyncssh's authors and asyncio's authors made it

selfuryon · 2019-05-19T13:07:52Z

netdev/connections/telnet.py

+
+    def __check_session(self):
+        if not self._stdin:
+            raise RuntimeError("SSH session not started")


Need to correct it from "SSH session" to "Telnet Session"

selfuryon · 2019-05-19T13:12:35Z

netdev/connections/telnet.py

+        """ Establish Telnet Connection """
+        self._logger.info("Host {}: telnet: Establishing Telnet Connection on port {}".format(self._host, self._port))
+        try:
+            self._stdout, self._stdin = await asyncio.open_connection(self._host, self._port, family=0, flags=0)


In SSH we have this code:

fut = asyncssh.connect(**self._conn_dict) try: self._conn = await asyncio.wait_for(fut, self._timeout)

So we can manage the timeout for this operation. But when we use telnet we can't. I think we should use the same approach like in SSH

selfuryon · 2019-05-19T13:15:18Z

netdev/connections/telnet.py

+        self._conn.close()
+        await self._conn.wait_closed()
+
+    def send(self, cmd):


The same thing like in here.
I'd prefer to leave it as a regular function, not a coroutine.

selfuryon · 2019-05-19T13:24:59Z

netdev/vendors/devices/aruba/aruba_aos_6.py

@@ -30,7 +29,7 @@ class ArubaAOS6(IOSLikeDevice):

        For Aruba AOS 6 devices base_pattern is "(prompt) (\(.*?\))?\s?[#|>]
        """
-        logger.info("Host {}: Setting base prompt".format(self._host))
+        self._logger.info("Host {}: Setting base prompt".format(self.host))


There is an interesting thing here. In BaseConnection we use self._host format but in BaseDevice we use self.host format. Maybe we should unify it?
If we want to use self.host should we use the same format for port, protocol and so on?

selfuryon · 2019-05-19T13:44:48Z

netdev/vendors/devices/ios_like.py

+
+        self.current_terminal = None  # State Machine for the current Terminal mode of the session
+
+        self.enable_mode = EnableMode(


I think we should transfer the default parameters like enter_command, _priv_enter and so on to EnableMode and ConfigMode classes.
This class shouldn't contain any related commands for different terminal modes.

I don't think that the user needs to correct this because they are constants for particular devices.

selfuryon · 2019-05-19T13:46:59Z

netdev/vendors/devices/ios_like.py

+
+    async def _session_preparation(self):
+        await super()._session_preparation()
+        await self.enable_mode()


Should be self.enable_mode.enter()?
I saw that you use this code for BaseTerminalMode:

async def __call__(self): """ callable terminal to enter """ return await self.enter()

But I think it too ambiguously: I tried to search a function but I know that it's a class. it may confuse others

selfuryon · 2019-05-19T13:53:35Z

netdev/vendors/devices/ios_like.py

+            return ""
+
+        # Send config commands
+        output = await self.config_mode()


Should be self.config_mode.enter()?
The same thing like here

selfuryon · 2019-05-19T13:54:38Z

netdev/vendors/devices/ios_like.py

+        output += await super().send_config_set(config_commands=config_commands)
+
+        if exit_config_mode:
+            output += await self.config_mode.exit()


See, we use directly self.config_mode.exit(), but for entering we use self.config_mode() instead of self.config_mode.enter().
It confuses.

selfuryon · 2019-05-19T13:58:36Z

netdev/vendors/devices/junos_like.py


        if exit_config_mode:
-            output += await self.exit_config_mode()
+            output += await self.config_mode.exit()

        output = self._normalize_linefeeds(output)


Maybe we should transfer the normalize_linefeeds function to BaseConnection?
It's a very common function and it's related to a connection not to a device

Ali-aqrabawi · 2019-05-19T16:47:45Z

please check updated pr

selfuryon · 2019-05-19T18:03:15Z

I have some problems with HP Comware in testing. Need to go deeper

selfuryon · 2019-06-01T13:22:39Z

Hello!
I think for more than 2 weeks about the structure of code from this PR.
It's a good new structure and new approaches but it still has some lack of isolation which can confuse.

As example:

We have a duplication for device hierarchy in vendor and terminal_modes folders: both have cisco, hp, and others
Terminal modes classes have a link to a device class and vice versa. They call the other one in their work
There are exist a send_command in both classes terminal mode and device
All constants for terminal modes still are included in the device class
Device class tracks current terminal mode
We still have a three level of inheritance

So I decided to get your idea, take some techniques and class and make a refactor for the module in a slight different another way (see #34)

Ali-aqrabawi · 2019-06-01T14:26:22Z

make sense, looking forward to see the new structure.

rishrapsody · 2019-09-26T22:53:46Z

is there any way to get structured output using asyncssh netdev library?
it seems asyncio is not compatible with netconf?
Please provide a way to get structured output using asyncio netdev. I am trying to run this on Juniper vMX devices

Ali-aqrabawi and others added 7 commits May 16, 2019 01:58

new structure commit#1

d35388c

code structure commit#2

678ccee

code_dup commit#3

bc0ad63

added telnet support

2154475

added doc strings

8fb5f1f

setup logging

c030ec6

added _logger to rest of Devices

0b7147d

selfuryon self-assigned this May 18, 2019

selfuryon added the Improvement label May 18, 2019

selfuryon reviewed May 18, 2019

View reviewed changes

code review changes

376a53c

remove telnet_port from doc-string

406a762

selfuryon reviewed May 18, 2019

View reviewed changes

netdev/vendors/devices/base.py Outdated Show resolved Hide resolved

Ali-aqrabawi and others added 2 commits May 18, 2019 23:14

add textfsm to dependencies and remove unnecesserly vars from BaseCon…

1b93264

…nection

resolve relative import of constant module

29ca346

selfuryon reviewed May 19, 2019

View reviewed changes

This was referenced May 19, 2019

await send_command not accepting 'use_textfsm=True' #15

Open

Add telnet support #18

Open

code review update

c97dfac

raise netdev.TimeoutError in telnet Connection

69d5b6f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code dup #33

Code dup #33

Ali-aqrabawi commented May 18, 2019

selfuryon commented May 18, 2019

Ali-aqrabawi commented May 18, 2019

selfuryon commented May 18, 2019

selfuryon left a comment

Ali-aqrabawi commented May 18, 2019 •

edited

Loading

selfuryon commented May 18, 2019

selfuryon commented May 18, 2019

selfuryon left a comment

selfuryon May 19, 2019

selfuryon May 19, 2019

selfuryon May 19, 2019

selfuryon May 19, 2019

selfuryon May 19, 2019

selfuryon May 19, 2019

selfuryon May 19, 2019

selfuryon May 19, 2019

selfuryon May 19, 2019

selfuryon May 19, 2019

Ali-aqrabawi commented May 19, 2019

selfuryon commented May 19, 2019

selfuryon commented Jun 1, 2019

Ali-aqrabawi commented Jun 1, 2019

rishrapsody commented Sep 26, 2019


		self.current_terminal = None # State Machine for the current Terminal mode of the session

		self.enable_mode = EnableMode(

Code dup #33

Are you sure you want to change the base?

Code dup #33

Conversation

Ali-aqrabawi commented May 18, 2019

selfuryon commented May 18, 2019

Ali-aqrabawi commented May 18, 2019

selfuryon commented May 18, 2019

selfuryon left a comment

Choose a reason for hiding this comment

Ali-aqrabawi commented May 18, 2019 • edited Loading

selfuryon commented May 18, 2019

selfuryon commented May 18, 2019

selfuryon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ali-aqrabawi commented May 19, 2019

selfuryon commented May 19, 2019

selfuryon commented Jun 1, 2019

Ali-aqrabawi commented Jun 1, 2019

rishrapsody commented Sep 26, 2019

Ali-aqrabawi commented May 18, 2019 •

edited

Loading