-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature/migrate esxi host inventory #91
Feature/migrate esxi host inventory #91
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
==========================================
- Coverage 27.89% 27.38% -0.52%
==========================================
Files 25 31 +6
Lines 2079 2542 +463
Branches 386 468 +82
==========================================
+ Hits 580 696 +116
- Misses 1499 1846 +347
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fdaa257
to
b4b6338
Compare
I see that the test failed due to the following error:
So, the inventory plugin did not return any hosts, causing inventory_results._meta.hostvars to be empty |
tests/integration/targets/vmware_inventory_esxi_hosts/tasks/main.yml
Outdated
Show resolved
Hide resolved
While testing the inventory plugin, I noticed that the initial output format looks a bit messy and difficult to read (as shown in the attached screenshot). However, below, I do see a more structured and readable version. Could you confirm if this initial "user unfriendly" format is expected behavior? If so, is it something that could potentially be optimized for better readability during debugging? |
@prabinovRedhat |
Thanks looks good! |
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
d700808
to
27db3cb
Compare
SUMMARY
Adds an ESXi host inventory plugin. This plugin was originally going to be a migration from the community collection, but I dont think I can call it that at this point.
While the output is largely the same, some redundant properties have been removed and some data has been manipulated to be more useful (i.e. tags now return the tag ID and tag name instead of just the tag name, which is not unique).
Additionally, some community options seemed confusing, conflicted with options we have for info modules, or are no longer relevant.
Also, Im not able to use the existing pyvmomi class in the inventory plugin since it is tightly coupled to modules. The community collection works around this by maintaining two nearly identical versions of the pyvmomi/rest utilities for modules and for plugins.
I think we can break some of the pyvmomi/rest utils into more generic "client" classes. Those can be used by module or plugin base classes (like i do in this PR for the inventory plugin), and then those base classes can be extended by individual modules and plugins.
This approach has greatly reduced the amount of duplicated code, and I think will make it easier for community to switch to the new utils.
ISSUE TYPE
COMPONENT NAME
vmware.vmware.esxi_hosts
ADDITIONAL INFO
I tested the new "client" classes with the modules and it worked as expected. Fully switching them over would be a task for next release