search

napalm-automation-community / napalm-panos

NAPALM driver for PAN-OS
Apache License 2.0
29 stars 31 forks source link

Support passing a commit comment #78

Closed hkam40k closed 3 years ago

hkam40k commented 3 years ago
hkam40k commented 3 years ago

Related issue https://github.com/napalm-automation-community/napalm-panos/issues/77

hkam40k commented 3 years ago

The pylama dependency is not compatible with newest versions of pytest, which is why travis is failing. Pylama already seems to have a pending PR to fix that. Even with a downgraded version of pylama, something in the tests seems to break, but I don't think it is because of my changes. Perhaps the reviewer can try to run the tests too, and verify if its something because of my changes, or yet more dependency issues?

hkam40k commented 3 years ago

Figured it out. Has to do with NAPALM version, their modules are imported in a different way in different versions. I'll see if I can sort that out too.

hkam40k commented 3 years ago

Hi,

Investigating the version issues, I found out that latest NAPALM 2.x version - 2.5 - requires netmiko 2.4.2. The support for passing a commit comment came into netmiko in 3.3.2, which is supported in NAPALM in a later version. Starting from NAPALM 3.0 however, support for Python version <3.6 is dropped. Therefore in order to make passing commit comments work, I upgraded the minimum required versions of NAPALM and netmiko, while modifying the napalm-panos plugin to also drop support for Python <3.6, similar to NAPALM. Additionally, in requirements-dev.txt, the pytest version has to be downgraded for now, until its dependency version issues are resolved.

The latest commit includes a few upgrades to method signatures (even if the parameter itself is not yet supported/functional. The tests require the method signatures to be matching), which are compliant with newest version of NAPALM, so test would be already passing if minimumn NAPALM version was raised to 3.2.0.

The drop of Python version <3.6 can be a rather major change (although NAPALM already went there), what do you think of this proposal?

itdependsnetworks commented 3 years ago

I like it, I did not even realize the sanitized parameter was added. Give me a few days to spin up and give it a try, but looks straight forward overall.

hkam40k commented 3 years ago

I like it, I did not even realize the sanitized parameter was added. Give me a few days to spin up and give it a try, but looks straight forward overall.

Hello, have you had the time to review the PR? Anything you would like to change or discuss?

itdependsnetworks commented 3 years ago

Apologies, a few busy weekends in a row, should have time this weekend.

hkam40k commented 3 years ago

Hello, any progress or comments on this PR?

itdependsnetworks commented 3 years ago

Just tested, looks good

>>> # Set ip, user, passw
>>> from napalm import get_network_driver
>>> import napalm_panos
>>> napalm_panos
<module 'napalm_panos' from '/github/napalm-panos/napalm_panos/__init__.py'>
>>>
>>> # ^^ Is local instance
>>> driver = get_network_driver('panos')
>>>
>>> device = driver(ip, user, passw)
>>> device.open()
>>> len(device.get_config())
3
>>> len(device.get_facts())
8
>>> len(device.get_lldp_neighbors())
3
>>> len(device.get_interfaces_ip())
8
>>>
itdependsnetworks commented 3 years ago

Once tests pass here: https://github.com/napalm-automation-community/napalm-panos/pull/82 I will merge, which will close this PR