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

Improve panos_commit module #50451

Merged
merged 5 commits into from Jan 11, 2019

Conversation

Projects
None yet
4 participants
@traittinen
Copy link
Contributor

traittinen commented Jan 2, 2019

SUMMARY

Adding new features for panos_commit module

  • Add description/comment for commit job
  • Run commit only on changes done by specified admin
  • Run commit only for specific VSYS
  • Return commit job ID and API status information instead of "okey dokey"
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

panos_commit

ADDITIONAL INFORMATION

Sample usage:

   - name: Commit configuration on FW
      panos_commit:
        ip_address: "{{ pan_host }}"
        username: "{{ pan_user }}"
        password: "{{ pan_pw }}"
        interval: 2.0
        timeout: 90
        description: "Added by Ansible network automation"
        commit_vsys: "vsys{{ vsys_id }}"
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 2, 2019

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

lib/ansible/modules/network/panos/panos_commit.py:163:15: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/network/panos/panos_commit.py:175:23: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/network/panos/panos_commit.py:181:23: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/network/panos/panos_commit.py:199:4: bare-except No exception type(s) specified

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/network/panos/panos_commit.py:111:0: ImportError: No module named lxml

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/network/panos/panos_commit.py:111:0: ImportError: No module named lxml

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/network/panos/panos_commit.py:111:0: ImportError: No module named 'lxml'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/network/panos/panos_commit.py:111:0: ModuleNotFoundError: No module named 'lxml'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/network/panos/panos_commit.py:111:0: ModuleNotFoundError: No module named 'lxml'

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/network/panos/panos_commit.py:199:5: E722 do not use bare 'except'

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/network/panos/panos_commit.py:0:0: E324 Value for "default" from the argument_spec (None) for "username" does not match the documentation ('admin')

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 2, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 2, 2019

@traittinen, just so you are aware we have a dedicated Working Group for network.
You can find other people interested in this in #ansible-network on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 2, 2019

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/modules/network/panos/panos_commit.py:187:15: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/network/panos/panos_commit.py:199:23: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/modules/network/panos/panos_commit.py:205:23: ansible-format-automatic-specification Format string contains automatic field numbering specification

click here for bot help

@shinmog
Copy link
Contributor

shinmog left a comment

@traittinen

This is a cool update, thanks :)

However, you are adding a dependency of lxml in this implementation that does not already exist for the panos modules. This needs to be changed to use xml.etree.ElementTree like pan-python does.

Also, does committing changes for a specific admin code work for Panorama as well? I haven't played with partial commits yet, so I don't know the answer.

Show resolved Hide resolved lib/ansible/modules/network/panos/panos_commit.py Outdated
@traittinen

This comment has been minimized.

Copy link
Contributor

traittinen commented Jan 4, 2019

@shinmog

However, you are adding a dependency of lxml in this implementation that does not already exist for the panos modules. This needs to be changed to use xml.etree.ElementTree like pan-python does.

I understand that it's better to use xml.etree.ElementTree in this case since adding a new dependency for existing module could break something when upgrading to new version of Ansible. However, would it be possible to utilize lxml in new modules? I'm asking because I have ready panos_facts module I could possibly share, but that's using lxml (xpath) quite heavily.

Also, does committing changes for a specific admin code work for Panorama as well? I haven't played with partial commits yet, so I don't know the answer.

Yes, it seems to work.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 4, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/network/panos/panos_commit.py:0:0: E324 Value for "default" from the argument_spec ('admin') for "username" does not match the documentation (None)

click here for bot help

@ansibot ansibot added the ci_verified label Jan 4, 2019

@ansibot ansibot removed the ci_verified label Jan 4, 2019

@traittinen

This comment has been minimized.

Copy link
Contributor

traittinen commented Jan 9, 2019

@shinmog are you happy with the changes?

@shinmog
Copy link
Contributor

shinmog left a comment

lgtm 👍

@shinmog

This comment has been minimized.

Copy link
Contributor

shinmog commented Jan 10, 2019

@traittinen

To answer your question about lxml in future modules, I think no, unfortunately. lxml provides some niceties, which is not a high enough bar to warrant adding another dependency to the panos modules.

@ansibot ansibot added core_review and removed needs_revision labels Jan 10, 2019

@ganeshrn

This comment has been minimized.

Copy link
Member

ganeshrn commented Jan 11, 2019

rebuild_merge

@ansibot ansibot added shipit and removed core_review labels Jan 11, 2019

@ansibot ansibot merged commit d3107d9 into ansible:devel Jan 11, 2019

1 check passed

Shippable Run 101731 status is SUCCESS.
Details

@traittinen traittinen deleted the traittinen:panos_commit branch Jan 11, 2019

kbreit added a commit to kbreit/ansible that referenced this pull request Jan 11, 2019

Improve panos_commit module (ansible#50451)
* Added description, admin commit and VSYS commit features.

* pep8 fixing

* Ansible pylint fixing + module.fail on commit fail

* Ported from lxml.etree to xml.etree. Changed username to fall back to admin if nothing else defined.

* Fixed documentation for username defaults.

andmos added a commit to andmos/ansible that referenced this pull request Jan 12, 2019

Improve panos_commit module (ansible#50451)
* Added description, admin commit and VSYS commit features.

* pep8 fixing

* Ansible pylint fixing + module.fail on commit fail

* Ported from lxml.etree to xml.etree. Changed username to fall back to admin if nothing else defined.

* Fixed documentation for username defaults.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment