Skip to content
This repository has been archived by the owner on May 15, 2019. It is now read-only.

add post-process-filter attribute to text parser #59

Merged
merged 6 commits into from
Jun 11, 2017

Conversation

matejv
Copy link
Contributor

@matejv matejv commented Jun 7, 2017

As discussed on NTC slack.

Just the most basic version for now. Looking forward for notes :)

One thing that came to mind would be ability to pass arguments to jinja filters in DSL. And support the same syntax as Jinja2. Something like:

- mode: block
  regexp: "(?P<block>router (?P<key>.+))^!$"
  post_process_filter: "truncate(64)"

Where truncate is the name of the filter called and 64 would be an extra argument passed to it along with the key.

What do you think?

@dbarrosop
Copy link
Member

Overall looks great :)

Where truncate is the name of the filter called and 64 would be an extra argument passed to it along with the key.

Sounds good, it's going to require some parsing but it should be fine.

@matejv
Copy link
Contributor Author

matejv commented Jun 9, 2017

As I don't really want to reinvent a Jinja2 template parser I tried to make the value under post_process_filter a template and let Jinja2 render it. So you could do something like:

- mode: block
  regexp: "(?P<block>router (?P<key>.+))^!$"
  post_process_filter: "{{ key|truncate(64) }}"

The value of post_process_filter gets passed to napalm_yang.helpers.template() function which does the rest. You can even use extra_vars in post_process_filter now. The result of the template rendering sets key

For this to work I had to make a change in helpers.py which I'm not sure is the most elegant. Please take a look and give me your opinion. If we don't like it, I have one more possible solution :)

@@ -30,6 +31,13 @@ def _parse_list_block(cls, mapping):
else:
key = extra_vars.pop("key")

if post_process_filter:
from napalm_yang.helpers import template
Copy link
Member

Choose a reason for hiding this comment

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

please, move the import to the top of the file for consistenct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't :( I'll get a circular import in that case, because of:

https://github.com/napalm-automation/napalm-yang/blob/dbarrosop/include/napalm_yang/helpers.py#L3

I can split helpers.py into two modules - one with functions dealing with YANG files/parsers (maybe keep them in helpers.py) and another that resolves/renders rules (move _resolve_rule(), resolve_rule()´ and template()´ into rules.py). Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I think the best solution would be to move get_parser to here then:

https://github.com/napalm-automation/napalm-yang/blob/dbarrosop/include/napalm_yang/parsers/__init__.py

@@ -30,6 +31,13 @@ def _parse_list_block(cls, mapping):
else:
key = extra_vars.pop("key")

if post_process_filter:
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind implementing this on the xmlparser as well, please?

@dbarrosop
Copy link
Member

Great idea :)

Just had a couple of tiny little comments. Would be great if you could add a note to the docs as well.

@matejv
Copy link
Contributor Author

matejv commented Jun 10, 2017

Thanks!

I'll do xmlparser and docs later today.

@dbarrosop dbarrosop changed the base branch from dbarrosop/include to develop June 10, 2017 11:11
@dbarrosop
Copy link
Member

I have also moved this PR to develop as I have merged my branch.

@matejv
Copy link
Contributor Author

matejv commented Jun 10, 2017

Done! (hopefully :)

Two questions regarding XMLParser:

@dbarrosop
Copy link
Member

When is element expected to be a dict here:

That was a backwards compatibility hack when I was migrating the parsers from dicts to lists. We could remove it now. I will probably rewrite the parsers and try to simplify them so don't worry about that.

Methods _parse_list_nested and _parse_list_container seem to be unused anywhere.


:)

@dbarrosop
Copy link
Member

This is great! Thanks for the PR :)

@dbarrosop dbarrosop merged commit d41976b into napalm-automation:develop Jun 11, 2017
@matejv
Copy link
Contributor Author

matejv commented Jun 12, 2017

Methods _parse_list_nested and _parse_list_container seem to be unused anywhere.


Ah!

Then I feel post_process_filter should be implemented for these two methods as well. Agreed? Should be simple for me to implement.

@dbarrosop
Copy link
Member

Yeah, makes sense. I am trying to improve the docs to spot this kind of inconsistencies and to make everything easier to understand (current docs are terrible). Still struggling so feedback is welcome:

#62

bewing added a commit to bewing/napalm-yang that referenced this pull request Jun 17, 2017
* develop: (74 commits)
  add post-process-filter attribute to text parser (napalm-automation#59)
  Allow each parser to modify native on start (napalm-automation#28)
  Updated CHANGELOG
  Updated docs
  Removed negate_recursively as it's unnecessary
  Added network-instance/junos translator
  Added reuse and replace_on_merge
  When using lists of actions allow advancing containers in xml
  Added _translate_leaf_map
  Added config parser for network-instances/junos
  Added missing key
  Various improvement to xml parser
  Minor fix to network-instance/eos
  Added tests for network-instance/eos
  Added network-instance parser/translator for EOS
  We can now remove elements recursively in cases where the nesting of YANG model doesnt reflect the nesting in native configuration
  Minor fixes to the logic of the lists
  Propagate attribute name rather than yang_name
  Adapted network instances model to lists
  Find translation point per rule as before
  ...

Conflicts:
	napalm_yang/helpers.py
	napalm_yang/parser.py
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants