Skip to content
This repository has been archived by the owner on Feb 10, 2018. It is now read-only.

Add ssh_keyfile as an optional argument. #145

Merged

Conversation

stiltzkin10
Copy link
Contributor

@stiltzkin10 stiltzkin10 commented Mar 31, 2017

Also change optional_args to default to an empty dict.

This is for issue/FR: #143

@@ -55,13 +55,14 @@
class JunOSDriver(NetworkDriver):
"""JunOSDriver class - inherits NetworkDriver from napalm_base."""

def __init__(self, hostname, username, password, timeout=60, optional_args=None):
def __init__(self, hostname, username, password='', timeout=60, optional_args={}):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to switch this back to optional_args=None (as otherwise we will have a problem with Python mutable).

@@ -70,12 +71,15 @@ def __init__(self, hostname, username, password, timeout=60, optional_args=None)
self.config_replace = False
self.locked = False

if optional_args is None:
optional_args = {}
self.port = optional_args.get('port', 22)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section should be retained as otherwise have a Python mutable problem.

@ktbyers
Copy link
Contributor

ktbyers commented Mar 31, 2017

I added a couple of comments about fixing the optional_args=None change. Also unit test failures need fixed...but I think that will fixed by the optional_args=None change. Also leave the password argument in __init__ as being required. I think that change probably needs to wait until we can make it uniform across all of the drivers.

Did you test validate that this change works using SSH keys.

@coveralls
Copy link

coveralls commented Mar 31, 2017

Coverage Status

Coverage decreased (-0.06%) to 83.898% when pulling 9a2ff79 on stiltzkin10:feature/junos_ssh_key into ebc0221 on napalm-automation:develop.

@stiltzkin10
Copy link
Contributor Author

Got it. Reverted back to optional_args=None. Thanks!

I have tested this and it works fine on the JUNOS devices I have access to.

To my knowledge the API does not support fall back to username/password.


self.device = Device(hostname, user=username, password=password, port=self.port)
if self.ssh_keyfile:
self.device = Device(hostname, user=username, ssh_private_key_file=self.ssh_keyfile)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need the port=self.port here.

@ktbyers
Copy link
Contributor

ktbyers commented Mar 31, 2017

Okay, I added one more comment about self.port missing at one point. I also think we should make the optional argument be key_file for consistency with the other drivers.

@stiltzkin10
Copy link
Contributor Author

Agree. Updated.

@ktbyers
Copy link
Contributor

ktbyers commented Mar 31, 2017

@stiltzkin10 This looks good to me.

Let me get @dbarrosop or @mirceaulinic to sanity check since they use Juniper driver more than I do.

@stiltzkin10
Copy link
Contributor Author

Yep, no worries. Thanks!

@coveralls
Copy link

coveralls commented Mar 31, 2017

Coverage Status

Coverage decreased (-0.06%) to 83.898% when pulling 7c8d967 on stiltzkin10:feature/junos_ssh_key into ebc0221 on napalm-automation:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 83.898% when pulling 7c8d967 on stiltzkin10:feature/junos_ssh_key into ebc0221 on napalm-automation:develop.

@dbarrosop
Copy link
Member

I don't use juniper devices anymore but I could confirm with TestJunOSDriver.py::TestConfigJunOSDriver that this PR works fine. If you have the key in the ssh-agent works as usual and if you don't you can pass the key_file arg. and work as promised.

Thanks! :)

@dbarrosop
Copy link
Member

Oh, just one thing. Would you mind updating the optional_args documentation in the napalm docs, please? Once that PR is sent link it here and I will merge both. Thanks again!

@stiltzkin10
Copy link
Contributor Author

stiltzkin10 commented Apr 10, 2017

Sure, I created napalm-automation/napalm#368. I actually created sub bullets since vyos/junos are slightly different.

@dbarrosop dbarrosop merged commit 7421f95 into napalm-automation:develop Apr 10, 2017
@stiltzkin10
Copy link
Contributor Author

Awesome, thanks!

@stiltzkin10 stiltzkin10 deleted the feature/junos_ssh_key branch April 10, 2017 08:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants