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

Updated Audiocode Driver #2869

Merged
merged 25 commits into from
Aug 3, 2022
Merged

Updated Audiocode Driver #2869

merged 25 commits into from
Aug 3, 2022

Conversation

ktbyers
Copy link
Owner

@ktbyers ktbyers commented Jul 26, 2022

Fixes Type Hinting
Tries to make things more consistent with expected netmiko behavior
Restructures audiocode class hierarchy

@ktbyers
Copy link
Owner Author

ktbyers commented Jul 26, 2022

@Gatorjosh14 I updated the PR adding the type hints here.

Other big/meaningful changes:

  • Created an Audiocode72 driver and corresponding audiocode_72 device_type. Basically Base in Netmiko means it is shared by all the drivers and I didn't want to put us into a bind later on if Audiocode changed this.
  • Restructured the class hierarchy to the following:
class AudiocodeBase(BaseConnection):
class Audiocode72Base(AudiocodeBase):
class Audiocode72Telnet(Audiocode72Base):
class Audiocode72SSH(Audiocode72Base):
class AudiocodeBase66(AudiocodeBase):
class Audiocode66SSH(AudiocodeBase66):
class Audiocode66Telnet(AudiocodeBase66):
class AudiocodeShellBase(NoEnable, AudiocodeBase):
class AudiocodeShellSSH(AudiocodeShellBase):
class AudiocodeShellTelnet(AudiocodeShellBase):

So AudiocodeBase is code that is generally common to the drivers. "72" specific things would be in the "72Base" driver and then similar things for "66" and "Shell" specific things.

The NoEnable is a multiple inheritance that let's me share the standard way for Netmiko handling devices that don't have an enable mode. It should be the equivalent of what you had previously (for the Shell classes).

  • Added all the type hints and got mypy to passing.
  • I simplified the code on _reload_device. I thought there was no good reason to have a reload_device argument. In other words, calling the method implies you want to reload the device. I ran into an issue on the Shell driver with _reload_device in that there was an argument reload_message="Resetting the board", that wasn't supported in the parent method. So we need to work out what this is supposed to do. I just commented that method out for now. I think I also eliminated the cleanup call in _reload_device (as if you were using a context manager, cleanup would be called twice). But am open to debates on this though.
  • I changed the disable_paging() methods to be more consistent with standard Netmiko behaviors (which is you never include the commands to enter/exit config mode into the config_commands argument and that Netmiko always handles these for you--though in the case of Audiocode this implies you have to add the config_mode_command argument.
  • Related to the above, I also set the enter_config_mode to be True for send_config_set i.e. you always have to enter config mode and if you don't provide config_mode_command Netmiko will raise an exception.
  • I eliminated the _device_terminal_exit method as I didn't see why it was needed. This is probably code that should be outside of the Netmiko driver (i.e. a function in your code to accomplish some particular purpose where you pass in the Netmiko object as the argument to the function).
  • I stripped out the docstring comments as these are in the BaseConnection class and I wouldn't update them in multiple places (i.e. they would potentially get out of date and not be valid). Also it clutters the code a lot so in general I try to put them only the BaseConnection classes.

@ktbyers
Copy link
Owner Author

ktbyers commented Jul 26, 2022

@Gatorjosh14 Feel free to review and let me know if you see issues. Once we decide the code is in a state that we both think is acceptable...it would be really good to run the unit tests one more time.

@ktbyers
Copy link
Owner Author

ktbyers commented Aug 2, 2022

@Gatorjosh14 Any update on this? We should really complete this before working on the next driver (the Fortinet driver).

If you are okay with the changes that I made then it would be great if you could run the Netmiko test suite again using this latest code (to make sure I didn't break anything).

@Gatorjosh14
Copy link
Contributor

I'll pull the code down, look it over and test it out. I'm fine with finishing this one up before Fortinet.

Copy link
Contributor

@Gatorjosh14 Gatorjosh14 left a comment

Choose a reason for hiding this comment

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

Looks good

@ktbyers ktbyers deleted the gj-audiocode2 branch February 28, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants