Skip to content
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

Merged
merged 8 commits into from
Jul 13, 2020
Merged

fix: force_no_enable enabled on ios and nx only #1240

merged 8 commits into from
Jul 13, 2020

Conversation

tynany
Copy link
Contributor

@tynany tynany commented Jun 22, 2020

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 the force_no_enable method has been implemented.

This MR only enables force_no_enable for the Cisco devices types (cisco_ios_telnet, cisco_ios and cisco_nxos).

Below is an example error output of a NAPALM driver that uses _netmiko_open but does not implement the force_no_enable method:

'$insertNAPALMDriveName' object has no attribute 'force_no_enable'

@ktbyers
Copy link
Contributor

ktbyers commented Jun 22, 2020

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.

@tynany
Copy link
Contributor Author

tynany commented Jun 22, 2020

Good call -- try/except is a much better idea. I have pushed a new commit

Copy link
Member

@mirceaulinic mirceaulinic left a 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

@mirceaulinic
Copy link
Member

Bump @tynany

@coveralls
Copy link

coveralls commented Jul 8, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling 57a5997 on tynany:develop into 052ced8 on napalm-automation:develop.

@tynany
Copy link
Contributor Author

tynany commented Jul 8, 2020

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:
Copy link
Member

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()

Copy link
Member

Choose a reason for hiding this comment

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

Any thoughts on this @tynany / @ktbyers ?

Copy link
Contributor

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.

Copy link
Contributor

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.

@mirceaulinic mirceaulinic merged commit 6529ad1 into napalm-automation:develop Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants