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.

Sign up for GitHub

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

Jump to bottom

new panos module to import software or configuration file into firewall #19901

Merged
merged 2 commits into from Feb 1, 2017
Merged

new panos module to import software or configuration file into firewall #19901

merged 2 commits into from Feb 1, 2017

Conversation

ivanbojer
Copy link
Contributor

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

panos_import

ANSIBLE VERSION
ansible 2.1.0.0

config file =

configured module search path = Default w/o overrides
SUMMARY

New panos module that allows for import of the configuration files or software releases onto firewall.

@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. networking Network category new_module This PR includes a new module. new_plugin This PR includes a new plugin. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 4, 2017
privateip
privateip suggested changes Jan 5, 2017
View reviewed changes
lib/ansible/modules/network/panos/panos_import.py Outdated
category: software
'''

RETURN = '''
Copy link
Contributor

Choose a reason for hiding this comment

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

doc string doesn't match actual return data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No return so I copied default return string from wakeonlan.py

lib/ansible/modules/network/panos/panos_import.py Outdated
module.fail_json(msg='pan-python, requests, and requests_toolbelt are required for this module')

ip_address = module.params["ip_address"]
if not ip_address:
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

lib/ansible/modules/network/panos/panos_import.py Outdated
if not ip_address:
module.fail_json(msg="ip_address should be specified")
password = module.params["password"]
if not password:
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

lib/ansible/modules/network/panos/panos_import.py Outdated

file_ = module.params['file']
url = module.params['url']
if file_ is None and url is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

use required_one_of kwarg when creating AnsibleModule instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

lib/ansible/modules/network/panos/panos_import.py

try:
import pan.xapi
import requests
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using ansible.module_utils.urls instead of requests for better compatability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did but unfortunately it does not have all the pieces I need. It would also require code change that would make it incompatible with our stand-alone code paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you elaborate on what pieces its missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not find MultipartEncoder that we need. It also makes it tightly dependent on ANSIBLE package which makes it hard to share code with stand-alone modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@privateip Can you please let me know if you are ok with that?

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jan 6, 2017
@bcoca bcoca changed the title new module to import software or configuration file into firewall new panos module to import software or configuration file into firewall Jan 6, 2017
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Jan 6, 2017
@ivanbojer
Copy link
Contributor Author

Thank you for the review. I've incorporated all the changes except request library per comments above.

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. community_review In order to be merged, this PR must follow the community review workflow. labels Jan 10, 2017
@ivanbojer
Copy link
Contributor Author

ivanbojer commented Jan 27, 2017 •
edited

Added new module to changelog as requested

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 27, 2017
gundalow
gundalow requested changes Jan 29, 2017
View reviewed changes
lib/ansible/modules/network/panos/panos_import.py Outdated
options:
ip_address:
description:
- IP address (or hostname) of PAN-OS device
Copy link
Contributor

Choose a reason for hiding this comment

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

All description: fields should be full sentences, capital letters and full stops.

Copy link
Contributor Author

@ivanbojer ivanbojer Jan 30, 2017 •
edited

Choose a reason for hiding this comment

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

Thank you for the review. I changed descriptions per request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gundalow Can you please let me know if you are ok with the change?

Ivan Bojer added 2 commits January 31, 2017 17:56
# This is a combination of 6 commits.
41ba6d9 
# The first commit's message is:
new module to import software or configuration file onto firewall

# This is the 2nd commit message:

changes based on the review comments; remove unecessary if statements; change returned value docstring

# This is the 3rd commit message:

empty checkin to trigger ANSIBLEbot

# This is the 4th commit message:

added additional exception handling

# This is the 5th commit message:

- added new module info to the changelog as requested

# This is the 6th commit message:

removed blank space as tox checks were failing
new module to import software or configuration file onto firewall
162ab36
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Feb 1, 2017
@ivanbojer
Copy link
Contributor Author

ready_for_review

gundalow
gundalow approved these changes Feb 1, 2017
View reviewed changes
Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

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

shipit

@gundalow gundalow merged commit 327ad42 into ansible:devel Feb 1, 2017
@gundalow
Copy link
Contributor

gundalow commented Feb 1, 2017

Peter is happy, so merging

@ivanbojer ivanbojer deleted the panos_import branch February 2, 2017 21:00
@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.
Reviewers

@privateip privateip privateip requested changes

@gundalow gundalow gundalow approved these changes

Assignees
No one assigned
Labels
affects_2.3 This issue/PR affects Ansible v2.3 module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. networking Network category new_module This PR includes a new module. new_plugin This PR includes a new plugin.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ivanbojer @gundalow @privateip @bcoca @ansibot