napalm-automation / napalm-nxos

Apache License 2.0
9 stars 21 forks source link

Add SSH originally from GG PR #101

Closed itdependsnetworks closed 7 years ago

itdependsnetworks commented 7 years ago

After talking yesterday I felt like I was missing something. This is a 90% clone from @GGabriele PR, just cleaned up to remove anything API like.

I think there was 14 getters and this is accounting for the 5 already in text format, and 2 simple getntp*. The other 7 getters are prepended by __get and left with the commands as a placeholder.

I suspect I am missing the point, but it was 2 hours to just do and I wasn't able to communicate this so I figure I would just do.

no idea on

pinging @ktbyers @dbarrosop and @mirceaulinic

ktbyers commented 7 years ago

@itdependsnetworks @GGabriele This looks reasonable. I think the next main item is to figure out how to get the unit testing working and to start adding real test data in.

We should probably move this into its own branch.

ktbyers commented 7 years ago

@itdependsnetworks A few items:

  1. Class should follow Python naming standard for classes so should be:

    NXOSSSHDriver

the multiple 'SSS' is slightly annoying, but that is hidden from users by get_network_driver so I think that is fine.

  1. This line in get_network_driver causes the load to fail

https://github.com/napalm-automation/napalm-base/blob/develop/napalm_base/__init__.py#L95

>>> from napalm_base import get_network_driver
>>> get_network_driver('nxos-ssh')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/kbyers/VENV/py27_venv/local/lib/python2.7/site-packages/napalm_base/__init__.py", line 103, in get_network_driver
    install_name=module_install_name
napalm_base.exceptions.ModuleImportError: Cannot import "napalm_nxosssh". Is the library installed?
  1. We need to make sure that we don't have any issues with PIP/Pypi with that separate package directory (at least if we release this prior to the napalm merge coming back together).
ktbyers commented 7 years ago

Here is a quick fix I made in napalm-base to fix the issue I mentioned in 2 above:

napalm-base]$ git diff
diff --git a/napalm_base/__init__.py b/napalm_base/__init__.py
index bce3d11..3a3b521 100644
--- a/napalm_base/__init__.py
+++ b/napalm_base/__init__.py
@@ -92,7 +92,12 @@ def get_network_driver(module_name, prepend=True):
         # Only lowercase allowed
         module_name = module_name.lower()
         # Try to not raise error when users requests IOS-XR for e.g.
-        module_install_name = module_name.replace('-', '')
+        if module_name in ['ios-xr', 'nx-os']:
+            module_install_name = module_name.replace('-', '')
+        elif '-ssh' in module_name:
+            module_install_name = module_name.replace('-ssh', '_ssh')
+        else:
+            module_install_name = module_name
         # Can also request using napalm_[SOMETHING]
         if 'napalm_' not in module_install_name and prepend is True:
             module_install_name = 'napalm_{name}'.format(name=module_install_name)

We should check whether there are any other cases besides 'ios-xr' and 'nx-os' where we would want the hyphen elimination behavior.

ktbyers commented 7 years ago

@itdependsnetworks I made a branch of this with some minor cleanup:

https://github.com/napalm-automation/napalm-nxos/commits/ssh_v2

itdependsnetworks commented 7 years ago

@ktbyers I actually call it like: driver = get_network_driver('nxos_ssh')

ktbyers commented 7 years ago

Yes, probably both of them to need to work i.e. nxos-ssh' andnxos_ssh`.

dbarrosop commented 7 years ago

We should probably move this into its own branch.

Agreed, I have created an nxos_ssh branch and changed the base of the PR.

dbarrosop commented 7 years ago

@ktbyers oh, I noticed you created another branch and merge the changes. Would you mind instead merging this PR into a dedicated one and applying your changes there? The only reason is to be able to merge and close the PR instead of having it lingering until we merge this into develop. I already created a branch and changed the base of this PR so you only have to merge and apply your changes on top.

dbarrosop commented 7 years ago

@itdependsnetworks thanks a lot by the way. This is pretty much what I meant although I wasn't able to express it appropriately :)

ktbyers commented 7 years ago

@dbarrosop Sounds good.

ktbyers commented 7 years ago

@itdependsnetworks @dbarrosop Closing this PR as this work is incorporated in the PR here:

https://github.com/napalm-automation/napalm-nxos/pull/103