-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Conversation
Hi @mcltn -- Thanks for submitting this new module to Ansible Extras! This module is now in community review, a process that is open to all Ansible users. In order for this module to be approved, it must gain the following votes: “works_for_me”: If you have tested the module thoroughly, including testing of all of the module’s options, and if the module works for you, please add “works_for_me” in the comments. “passes_guidelines”: If you have gone through the module guidelines and the module meets all of the requirements, please add “passes_guidelines” in the comments. Guidelines are available here: http://docs.ansible.com/developing_modules.html#module-checklist “needs_revision”: If the module fails to work for you, or if it doesn’t meet guidelines, please add “needs_revision” in the comments with details about what needs to be fixed. When a module has both “works_for_me” and “passes_guidelines” tags, we will promote the module for inclusion in Ansible Extras. At this point, you will be expected to maintain the module by fixing bugs and evaluating pull requests in a timely manner. Thanks again for submitting your Ansible module! |
"passes_guidelines" Side Note: While PEP8 is not required the use of spaces instead of tabs would make any potential Pull Requests easier as spaces seem to be the more default option. |
while we don't force PEP8, tabs as indentation do create issues, so please convert those to spaces. |
All of the tabs contained in the code have been converted to spaces. |
works_for_me |
description: | ||
- Creates or cancels SoftLayer instances. When created, optionally waits for it to be 'running'. | ||
options: | ||
hostname: |
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 docs need to be valid YAML, the indentation is off in many places
Issues with spacing within documentation yaml has been corrected. |
''' | ||
|
||
import time | ||
from ansible.module_utils.facts import * |
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.
not sure why you have facts imported here
also we would appreciate it if you could flatten this into a single commit |
Flattened to a single commit. |
Looks good. My one nit would be that softlayer.py is the name of the dynamic inventory script as well. |
yep, been thinking about that as well. Going to rename to your suggestion. |
Testing now. I'm seeing something odd. Passing in an int for cpus works fine, but passing a variable that resolves to an int fails. Could you try passing in a var for cpus and see if you can reproduce? failed: [localhost -> 127.0.0.1] => (item={'count': 3, 'datacenter': 'dal06', 'hostname': u'worker', 'disks': [25, 100], 'tags': ['mesos_slave', 'hdfs_datanode', 'consul_client'], 'disk_info': [{'device': 'xvda', 'size': 25}, {'device': 'xvdc', 'size': 100}, {'device': 'xvdd', 'size': 200}], 'cpus': 1, 'local_disk': False}) => {"failed": true, }} as you can see, item.cpus is resolving to an int (no quotes around the 1 in 'cpus': 1) |
This throws the error due to the "choices" option in the parameter. Only certain cpu sizes are allowed and the array is of ints. |
Docs don't specify if ssh_keys: var takes a keyname or id. Name doesn't work, I have to use the key id found by It would be nice to do id lookup of ssh_key names but i wouldn't hold up this pr over it. Just clarify in the docs. |
Specified in the docs that VLAN and SSH Keys are by Id. |
@mcltn Here is my debug using a jinja2 test to make sure it is a number
where returns So it seems I am passing in an int and it is in the list of allowed values. |
looks like adding type='int' to the params fixes the issue. Adding to both cpus and memory. |
for consistency with ec2 module, could you move the wait logic to its own top-level arg rather than putting it into state? I wasn't clear if state = wait would create, then wait, or just wait assuming it was already created. ec2 docs: |
I'm hesitant to do it this way since some users, as well as myself, would want to potentially run provisioning in parallel, but I guess it could place an order in the first play, say for web heads, then when running a second play, say for a db, this step could have a wait flag. It would still require a tag or something to use to know what instances to wait for. Other thoughts? |
@mcltn I'll have to check but I think the way "wait" works with ec2 is that all jobs are launched in parallel, but all jobs must complete before moving on to the next task. I see no use case to waiting for each to complete serially. You would use the "serial" keyword for that i guess. |
- Quantity of virtual instances to provision | ||
required: false | ||
default: 0 | ||
increment: |
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.
this is a bad pattern that crept into other modules, ansible already allows the use of sequences and loops to take care of this
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.
Making edits to remove this functionality within the module. Makes better sense to utilize it outside.
@mcltn 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. |
Planning on making some adjustments, may need some time. Should have updates coming soon. Thanks! |
@mcltn 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. |
if @mcltn doesn't respond by end of week, i can try responding to the On Thu, Mar 3, 2016 at 9:14 AM, Greg DeKoenigsberg <notifications@github.com
kesten broughton |
@mcltn 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. |
Removed the ability to specify quantity. The example documentation shows how to perform multiple provisions using the loop feature with_items. I'd still like to see a better way to handle the wait flag, since now this would wait for each instance in serial as they build for ones that are flagged. This could cause quite a delay when building a large volume of servers. |
ready_for_review |
Thanks @mcltn 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. |
@mcltn A little late, but shipit! Sorry, missed your initial request. |
Question: where do I put in the SoftLayer username and APIkey for authentication? |
Try putting it in ~/.softlayer
|
This module allows the functionality of building and canceling SoftLayer virtual instances. This module requires the SoftLayer library to work. The parameters allow you to build a single instance as well as specify a quantity of instances to build in addition to the various options for the virtual instances themselves.