search

napalm-automation / napalm-ios

Apache License 2.0
31 stars 40 forks source link

Add allow_agent to accepted optional args #59

Closed XioNoX closed 7 years ago

XioNoX commented 7 years ago

Now that netmiko supports paramiko's "allow_agent" (see https://github.com/ktbyers/netmiko/releases/tag/v1.1.0 )

This patch is to add that option to the accepted optional args for napalm-ios

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 67.834% when pulling c066f04c1bc392a788f6dfd2588ffbaa82646462 on XioNoX:patch-1 into e636f2d0c92f784787f66f41451895d74c9229eb on napalm-automation:develop.

ktbyers commented 7 years ago

@XioNoX Your PR will fail if running netmiko 1.0. I think you should probably do a check for Netmiko version, if netmiko 1.0 then no allow_agent key, if netmiko >= 1.1 then allow_agent key in the dictionary.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 68.0% when pulling 46272374b6d1124de9f753f74742a05a017fc603 on XioNoX:patch-1 into e636f2d0c92f784787f66f41451895d74c9229eb on napalm-automation:develop.

ktbyers commented 7 years ago

@XioNoX Hmmmm don't like it (as you are counting on there never being a Netmiko version that used '10' for min_ver or bug_fix)...

 if int(maj_ver + min_ver + bug_fix) >= 110:
     netmiko_argument_map['allow_agent'] = False

How about this? I know it is a few more lines of code, but it will go away before whenever the Netmiko-napalm version requirement is increased.

fields = netmiko_version.split('.')
fields = [int(x) for x in fields]
maj_ver, min_ver, bug_fix = fields
if maj_ver >= 2:
    netmiko_argument_map['allow_agent'] = False
elif maj_ver == 1 and min_ver >= 1:
    netmiko_argument_map['allow_agent'] = False
coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 68.036% when pulling ca196ed6f7e13d001e9361fef932448c385d0bb5 on XioNoX:patch-1 into e636f2d0c92f784787f66f41451895d74c9229eb on napalm-automation:develop.

ktbyers commented 7 years ago

Reviewed by me.

dbarrosop commented 7 years ago

Two things:

  1. Out of curiosity, what does it do?
  2. Could you add a note here, please? https://napalm.readthedocs.io/en/latest/support/index.html#optional-arguments

Thanks!

XioNoX commented 7 years ago
  1. The existing "use_keys", will tell paramiko to look for private keys to use to ssh to a device The new "allow_agent" will check if you have an SSH agent running on your computer (usually an unlocked SSH key) and try to use it to ssh to a device
  2. https://github.com/napalm-automation/napalm/pull/314
dbarrosop commented 7 years ago

Thx for the clarification and the PR : )