Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Change TextFSM Windows failure to a runtime failure (instead of an import failure) #803

Merged
merged 4 commits into from
Sep 15, 2020

Conversation

ktbyers
Copy link
Contributor

@ktbyers ktbyers commented Sep 15, 2020

Also I was getting an unrelated test failure related to there being no tests for show arp on the cisco_ftd.

It looks like these tests were never included with the original PR (and I don't have an FTD) so I just removed the cisco_ftd from the show arp in the index .

It basically was saying cisco_ftd is the same as cisco_asa for show arp, but then provided no data in the original PR. The other option is to just copy the cisco_asa show arp tests over for the FTD (unless someone can get the FTD test data).

Just let me know if you want me to switch over to that (copying the show arp ASA data) or I can just straight remove that index change. Not sure why tests are working on CI-CD system, but failed for me locally due to the FTD issue.

@ktbyers ktbyers changed the title Change TextFSM Windows failure to a runtime failure (instead of an import failure) [Do Not Merge] - Change TextFSM Windows failure to a runtime failure (instead of an import failure) Sep 15, 2020
@ktbyers
Copy link
Contributor Author

ktbyers commented Sep 15, 2020

Testing on Linux with ntc-templates installed, but no TextFSM installed:

In [1]: from ntc_templates import parse

In [2]: parse.parse_output()
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-2-40c518d91d75> in <module>
----> 1 parse.parse_output()

~/ntc-templates/lib/ntc_templates/parse.py in parse_output(platform, command, data)
     46 
     47 """
---> 48         raise ImportError(msg)
     49 
     50     template_dir = _get_template_dir()

ImportError: 
The TextFSM library is not currently supported on Windows. If you are NOT using Windows
you should be able to 'pip install textfsm' to fix this issue. If you are using Windows
then you will need to install the patch referenced here:

https://github.com/google/textfsm/pull/82

@ktbyers
Copy link
Contributor Author

ktbyers commented Sep 15, 2020

Follow-up action:

PyEZ library needs to eliminate the following (once ntc-templates releases this fix to pypi)

In setup.py:

https://github.com/Juniper/py-junos-eznc/blob/master/setup.py#L11-L15

# refer: https://github.com/Juniper/py-junos-eznc/issues/1015
# should be removed when textfsm releases >=1.1.1
if sys.platform == "win32":
    if "ntc_templates" in install_reqs:
        install_reqs.remove("ntc_templates")
        install_reqs.append("ntc_templates==1.4.1")
    install_reqs.append("textfsm==0.4.1")

@ktbyers
Copy link
Contributor Author

ktbyers commented Sep 15, 2020

Also the Juniper import no longer fails (on import):

# No TextFSM installed
$ pip list | grep textfsm

# Patched ntc-templates installed
$ pip list | grep ntc-templates
ntc-templates         1.5.0      /home/kbyers/ntc-templates/lib

$ pip list | grep junos
junos-eznc            2.5.3

$ ipython
Python 3.6.12 (default, Aug 31 2020, 18:56:18) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.16.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from jnpr.junos.factory.cmdtable import CMDTable

In [2]: 

@ktbyers ktbyers changed the title [Do Not Merge] - Change TextFSM Windows failure to a runtime failure (instead of an import failure) Change TextFSM Windows failure to a runtime failure (instead of an import failure) Sep 15, 2020
templates/index Outdated
@@ -146,7 +146,7 @@ cisco_asa_show_version.textfsm, .*, cisco_asa, sh[[ow]] ver[[sion]]
cisco_asa_show_route.textfsm, .*, cisco_asa, sh[[ow]] ro[[ute]]
cisco_asa_show_xlate.textfsm, .*, cisco_asa, sh[[ow]] x[[late]]
cisco_asa_show_name.textfsm, .*, cisco_asa, sh[[ow]] nam[[e]]
cisco_asa_show_arp.textfsm, .*, cisco_(asa|ftd), sh[[ow]] arp
Copy link
Contributor

Choose a reason for hiding this comment

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

I just ran tox on master and it passed the tests, as well as the last build on here shows as passing. Removing this will cause it to not work for FTD devices. The test case is currently covered in ASA, as we do not require testing both platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries at all...will restore the index to its original state

FYI, here is the test failure for FTD (using py.test -vv)

tests/test_testcases_exists.py::test_verify_parsed_and_reference_data_exists[tests/vyatta_vyos/show_interfaces] PASSED [ 99%]
tests/test_testcases_exists.py::test_verify_parsed_and_reference_data_exists[tests/watchguard_firebox/show_arp] PASSED [100%]

============================================ FAILURES =============================================
_____________ test_verify_parsed_and_reference_data_exists[tests/cisco_ftd/show_arp] ______________

mock_directory = 'tests/cisco_ftd/show_arp'

    @pytest.mark.parametrize("mock_directory", extract_index_data())
    def test_verify_parsed_and_reference_data_exists(mock_directory):
        """Verify that at least one test exists for all entries in the index file."""
        cases = f"{mock_directory}/*.raw"
        test_list = glob.glob(cases)
>       assert len(test_list) != 0, f"Could not find tests for {mock_directory}.textfsm"
E       AssertionError: Could not find tests for tests/cisco_ftd/show_arp.textfsm
E       assert 0 != 0
E        +  where 0 = len([])

tests/test_testcases_exists.py:41: AssertionError
================================= 1 failed, 972 passed in 23.26s ==================================

@ktbyers
Copy link
Contributor Author

ktbyers commented Sep 15, 2020

Also adding @vnitinv here.

FYI, @vnitinv we are a battling each other a bit on these changes.

In other words, PyEZ is breaking NAPALM and Nornir on Windows which is causing me to frequently pin to downrev PyEZ. You are now pinning to both downrev ntc-templates and downrev textfsm 0.4.1 in PyEZ.

I am hoping we can come up with an interim solution where we can cease the pinning to down rev versions of each others libraries (until TextFSM fixes the underlying issue with this PR here google/textfsm#82)

Note TextFSM 0.4.1 is fundamentally broken as it not packaged correctly (which causes a lot of negative side-effects). And the terminal.py library has not had a release since 2013 (and it really only works because it overwrites the namespace of TextFSM 0.4.1 due to it not being a package). Consequently, I don't view using TextFSM 0.4.1 + terminal.py as a recommended solution to this problem.

This fix proposed here would mean TextFSM on Windows would only break if you actually use TextFSM on Windows (via PyEZ). It would also lead the end-user to the fix that they need.

TextFSM would cease to fail when it was imported.

Netmiko/NAPALM/Nornir all have a separate fix where TextFSM on Windows works properly for them. Basically, I vendored part of TextFSM (in Netmiko) a few years back because of this issue.

Let me know if that is a reasonable fix from your perspective or not.

Regards, Kirk

@vnitinv
Copy link

vnitinv commented Sep 15, 2020

Sure @ktbyer. Can you point me to the fix in nornir/napalm. we can follow the same approach.

@rahkumar651991, we might need to work on this.

@ktbyers
Copy link
Contributor Author

ktbyers commented Sep 15, 2020

@vnitinv I basically versioned part of TextFSM library here such that when clitable gets used via Netmiko (and consequently via NAPALM/Nornir) that it uses this local TextFSM (to bypass the original import issue on Windows):

https://github.com/ktbyers/netmiko/tree/develop/netmiko/_textfsm

I will actually remove this if/when Google accepts that referenced PR (and pushes a new release to pypi).

And this in my Netmiko version of clitable.py

https://github.com/ktbyers/netmiko/blob/develop/netmiko/_textfsm/_clitable.py#L35

try:
    # TextFSM >= 1.0 (new package structure)
    from textfsm import copyable_regex_object
except ImportError:
    # TextFSM <= 0.4.1
    import copyable_regex_object
import textfsm
from netmiko._textfsm import _texttable as texttable

Note, the textfsm module is still being used from Google's TextFSM. IIRC, it is clitable where things break.

@jmcgill298
Copy link
Contributor

@ktbyers are we good to merge this here?

@ktbyers
Copy link
Contributor Author

ktbyers commented Sep 15, 2020

Yes, should be good to go.

@jmcgill298 jmcgill298 merged commit 95e7659 into networktocode:master Sep 15, 2020
thomasblass pushed a commit to thomasblass/ntc-templates that referenced this pull request Oct 25, 2020
jvanderaa pushed a commit that referenced this pull request Nov 10, 2021
guillaume-mbali pushed a commit to unyc-io/ntc-templates that referenced this pull request Apr 12, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants