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
VMware: new module vmware_guest_register_operation #54647
VMware: new module vmware_guest_register_operation #54647
Conversation
@sky-joker, just so you are aware we have a dedicated Working Group for vmware. |
The test
|
@GBrawl @MikeKlebolt @Tomorrow9 @Akasurde @alongchamps @bedecarroll @CastawayEGR @chrrrles @dav1x @garbled1 @GyorgyPeter @imjoseangel @jjahns @kamsz @karstenjakobsen @kryptsi @lrivallain @michaeldeaton @nafpliot-ibm @nerzhul @oboukili @rhoop @ritzk @rmin @stravassac @sumkincpp @vmwjoseph @warthog9 @woshihaoren 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 |
The test
|
- Destination datacenter for the register/unregister operation. | ||
- This parameter is case sensitive. | ||
default: ha-datacenter | ||
cluster_name: |
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.
These are random comments from someone extremely wanting to see this module (so much that I've pulled it and am testing it out). Other vmware modules use cluster instead of cluster_name. Would be nice if it were consistent throughout all the modules.
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.
Thank you for comment!
Modify cluster_name to cluster.
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.
Modified cluster_name parameter name to cluster. :)
- 'Examples:' | ||
- ' [datastore1] vm/vm.vmx' | ||
- ' [datastore1] vm/vm.vmtx' | ||
resource_pool: |
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 set both cluster_name and resource_pool, resource_pool seems to be ignored and the guest is created at the top level of the cluster. If I leave cluster_name off, and just set resource_pool, the guest creates in the right place.
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 assumes that only one of the cluster or resource_pool is specified.
If the and
conditions for the cluster and resource_pool are required, modify the code.
… to DOCUMENTATION
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.
Nice patch! Thanks @sky-joker.
description: | ||
- This module can register or unregister VMs to the inventory. | ||
requirements: | ||
- python >= 2.6 |
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.
pyvmomi only support 2.7+, so you can safely bump to Python version to 2.7
description: | ||
- Destination datacenter for the register/unregister operation. | ||
- This parameter is case sensitive. | ||
default: ha-datacenter |
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.
We don't use any default value for the datacenter
, See: https://docs.ansible.com/ansible/devel/dev_guide/platforms/vmware_guidelines.html#datacenter-argument-with-esxi
register: resource_pools | ||
|
||
- name: get a resource pool | ||
set_fact: resource_pool="{{ resource_pools['json'][2].split('/').7 }}" |
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.
And you can remove the two steps above.
set_fact: resource_pool="{{ resource_pools['json'][2].split('/').7 }}" | ||
|
||
- name: get a vm name | ||
set_fact: vm_name="DC0_H0_VM0" |
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.
Two options here:
- You use
setup_virtualmachines: true
to letprepare_vmware_tests
prepare the VM for you, and you can use "{{ virtual_machines[0].name }}" or "{{ virtual_machines_in_cluster[0].name }}" - OR, you remove
setup_virtualmachines: true
the you prepare the VM yourself (please just call it test_vm1 https://docs.ansible.com/ansible/devel/dev_guide/platforms/vmware_guidelines.html#vm-names-should-be-predictable)
else: | ||
if self.esxi_hostname: | ||
host_obj = self.find_hostsystem_by_name(self.esxi_hostname) | ||
if not host_obj: |
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 the 4th if
level, it probably means you can re-factorize your code a bit, e.g: with a function, of smaller if/else block.
This being said, the code is short enough to be comprehensible, I don't mind merging it like it is today.
Thank you @goneri for pointing it out! |
da124eb
to
6ed2e17
Compare
The tests pass just fine now, thanks @sky-joker for the quick fix. |
Thank you @goneri for merging! |
Can I just use this vmware_guest_register_operation.py as a module in by copying this script in /usr/lib/python2.7/site-packages/ansible/modules |
@dinu21 You can -
Please refer - https://docs.ansible.com/ansible/latest/dev_guide/developing_locally.html#adding-a-module-locally for more details. |
@Akasurde Thanks this worked |
SUMMARY
This module can do the following.
This is useful when you want to unregister a VM from inventory and register it again.
ISSUE TYPE