Skip to content
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

adds jinja2 filters for working with network devices #24216

Closed
wants to merge 1 commit into from
Closed

adds jinja2 filters for working with network devices #24216

wants to merge 1 commit into from

Conversation

privateip
Copy link
Contributor

  • adds network textfsm filters
  • adds network config filter

@ansibot
Copy link
Contributor

ansibot commented May 2, 2017

@privateip Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information.

Here are the items we could not find in your description:

  • issue type

Please set the description of this pullrequest with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/PULL_REQUEST_TEMPLATE.md

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented May 2, 2017

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 c:plugins/filter docs_pull_request needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. needs_triage Needs a first human triage before being processed. labels May 2, 2017
* adds network textfsm filters
* adds network config filter

.. versionadded:: 2.4

To generate a list of command diffs for a network configuration:
Copy link
Contributor

Choose a reason for hiding this comment

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

For code blocks in RST you use ::, blank line, four spaces

e.g.

To generate a list of command diffs for a network configuration::

    {{ myvar | diff_network_config(current_config, indent=1 }}

from ansible.utils.display import Display
display = Display()

NETWORK_OS = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be called NETWORK_OS_INDENT, or similar

if url.scheme.startswith('http'):
try:
handler = {}
if 'HTTP_PROXY' in os.environ:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding some comments to say why you set the proxy to localhost.

@ansibot
Copy link
Contributor

ansibot commented May 2, 2017

The test ansible-test sanity --test ansible-var-precedence-check failed with the following error:

Command "test/sanity/code-smell/ansible-var-precedence-check.py" returned exit status 1.
>>> Standard Output
CHECKING: extra_vars (var passed via the cli)
ERROR !!!
playbook failed (rc=2), stdout: 'b'\nPLAY [testhost] ****************************************************************\n\nTASK [set_fact] ****************************************************************\nok: [testhost]\n\nTASK [include_vars] ************************************************************\nok: [testhost]\n\nTASK [debug] *******************************************************************\nAn exception occurred during task execution. To see the full traceback, use -vvv. The error was: ImportError: No module named \'urllib2\'\nfatal: [testhost]: FAILED! => {"failed": true, "msg": "Unexpected failure during module execution.", "stdout": ""}\n\tto retry, use: --limit @/tmp/tmp4ymr89uu/site.retry\n\nPLAY RECAP *********************************************************************\ntesthost                   : ok=2    changed=0    unreachable=0    failed=1   \n\n'' stderr: 'b'''
feature: extra_vars failed

The test ansible-test sanity --test pep8 failed with the following errors:

lib/ansible/plugins/filter/network.py:50:1: E302 expected 2 blank lines, found 1
lib/ansible/plugins/filter/network.py:67:1: E302 expected 2 blank lines, found 1
lib/ansible/plugins/filter/network.py:103:1: E302 expected 2 blank lines, found 1
lib/ansible/plugins/filter/network.py:106:17: E222 multiple spaces after operator

The test ansible-test sanity --test replace-urlopen failed with the following error:

Command "test/sanity/code-smell/replace-urlopen.sh" returned exit status 1.
>>> Standard Output
./lib/ansible/plugins/filter/network.py:            resp = urllib2.urlopen(template)
One or more file(s) listed above use urlopen.
Use open_url from module_utils instead of urlopen.

click here for bot help

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label May 2, 2017
@jedelman8
Copy link
Contributor

Glad to see TextFSM integration going into core.

Few questions for now.

  • Are you doing anything with respect a template repository? I hope not.
  • Are you open to having more options in the filter such as returning a list of dictionaries and normalizing the TextFSM object a bit more.
  • Are you open to using the clitable and add inputs such as template directory, command, and platform?


To generate a list of command diffs for a network configuration:

{{ myvar | diff_network_config(current_config, indent=1 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

need to close out the function: e.g.
{{ myvar | diff_network_config(current_config, indent=1 ) }}

Also might be a good idea to show example using a file lookup.

assert replace in ('line', 'block')
assert network_os in NETWORK_OS.keys()

if network_os:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how this would ever hit base on the previous assert? Perhaps the assert should be here?

@gundalow gundalow removed the needs_triage Needs a first human triage before being processed. label May 3, 2017
@gundalow gundalow added this to the 2.4.0 milestone May 3, 2017
@itdependsnetworks
Copy link
Contributor

Just want to confirm there is no namespace issue with filter_plugins, like there is with modules, e.g. the setup.py issue. Otherwise network.py may be too generic.

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label May 5, 2017
@privateip
Copy link
Contributor Author

@jedelman8

Are you doing anything with respect a template repository? I hope not.

No, nothing planned

Are you open to having more options in the filter such as returning a list of dictionaries and normalizing the TextFSM object a bit more.

Can you elaborate more what you are thinking here?

Are you open to using the clitable and add inputs such as template directory, command, and platform?

I'm not sure this is needed as we the same thing can be easily accomplished in the playbook unless I'm not completely understanding what you are asking here. WRT clitable sure

@privateip
Copy link
Contributor Author

@itdependsnetworks

Just want to confirm there is no namespace issue with filter_plugins, like there is with modules, e.g. the setup.py issue. Otherwise network.py may be too generic.

filters same as modules aren't namespaced, not sure what the concern here is

@gundalow
Copy link
Contributor

gundalow commented May 9, 2017

I wonder if it would be worth adding this to CHANGELOG.md as well

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jun 23, 2017
@privateip privateip closed this Jul 29, 2017
@privateip privateip deleted the network-filters-textfsm branch August 4, 2017 16:05
@ansibot ansibot added docs This issue/PR relates to or includes documentation. and removed docs_pull_request labels Mar 4, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 c:plugins/filter ci_verified Changes made in this PR are causing tests to fail. docs This issue/PR relates to or includes documentation. needs_info This issue requires further information. Please answer any outstanding questions. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants