Module to manage F5 GTM virtual servers #555
Conversation
perzizzle
changed the title
|
@perzizzle ready for review? |
|
I think its ready for review. There is plenty of opportunity to add more features but this is all I needed so far. I intend on adding more eventually. |
|
Since I don't run GTM, I'm a bad person to test this. Can you solicit some GTM users on the mailing list to test this? |
| else: | ||
| bigsuds_found = True | ||
|
|
||
| def bigip_api(bigip, user, password): |
There was a problem hiding this comment.
Perhaps replace bigip with server for consistency
There was a problem hiding this comment.
I think I just copied this out of the LTM modules but seems like a reasonable change to improve readability.
| module.fail_json(msg="virtual server does not exist") | ||
|
|
||
| if state == 'absent': | ||
| if virtual_server_name and virtual_server_server: |
There was a problem hiding this comment.
This condition seems unneccessary as both vars are required?
Also, if this condition wasn't met (which can't happen), the module would exit without result being set.
|
So, same for me, I don't have GTM gear to test this. Most things seem OK, some minor remarks on the specific functions, but unless I read main() badly (it's late here) it seems to have some logic flaws. |
|
Thanks for the review, I'll make some updates based on the comments. You can tell I started out with just providing the ability to enable/disable virtual servers and then shoehorned in the ability to add/remove virtual servers. |
|
Playbook to validate functionality https://gist.github.com/perzizzle/5ffa93735881ce7cbf9b depends on an extra_vars.yml similar to https://gist.github.com/perzizzle/190195da598a0311ce6b run with ansible-playbook main_virtual_server.yml --extra-vars=@extra_vars.yml -vv |
|
@caphrim007 Please review this PR to make sure it adheres to the following guidelines: http://docs.ansible.com/developing_modules.html#module-checklist If it passes these guidelines, and if you believe it’s a good PR otherwise, please add a comment with "shipit" in the text, and we will flag it for inclusion. If it fails these guidelines, please add a comment with "needs_revision" in the text, along with the needed changes, and we will flag it for revision. |
|
|
||
| # import module snippets | ||
| from ansible.module_utils.basic import * | ||
| main() |
There was a problem hiding this comment.
My only nit here is that I'd wrap this main in a conditional
|
I've verified that this module works on 11.6.0 provided you create a data center and a server to associate to the VS. Add the conditional to the bottom and lgtm |
|
Hi @perzizzle -- sorry this one is taking so long -- looks like it fell through the cracks. :\ Looks like @caphrim007 had a request for you to add something -- can you address that, and then it looks like we can probably merge this? Thanks! |
|
@robynbergeron arg, my bad. I forgot to tag the needs_revision. Still new to this workflow. sorry about that! |
|
@caphrim007 no worries; so are we. :) |
|
@gregdek @caphrim007 I updated this PR with the parameter changes suggested by @resmo. And then ran through creating, deleting, enabling and disabling virtual servers with the new defaults. |
|
there is a catchall exception handling, but not a reason to block this module for integration. Based on my code review, I give it a +1. shipit |
|
I count multiple shipits. :) |
|
@perzizzle as I see you do not return anything, you can just add an empty return: |
|
Just to verify, the 'bigip_gtm_virtual_server' module here is different than the https://github.com/ansible/ansible-modules-extras/blob/devel/network/f5/bigip_virtual_server.py |
|
@alikins that module manages F5 LTMs, this one manages GTM. |
|
Pushed to devel at 677a2dd with some minor pep8 cleanups and the RESULTS change resmo suggested. |
|
@alikins the thing that you pushed to devel went in to the f5/ directory. Not the network/f5 directory as per the other f5 modules. Can you take a look? |
|
Yeah, that's weird. Wrong directory! |
|
Huh, weird. I think I must have done a git-format-patch/git-am when rebasing/testing and dropped a '-P' level somewhere. Fixing... |
|
Dir fixup pr at #2302 |
f5/ was the wrong directory. Move it to network/f5 and remove f5/.
Supports enabling/disabling F5 GTM virtual servers. Stubbed out where to add/remove virtual servers.