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.
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
New module: Add Openstack Tempest run module (cloud/openstack/os_tempest_run) #24173
Conversation
The test
|
The bot didn't tag anyone again. ready_for_review |
options: | ||
workspace: | ||
description: | ||
The workspace as was configured in 'Tempest init <I(workspace)>' |
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 I(workspace)
will turn into italics, which I'm not sure if that's what you want in this case
https://docs.ansible.com/ansible/dev_guide/developing_modules_documenting.html#formatting-options
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 documentation says "I() for option names" but I think I'll change that to C(workspace) to make it more visible.
edit: now that I think about it, it should be C(workspace) as it is the option value and not the name.
notes: | ||
- You can find out more about Tempest at U(http://docs.openstack.org/developer/tempest/) | ||
- The module requires to tempest init <I(workspace)> before usage | ||
- The options I(whitelist_file) and I(blacklist_file) are mutually exclusive, if they are both given only the I(whitelist_file) will be used |
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.
If you use the mutually_exclusive
function you can remove this line
''' | ||
|
||
EXAMPLES = ''' | ||
# Run all tests with default number of workers |
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.
As per Ansible best practice, please use - name: Run all tests with default number of workers
then remove the -
from in front of the module name
http://docs.ansible.com/ansible/dev_guide/developing_modules_documenting.html#examples-block
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.
Thanks, I fixed it.
I originally wrote it after looking at the pip module as an example (https://docs.ansible.com/ansible/pip_module.html#examples), might be nice to add it as an issue for something like 'First Timers Only' or 'YourFirstPR'.
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.
That's a good idea.
Where in the existing documentation/examples did you look?
I'm wondering where I can put/link to the example so people will find it.
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.
In the documentation website: https://docs.ansible.com/ansible/pip_module.html#examples
And in the module implementation starts at: https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/packaging/language/pip.py#L148
concurrency=dict(type="int", required=False, default=None), | ||
force=dict(type="bool", required=False, default=False), | ||
subunit=dict(type="bool", required=False, default=True), | ||
)) |
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.
There are some options to AnsibleModule
which you may be able to use to validate options passed in.
Look at existing modules for examples:
mutually_exclusive
required_together
required_one_of
require_if
Should whitelist_file
and blacklist_file
be mutually_exclusive
mutually_exclusive=(('blacklist_file', 'whitelist_file'),),
DOCUMENTATION = ''' | ||
--- | ||
module: os_tempest_run | ||
short_description: Runs Tempest |
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.
I wonder if it might be worth saying Run OpenStack Tempest
to make it a little easier for people to find via a search engine.
:arg follow: A boolean to indicate of symlinks should be resolved or not | ||
:raises UnicodeDecodeError: If the canonicalized version of the path | ||
contains non-utf8 byte sequences. | ||
:rtype: A text string (unicode on pyyhon2, str on python3). |
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.
pyyhon2 -> python2
Thanks for the review @gundalow |
waiting_on: TalShafir |
ready_for_review |
Hi! Thanks for the new module. However, I don't feel like tempest modules are a good fit here. The current set of modules are geared toward using OpenStack services. Modules for testing the services seems like they should be kept separate. Most consumers of the core Ansible OpenStack cloud modules are only going to be interested in the former, not the latter. And on a more personal level, as a core maintainer for these modules, I don't want to have to learn the tempest testing environment in order to support these going forward. I just have no interest in it. I'd like to get more feedback from possibly @emonty, @j2sol, or @rcarrillocruz and get their thoughts on this. |
I agree with @Shrews |
@j2sol @juliakreger @Shrews @Thingee As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add |
I agree with @omgjlk and @Shrews. I would recommend the following role to run Tempest if you'd like https://github.com/openstack/openstack-ansible-os_tempest @emonty do you feel the same as well? |
Thanks for submitting patch for Openstack Ansible modules! |
Closing as per above. |
SUMMARY
New module that lets the user run Tempest
ISSUE TYPE
COMPONENT NAME
os_tempest_run
ANSIBLE VERSION