-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: force_no_enable enabled on ios and nx only #1240
Conversation
Hmmm, how about we do a try/except and gracefully handle the AttributeError exception instead? I am pretty strongly against doing a platform conditional check. |
Good call -- try/except is a much better idea. I have pushed a new commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but Black seems unhappy about something, could you please check? https://travis-ci.org/github/napalm-automation/napalm/jobs/700735592
Bump @tynany |
Apologies for the delay in getting back to you. Travis is now happy -- https://travis-ci.org/github/napalm-automation/napalm/builds/706059614 |
if not self.force_no_enable: | ||
# Disable enable mode if force_no_enable is true (for NAPALM drivers | ||
# that support force_no_enable) | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I'm looking at this again, it does look a little strange, wondering if the following would be more readable:
if not getattr(self, 'force_no_enable', False):
self._netmiko_device.enable()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the try/except is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good to me so I am fine with merging it.
As per NAPALM documentation,
force_no_enable
is only used for Cisco NXOS and IOS devices, but is currently implemented for all NAPALM drivers that use_netmiko_open
, which breaks those drivers as it is unlikely theforce_no_enable
method has been implemented.This MR only enables
force_no_enable
for the Cisco devices types (cisco_ios_telnet
,cisco_ios
andcisco_nxos
).Below is an example error output of a NAPALM driver that uses
_netmiko_open
but does not implement theforce_no_enable
method: