Skip to content
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

Merged

Conversation

sky-joker
Copy link
Contributor

SUMMARY

This module can do the following.

  • Register VM to inventory
  • Unregister VM from inventory

This is useful when you want to unregister a VM from inventory and register it again.

ISSUE TYPE
  • New Module Pull Request

@ansibot
Copy link
Contributor

ansibot commented Mar 31, 2019

@ansibot
Copy link
Contributor

ansibot commented Mar 31, 2019

@sky-joker, just so you are aware we have a dedicated Working Group for vmware.
You can find other people interested in this in #ansible-vmware on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 cloud community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. vmware VMware community labels Mar 31, 2019
@ansibot
Copy link
Contributor

ansibot commented Mar 31, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest_register_operation.py:0:0: E312 No RETURN provided

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 31, 2019
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Mar 31, 2019
@ansibot
Copy link
Contributor

ansibot commented Mar 31, 2019

@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 shipit if you would like to see it merged.

click here for bot help

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 31, 2019
@ansibot
Copy link
Contributor

ansibot commented Apr 9, 2019

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/vmware_guest_register_operation/aliases:0:0: missing alias `shippable/posix/group[1-4]` or `unsupported`

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 9, 2019
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 9, 2019
@ansibot ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label May 29, 2019
@ansibot ansibot removed the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jun 30, 2019
@ansibot ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jul 8, 2019
- Destination datacenter for the register/unregister operation.
- This parameter is case sensitive.
default: ha-datacenter
cluster_name:
Copy link

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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:
Copy link

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.

Copy link
Contributor Author

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.

@ansibot ansibot removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Aug 17, 2019
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Aug 25, 2019
@ansibot ansibot removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Sep 26, 2019
Copy link
Contributor

@goneri goneri left a 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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

register: resource_pools

- name: get a resource pool
set_fact: resource_pool="{{ resource_pools['json'][2].split('/').7 }}"
Copy link
Contributor

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two options here:

else:
if self.esxi_hostname:
host_obj = self.find_hostsystem_by_name(self.esxi_hostname)
if not host_obj:
Copy link
Contributor

@goneri goneri Sep 30, 2019

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.

@sky-joker
Copy link
Contributor Author

Thank you @goneri for pointing it out!
I will cope with the points pointed out.

@sky-joker sky-joker force-pushed the new_module_vmware_guest_register_operation branch from da124eb to 6ed2e17 Compare October 1, 2019 16:04
@goneri
Copy link
Contributor

goneri commented Oct 2, 2019

The tests pass just fine now, thanks @sky-joker for the quick fix.

@goneri goneri merged commit ff933be into ansible:devel Oct 2, 2019
@sky-joker
Copy link
Contributor Author

Thank you @goneri for merging!

@sky-joker sky-joker deleted the new_module_vmware_guest_register_operation branch October 3, 2019 13:57
@Akron27
Copy link

Akron27 commented Oct 17, 2019

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

@Akasurde
Copy link
Member

@dinu21 You can -

  • copy required module to /usr/lib/python2.7/site-packages/ansible/modules/cloud/vmware/
    or
  • create a directory library in current working directory and copy-paste required module inside this directory

Please refer - https://docs.ansible.com/ansible/latest/dev_guide/developing_locally.html#adding-a-module-locally for more details.

@Akron27
Copy link

Akron27 commented Oct 18, 2019

@dinu21 You can -

  • copy required module to /usr/lib/python2.7/site-packages/ansible/modules/cloud/vmware/
    or
  • create a directory library in current working directory and copy-paste required module inside this directory

Please refer - https://docs.ansible.com/ansible/latest/dev_guide/developing_locally.html#adding-a-module-locally for more details.

@Akasurde Thanks this worked

@ansible ansible locked and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 cloud module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. vmware VMware community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants