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

[Feat] - Get tags of VM from vSphere SDK Client #335

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

oijkn
Copy link

@oijkn oijkn commented Jan 3, 2023

With the previous code, the tags did not appear when I did the salt-call vmware_vm.info <minion> command.

By adding the vSphere SDK Client, the tags are ok in the return of the command.

Important: it is mandatory to install the new dependencies.

pip3 install --upgrade git+https://github.com/vmware/vsphere-automation-sdk-python.git

@vmwclabot

This comment was marked as resolved.

@vmwclabot

This comment was marked as resolved.

@vmwclabot

This comment was marked as outdated.

@waynew
Copy link
Contributor

waynew commented Jan 10, 2023

@oijkn thanks for the PR!

I was under the impression that tags were get-able with our current approach but maybe that's a specific version of vSphere or something.

A couple of things - first, tests are required for all code contributions. Meaningful unit tests are fine, but unit+integration are better. If you need help with writing tests - attend one of the Test Clinics on Twitch, and we'll be happy to help you get started 👍. We also want a changelog entry

But more importantly, the vSphere SDK is a very finicky dependency. It has a known issue about pip-installing for a very long time. Almost a decade long.

I've seen that dependency cause a number of issues in upstream Salt, which gives me very very strong reservations about making it a dependency of our project.

I'm OK with making it an optional dependency that's basically completely isolated. Something to this effect:

import utils.vsphere_sdk


...

    tags = utils.vsphere_sdk.get_tags(vm)

Then it could do something like this:

try:
    from com.vmware.vapi.std_client import DynamicID
    HAS_VSPHERE_SDK = True
except ImportError:
    HAS_VSPHERE_SDK = False


def get_tags(vm):
    if HAS_VSPHERE_SDK:
        try:
            vm_mid = vm._moId
            ...
            tags = [client.tagging.Tag.get(tag).name for tag in tags_list]
        except Exception:  # not sure if this try/except is necessary
            tags = []
    else:
        tags = [tag.name for tag in vm.tag]
    return tags

The create_vsphere_client would also need to be similarly protected. Honestly, I wouldn't put the api_client in connect - I'd keep it completely isolated in the vsphere_sdk module. That's just because of the number of issues I've seen using the SDK.

If we can't get the tags any other way 🤷 we can go ahead and provide the ability here. But if someone can't install the SDK, or it gives some issues, then we shouldn't be broken -- we should just carry on as normal, though we can log at an info or warning level or something, if that's useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants