-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Add ciscoconfparse execution module for various text config mainipulations #48733
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
Conversation
44f92fc to
e4d03b8
Compare
rallytime
left a comment
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.
I like this in general, but I think some of the function names could be a little more direct. Let me know what you think.
|
|
||
|
|
||
| def __virtual__(): | ||
| return HAS_CISCOCONFPARSE |
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.
Might be good to give the reason (missing dependency) if this virtual check fails, like we do in other modules.
|
|
||
|
|
||
| def _get_ccp(config=None, config_path=None, saltenv='base'): | ||
| ''' |
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.
Did you mean to add some docs here? I think that would be good. :)
| return [line.text for line in lines] | ||
|
|
||
|
|
||
| def find_objects_w_child(config=None, |
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.
Maybe find_child_objects? I think this function name is a bit awkward as it sits currently.
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.
I used this name to preserve the nomenclature of the underlying library, e.g., http://www.pennington.net/py/ciscoconfparse/tutorial_parent_child.html#method-3-find-objects-w-child
I agree that name is a bit awkward, but I'm happy to change it as suggested. I'll let you reply with your thoughts given my reasoning above, then I can update if necessary.
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.
Ah, OK. If that's the way people are used to using these, then that is fine.
| return lines | ||
|
|
||
|
|
||
| def find_lines_w_child(config=None, |
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.
Same comment here. Maybe find_child_lines?
| return [line.text for line in lines] | ||
|
|
||
|
|
||
| def find_objects_wo_child(config=None, |
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.
What about find_objects_no_child? I'm not sure if that's better, but I think the wo is a tad confusing.
| return lines | ||
|
|
||
|
|
||
| def find_lines_wo_child(config=None, |
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.
Same comment here: find_lines_no_child?
|
|
||
|
|
||
| @depends(HAS_CISCOCONFPARSE) | ||
| def config_lines_w_child(parent_regex, child_regex, source='running'): |
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.
I think the functions in this file could use some new names as well, just like above. I won't comment on each one though, you get the idea.
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.
Please see #48733 (comment) for my reasoning - happy to update if you think that's best - no problem at all!
Adding a simple execution module for various text manipulation when working with configuration style formatted according to a very poor "industry standard" Cisco IOS (space based hierarchy), but not limited to.
This helps identifying individual lines having a specific pattern, or if their children have a given pattern as well.
This can have unlimited use cases, and can be equally helpful from the command line, e.g., identify what interfaces are administratively shut down:
salt <net device> napalm.config_lines_w_child '^interface' '^\s?shutdown', as well as parsing various chunks of configuration from a Jinja template, etc.The functions added to the
napalmmodule are helpers to easier access the ciscoconfparse features applying directly on the running / startup / candidate config as preferred.Also updating the autodoc for the
napalmmodule (which I have renamed yesterday fromnapalm.pytonapalm_mod.py: #48707, but forgot to update the RST build).