-
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: Fix vApp property handling #2220
Conversation
Build failed. ✔️ ansible-tox-linters SUCCESS in 4m 18s |
I've just checked out the CI logs, but to the best of my knowledge they are not related to this MR and rather a form of flaky tests, as they do not mess with vApp properties and the |
recheck |
Build succeeded. ✔️ ansible-tox-linters SUCCESS in 4m 18s |
@ppmathis FYI You, as the author of this PR, should also be able to re-run the CI again by simply commenting Sorry, I didn't have the time to look at this. Although what I can say at the spur of the moment, you have to add a changelog fragment. |
The vmware_guest module did not properly handle setting vApp properties if the VmConfigSpec never contained an active vAppConfig. This resulted in an exception due to trying to access `property` on `None`. This commit changes the behavior to default the current list of vApp properties to an empty list if there has never been any vAppConfig.
2f239f3
to
198e277
Compare
Build succeeded. ✔️ ansible-tox-linters SUCCESS in 4m 26s |
@mariolenz Thanks a lot for your guidance! I've just added a bugfix changelog fragment as asked. |
There have been some problems in the CI and also some changes to |
Build succeeded. ✔️ ansible-tox-linters SUCCESS in 4m 32s |
Hi @mariolenz, Is there anything I can do to assist with the review of this MR? It would be great to see this upstreamed and released, as it would allow me to stop relying on a fork due to this specific bug. Please let me know if there's anything further I can provide to help move this forward. Thanks! |
I'm sorry, I didn't find much time to work on this collection during the last weeks. I hope I'm able to review this PR soon. |
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 wasn't able to test this, but the code LGTM.
Thanks @ppmathis
Build succeeded (gate pipeline). ✔️ ansible-tox-linters SUCCESS in 4m 56s |
Pull request merge failed: Resource not accessible by integration, You may need to manually rebase your PR and retry. |
SUMMARY
The
vmware_guest
module did not properly handle setting vApp properties if theVmConfigSpec
never contained an activevAppConfig
. This resulted in an exception due to incorrectly trying to accessproperty
onNone
.This PR changes the behaviour to default the current list of vApp properties to an empty list if there has never been any
vAppConfig
.ISSUE TYPE
COMPONENT NAME
vmware_guest
ADDITIONAL INFORMATION
A simple reproducer would be a to attempt to use
community.vmware.vmware_guest
like this:Doing so on any VM within vSphere that has its vApp options disabled, meaning that no current vApp properties exist, leads to this exception:
With the fix from this commit applied, the situation where
vAppConfig
equals None is gracefully handled and the vApp properties can be successfully set.