-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Conversation
@caphrim007 This is the module we talked about awhile back, it works in conjunction with bigip_gtm_wide_ip/pool/virtual_server and is modeled in large part on bigip_facts. |
alrighty, i'll have a look-see. Cursory glances suggest some improvements. The output listed in the PR needs to be documented in a RETURN = ''' Block in the head of the file. Also, there is an f5 file in the module_utils that should be used to get access to the bigip soap api as it includes cert validation. Also it can be used to include the f5_module_spec and then add arguments to that. There are some examples for reference in this module here https://github.com/F5Networks/f5-ansible/blob/master/library/bigip_partition.py Lines 102 and 365. Thanks! |
@caphrim007 Do you know of any modules that only return ansible_facts for a look at their RETURN string? bigip_facts doesn't seem to support this yet and the developing modules documentation doesn't provide much guidance. I committed an initial guess. I'll update the module to use the helper methods shortly, those weren't available when I originally wrote this. |
@perzizzle the link I provided at line 365 has an example. It's enough to list the return key (ex. "wide_ip") and an example of the values of the key. For example http://docs.ansible.com/ansible/clc_group_module.html |
@caphrim007 I updated the module to use the f5 module_utils. My confusion about the RETURN document string is whether or not I should use ansible_facts (which is the actual key that's returned) or wide_ip, virtual_server, and pools which are the items within ansible_facts. The sample string I provided would be the format returned if you did include=wide_ip. |
@perzizzle the latter. That key causes ansible to merge the facts you provide, as JP mentions here http://jpmens.net/2012/07/15/ansible-it-s-a-fact/ |
@perzizzle here's a better example of a dictionary being documented |
@caphrim007 Alright took another shot at a the RETURN string. Do you know of way to see how it will be displayed in the documentation? It passes yamllint but it would be nice to see what it looks like after its been parsed and displayed. |
@perzizzle Yep, there is the ansible-doc command. You can run it on the existing stat module like so ansible-doc stat and see the RETURN section for details. For your module there you can run it and specify the module search path. For example, if your module is located in the current working directory's library/ directory, it might look like this ansible-doc -M library/ bigip_gtm_facts Hope that helps |
Thanks @perzizzle for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion. |
Thanks @perzizzle for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review. [This message brought to you by your friendly Ansibull-bot.] |
I updated version added to pass the build, otherwise I think its ready. @caphrim007 @gregdek |
- "Tested with manager and above account privilege level" | ||
|
||
requirements: | ||
- bigsuds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requirements
is just text, so you can make this clearer by doing
- "bigsuds >= 11.4"
I'll leave the functional review to the Networking experts. PEP8 is showing a number of issues that it would be good to get addressed
|
@perzizzle ping, any update here? |
@gregdek quick before the merge conflicts appear again :) |
I'll pull this and check it against my bigips here |
@caphrim007 There's been no changes I just pulled in the latest devel so that there weren't any merge conflicts. |
Hi, Just reviewing Networking PRs. What's the plan for this (who's doing what)? |
Hey @gundalow I think the ball is currently with me and reviewing it to make sure it is returning expected results. I can respond today |
@caphrim007 That's great thanks. If there is anything for the Ansible Core Team to do feel free to add it to the Networking Meeting agenda (we have a meeting today) https://github.com/ansible/community/blob/master/MEETINGS.md |
|
||
class F5(object): | ||
def __init__(self, host, user, password, validate_certs, session=False): | ||
self.api = bigip_api(host, user, password, validate_certs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
port needs to go here
@gundalow and @perzizzle needs_review |
- BIG-IP host | ||
required: true | ||
default: null | ||
choices: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to list empty parameters, so you can delete these lines
needs_revision |
Thanks @perzizzle for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review. [This message brought to you by your friendly Ansibull-bot.] |
@perzizzle A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer. [This message brought to you by your friendly Ansibull-bot.] |
@perzizzle Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request. [This message brought to you by your friendly Ansibull-bot.] |
Merge conflict in |
@perzizzle A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer. [This message brought to you by your friendly Ansibull-bot.] |
@gregdek @gundalow @caphrim007 Has offered to usher this PR through the approval process. |
Thabks again for working on this. Just for completeness once you raise the new PR please link to this one in the comments. |
@perzizzle here is the facts module that has been merged #3232 |
Issue Type:
Plugin Name:
bigip_gtm_facts
Ansible Version:
Summary:
This module can be used to gather details about F5 GTM Wide Ips, Pools and Virtual Servers
Example output: