-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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: Add new module: vmware_host_service_manager #34862
Conversation
@Akasurde @bedecarroll @chrrrles @dav1x @garbled1 @jcpowermac @jjahns @kamsz @mtnbikenc @nafpliot-ibm @nerzhul @pdellaert @rhoop @ritzk @tchernomax @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 |
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 addition, just some remarks/potential improvements
- ESXi hostname. | ||
- Service settings are applied to this ESXi host system. | ||
- If C(cluster_name) is not given, this parameter is required. | ||
state: |
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.
Should there be a state to enable/disable the service on host boot?
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.
Sounds good idea.
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.
It's now handle by the "service_policy" option.
changed = True | ||
self.module.exit_json(changed=changed, results=self.results) | ||
|
||
def check_service_state(self, host, service_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.
Can be replaced with:
def check_service_state(self, host, service_name):
host_service_system = host.configManager.serviceSystem
if host_service_system:
services = host_service_system.serviceInfo.service
for service in services:
if service.key == service_name:
return service.running
msg = "Failed to find '%s' service on host system '%s'" % (service_name, host.name)
cluster_name = self.params.get('cluster_name', None)
if cluster_name:
msg += " located on cluster '%s'" % cluster_name
msg += ", please check if you have specified a valid ESXi service name."
self.module.fail_json(msg=msg)
This way it stops looping through services as soon as it finds the correct one, and if it finds none, it fails
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.
OK.
actual_service_state = self.check_service_state(host=host, service_name=self.service_name) | ||
host_service_system = host.configManager.serviceSystem | ||
if host_service_system: | ||
service_state = False |
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.
Personally i'd like it better if this was changed_state
, for instance, as that better indicates the purpose of this variable, looking at the code
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.
OK.
Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
127a638
to
0ef47b0
Compare
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.
LGTM
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.
LGTM
shipit
@tchernomax @pdellaert Thanks for your valuable comments and reviews. |
SUMMARY
Signed-off-by: Abhijeet Kasurde akasurde@redhat.com
ISSUE TYPE
COMPONENT NAME
lib/ansible/modules/cloud/vmware/vmware_host_service_manager.py
test/integration/targets/vmware_host_service_manager/aliases
test/integration/targets/vmware_host_service_manager/tasks/main.yml
ANSIBLE VERSION