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

Fortinet FortiManager Connection Plugin RC, plus associated utilities #50336

Merged
merged 9 commits into from Feb 7, 2019
Merged

Fortinet FortiManager Connection Plugin RC, plus associated utilities #50336

merged 9 commits into from Feb 7, 2019

Conversation

ftntcorecse
Copy link
Contributor

@ftntcorecse ftntcorecse commented Dec 27, 2018 •
edited

SUMMARY

A while back, we were asked by @gdpak to re-write our modules to use the new httpapi plugin method. We have done so. This is our first PR/RC for the FortiManager Ansible Connection Plugin.

Please find the plugin file, as well as changes to module_utils/fortimanager.py, and the addition of module_utils/common.py.

Modules to run this on, are available on request. We have already converted all existing modules to these new plugins and tools, so we're ready to go.

Please note that backwards compatibility with module_utils/fortimanager.py is maintained for older modules, or older versions of Ansible, or for customers who don't want to upgrade to the new plugin.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

module_utils/network/fortimanager/common.py -- A lib of common procedures and static methods.

module_utils/network/fortimanager/fortimanager.py -- Updated to provide tools to work with the new connection plugin, as well as static code used throughout modules.

plugins/httpapi/fortimanager.py -- NEW Plugin for FortiManager.

ADDITIONAL INFORMATION

@ftntcorecse ftntcorecse changed the title PR Candidate for FortiManager Connection Plugin, plus associated utilities Fortinet FortiManager Connection Plugin RC, plus associated utilities Dec 27, 2018
@ansibot
Copy link
Contributor

ansibot commented Dec 27, 2018

cc @ftntcorecse @Ghilli3 @NilashishC @Qalthos @ganeshrn @justjais @lweighall @p4r4n0y1ng @rcarrillocruz @trishnaguha
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. networking Network category new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:network This issue/PR relates to code supported by the Ansible Network Team. labels Dec 27, 2018
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Dec 27, 2018
@spoyd spoyd mentioned this pull request Dec 29, 2018
Closed
@ftntcorecse
Update fortimanager.py
fd81425 
Adding additional comments
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 4, 2019
@Ghilli3
Copy link
Contributor

Ghilli3 commented Jan 5, 2019 •
edited

@gundalow Hi John, we have rewritten our code to adhere to your new requested standards of using plugins. Please let us know how this looks and how we can move forward with moving this code through.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jan 5, 2019
@Ghilli3
Copy link
Contributor

Ghilli3 commented Jan 7, 2019

Hi @gdpak - We've rewritten our modules to work with the new plugin standards. Please review our changes and let us know what you think. We need to have this reviewed and pushed through the PR so that we can go back and change all of our existing FortiManager modules to use this new code. Thanks!

@ftntcorecse
Copy link
Contributor Author

Hi @gundalow May you please help us with this PR? This PR is critical for us to continue pushing new modules for our Fortinet products. Please let us know the next steps, even if it is simply assigning a reviewer to this PR.

Thank you for your time and help!

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 14, 2019
@abenokraitis abenokraitis added this to Needs Triage in Networking via automation Jan 14, 2019
Qalthos
Qalthos suggested changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@Qalthos Qalthos left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with how FortiMnager works presently, so I've tried to review the HttpApi plugin based on how things have been done with other such plugins. Feel free to let me know if I've overlooked something or failed to explain something adequately.

lib/ansible/plugins/httpapi/fortimanager.py Outdated Show resolved Hide resolved
lib/ansible/plugins/httpapi/fortimanager.py Outdated Show resolved Hide resolved
lib/ansible/plugins/httpapi/fortimanager.py Outdated Show resolved Hide resolved
lib/ansible/plugins/httpapi/fortimanager.py Outdated Show resolved Hide resolved
lib/ansible/plugins/httpapi/fortimanager.py Outdated Show resolved Hide resolved
lib/ansible/plugins/httpapi/fortimanager.py Show resolved Hide resolved
lib/ansible/plugins/httpapi/fortimanager.py Outdated Show resolved Hide resolved
lib/ansible/module_utils/network/fortimanager/common.py Show resolved Hide resolved
lib/ansible/plugins/httpapi/fortimanager.py Outdated Show resolved Hide resolved
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jan 14, 2019
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 16, 2019
@lweighall
Minor doc change to kick off new shippable test. Unrelated code (not …
b881f73 
…our stuff) caused a failure on the last test.
@ftntcorecse
Copy link
Contributor Author

ftntcorecse commented Jan 19, 2019 •
edited

@Qalthos We’ve updated the code to address your comments. Please provide feedback at your earliest convienence so we can finish this month long PR. It is holding up the rest of our module submissions. We’d like to resolve this as soon as possible. Thanks.

@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. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jan 27, 2019
@abenokraitis abenokraitis added the P2 Priority 2 - Issue Blocks Release label Jan 29, 2019
@abenokraitis abenokraitis added this to the 2.8.0 milestone Jan 29, 2019
@ansibot ansibot removed the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jan 31, 2019
@lweighall
Removed generic methods for get/set/etc. Moved a copy of FMGRLockCTX …
8780d78 
…into the plugin for portability, and to left the original in the mod_utils/fortimanager.py as deprecated code for pre-2.7 customers still running on pyFMG and not the plugin.

Tested all playbooks and all modules, and all appears well.
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 1, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 1, 2019
@ftntcorecse
Copy link
Contributor Author

Looks like we've passed all tests and resolved raised concerns. When can we expect this to get merged? As soon as that happens we can start updating modules, submitting more content, etc.

This is still a bottleneck for us. Please let us know when this plugin can be merged.

Thank you!

@Qalthos Qalthos merged commit e8209c2 into ansible:devel Feb 7, 2019
Networking automation moved this from Needs Triage to Done Feb 7, 2019
@dagwieers dagwieers added the fortimanager Fortimanager community label Feb 22, 2019
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Qalthos Qalthos Qalthos approved these changes

Assignees

@Qalthos Qalthos

@gdpak gdpak

Labels
affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. fortimanager Fortimanager community networking Network category new_plugin This PR includes a new plugin. P2 Priority 2 - Issue Blocks Release support:community This issue/PR relates to code supported by the Ansible community. support:network This issue/PR relates to code supported by the Ansible Network Team.
Projects
No open projects
Networking
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ftntcorecse @ansibot @Ghilli3 @Qalthos @dagwieers @gdpak @abenokraitis @lweighall