-
Notifications
You must be signed in to change notification settings - Fork 339
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_guest_info - Make "datacenter" property optional #2241
vmware_guest_info - Make "datacenter" property optional #2241
Conversation
…VMs were found and make providing the datacenter property in the guest_info module optional. Signed-off-by: Patrick Pfurtscheller <[email protected]>
Build failed. ✔️ ansible-tox-linters SUCCESS in 4m 38s |
Signed-off-by: Patrick Pfurtscheller <[email protected]>
Build succeeded. ✔️ ansible-tox-linters SUCCESS in 4m 17s |
Build succeeded. ✔️ ansible-tox-linters SUCCESS in 4m 28s |
The code you've changed in
So we'll have to make sure that it's really safe and we don't break anything by accident. Apropos of breaking things, this might be considered a breaking change because people are used to the current behavior. And that means we might have to wait for the next major release to implement this. |
@mariolenz Okay, what would be the best approach here going foreward? |
Build failed. ✔️ ansible-tox-linters SUCCESS in 4m 35s |
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.
Okay, what would be the best approach here going foreward?
I've had a look at those other modules and think that your changes should be OK. Let's give it a try! Although we might revert this in a future minor or bugfix release if it turns out to be problematic later.
There's just one minor thing I see in the error message. I should say it hasn't been that great before, but maybe we should improve it a bit while we're at it.
cc @ihumster
Co-authored-by: Mario Lenz <[email protected]>
Build failed. ✔️ ansible-tox-linters SUCCESS in 4m 46s |
@mariolenz I looked at the vmware.py library code and I think the changes Patrick made are pretty safe for the modules to function. And indeed, if something goes wrong, we will release a rollback fix. |
recheck |
Build failed. ✔️ ansible-tox-linters SUCCESS in 4m 57s |
recheck |
Build succeeded. ✔️ ansible-tox-linters SUCCESS in 4m 37s |
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.
Build succeeded (gate pipeline). ✔️ ansible-tox-linters SUCCESS in 4m 36s |
5bca41c
into
ansible-collections:main
SUMMARY
We run a big VMware environment featuring multiple vCenters and multiple datacenters within them.
When automating the deployment of VMs at the moment we search for existing machines with the name of the to be deployed machine in order to perform clean-up activities. At the moment we use the guest find module, but it's slow.
Therefore we looked into the guest_info module and found that if the VM is unique, the datacenter property is never actually used as it is only relevant in case multiple VMs have been found.
In case multiple VMs are found, the folder and datacenter attributes are used to decide on "the right one".
Therefore, the datacenter property could as well be an optional parameter on the module and it could simply fail in case there is more than one VM matching and "datacenter" as well as "folder" are required.
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION