feat: datasource for virtual machines

[castorsky]

I have implemented one of datasources which was requested in #196.

This datasource searches the vSphere cluster for some VMs that matches specified filters and returns VM name for one (and only one) machine. I have chosen such behavior because it will be straightforward for vsphere-clone builder - there will be no need for user to manipulate lists in locals.

I have included some tests and documentation for datasource.

By strange coincidence issues for vSphere and Proxmox datasources share the same ID. =)

Partially closes #196

[tenthirtyam]

Thanks @castorsky. I’ll review this week.

[tenthirtyam]

Question: Since tags can long pass more than one tag category and name, should it be represented as singular tag?

Also, it looks like a go fmt ./... is needed for the CI.

[tenthirtyam]

Also, it looks like a go fmt ./... is needed for the CI.

I took care of the fmt on the last commit and the CI is good now.

[tenthirtyam]

Hi @castorsky 👋 - I've pushed updates to your branch with the requested changes from 2024-12-08. I'll do another review today and then plan to test the functionality along with Lucas.

Ready for next round of reviews.

[castorsky]

Hi @tenthirtyam! I agree with you about tag(s) field and renamed it. It was the network_adapters in the vsphere-iso builder that confused me. =)

[lbajolet-hashicorp]

Hi @castorsky,

Thanks for the PR, I left a couple comments on the code, most are nits/small changes or questions, but overall this looks good to me.

Let me know how you wish to respond to those comments, and once you've had a chance to do that, I'll do another review pass.

[lbajolet-hashicorp]

I see we're introducing a common driver, does that mean that the builder's driver is to be moved to this package? I don't see anything to that effect in the PR, is that a later item?

At first glance, the code seems to be equivalent to the NewDriver function from the builder/vsphere/driver package, so I assume this is meant to become one package eventually?

[castorsky]

I am not an experienced developer and do not want to introduce potentially breaking changes to the main branch. So, I implemented a basic driver that exposes some govmomi functions required by my data source and its filters to process requests to the vSphere cluster. I have no intention of replacing the main driver from the builder.

[lbajolet-hashicorp]

Unless I'm mistaken in my assumption, this is only meant for testing the Driver isn't it? Usually we call them Mock, as is the case for the driver in the builder, coupled with an interface for typing, so you can specialise it for individual tests.
If I understand correctly, is there a reason for this to be implemented differently?

[lbajolet-hashicorp]

Note: looking at the test and this code in more detail, the simulator structures come from govmomi, which makes sense if this is simpler to reason with than a mocked driver, though I would suggest the mocked driver still for consistency.

[castorsky]

The simulator is used to test the datasource itself, not the driver. I used the driver_test.go as a reference. I did not implement a mock driver since my driver only exposes standard govmomi functions.

[lbajolet-hashicorp]

It seems to me that since the Finder is the only component used here, we could be using the builder's Driver for this purpose, if the attribute becomes public. I think it further highlights the point that the driver should become a common structure for both the builders and the datasource.
It can be done in a later PR to be clear, it doesn't block us from merging this now if the datasource is relevant immediately, but it is something to consider I think.

[castorsky]

virtualmachine datasource and its filters use almost all of the driver attributes:

• Finder to search for virtual machines and ESX hosts;
• Client to list virtual machines on the ESX host;
• RestClient to list tags attached to the virtual machine;
• Context to use some govmomi functions.

[lbajolet-hashicorp]

Regarding context, the idea is that we have refined instances of that for calls: contexts with a timeout for example, which aren't necessarily one global instance. The global instance otherwise becomes a context.Todo() or a context.Background(), which essentially do nothing, in which case I wonder if we need this, as we could as well directly use context.Background() to create an instance where needed.

For the others I can understand why we need them though, I'm still not sure we need another Driver implementation for those, since we directly use the functions exposed by those clients, it seems to me that the Driver from builders/common would do fine if its components became public, but as discussed in another thread, making that driver common can be a later thing.

[castorsky]

Totally agree with removing of my driver when builders/vshpere/driver will open its components.

[castorsky]

Hi @lbajolet-hashicorp ! Thank you for the code review.

Let me know how you wish to respond to those comments, and once you've had a chance to do that, I'll do another review pass.

I plan to work on it this weekend and will try to complete all necessary changes within the next week or two.

[castorsky]

Hi @lbajolet-hashicorp ! I refactored the filters section to use unit functions. However, I believe the other requested changes require further discussion. I’ve left comments on all your requests.