-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add support for nested XML configuration #3
base: develop
Are you sure you want to change the base?
Conversation
When a `show conf | display xml` is performed on a juniper device, the configuration is provided as follows: ``` <rpc-reply xmlns:junos="http://xml.juniper.net/junos/15.1X49/junos"> <configuration junos:commit-seconds="1556580726" junos:commit-localtime="2019-04-29 23:32:06 UTC" junos:commit-user="vagrant"> - configuration XML here - </configuration> <cli> <banner></banner> </cli> </rpc-reply> ``` However, this module currently expected the "configuration" tag to be the top level tag. This minor modification searches for the configuration tag before continuing, and raises an AttributeError if it cannot be found (as the parsing would fail silently anyway if was not the case)
This doesn't actually work - there was a flaw in my testing. |
Changes now appear to be working as expected.
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.
thanks for your contribution, could you add some tests to avoid regressions?
… happy * Update test_models to allow multiple test cases for each test type * Add test case for the junos parser * Add failing test case for junos parser * Add raises test case for junos parser, to test new code in parsers/openconfig/junos/__init__.py
Test cases added |
Ok, played a bit with this and now I see why the copy is done. I think Re the tests, I am going to hold this PR while I rework the tests a bit, I think I need to get rid of all that magic and use a simple list instead. This is too clever for anyone's good. |
s/deepcopy/copy
Sounds good! |
parsed_xml = parsed_xml.find("configuration") | ||
if parsed_xml is None: | ||
raise AttributeError( | ||
"Unable to locate 'configuration' tag in XML blob" |
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.
We made changes to the testing so if you could rebase off the develop
branch and change the testing for these change to match the new way, that would be great.
I think we should also start creating our own exceptions for parsing errors rather than use built-in errors. Such as ntc_rosetta.exceptions.ConfigurationParsingError
, but this needs more discussion.
When a
show conf | display xml
is performed on a juniper device, the configuration is provided as follows:However, this module currently expected the "configuration" tag to be the top level tag. This minor modification searches for the configuration tag before continuing, and raises an AttributeError if it cannot be found (as the parsing would fail silently anyway if was not the case)