r/virtual_machine: Handle missing environment browser

[vancluever]

The virtual machine resource relies on the cluster or standalone host
it's deployed on for information regarding a default hardware set or
guest OS family information. This is obtained via the EnvironmentBrowser
MO referenced by the respective cluster or host's ComputeResource MO.

There are certain situations where this attribute will be missing, two
of which have been identified so far via bug reports:

• A cluster without hosts will not contain an environment browser.
• vSphere licensing issues may cause a missing environment browser (Create of new vMWare vm crashes on plan #409
  reported an expired vCenter).

This update wraps our fetching of the environment browser by the two
current compute resource helpers that utilize it in a helper that
validates that we actually have an environment browser before proceeding
to load it up, issuing an error alluding to the above if it is missing.

Fixes #411.
May also correct #409.

[paddycarver]

nit: these functions feel like they'd be more reusable if the environment variables are passed in as arguments, but I don't have the context to know if that's a valid design consideration or not.

[vancluever]

Where applicable, configuration generators do have parameterization, but again, only when it's necessary in a test (ie: a multi-step test where the configuration is the same but a variable is changing). Otherwise, these configurations should be considered static and doubling up the parameterization would only serve to make things more redundant.

Some future work I'm considering doing is possibly adding to the test suite so that we can load external configuration (ie: out of a test-fixtures directory), and allowing the use of actual variables. If that indeed comes to fruition, this is probably when I would come back to this and refactor things so that the parameterization is in the individual test cases themselves.

[tombuildsstuff]

on that note - tbh there's an argument for making these values a Struct that's parsed out once, rather than in each test function (this is the case in all providers)

[vancluever]

I would be fine with this if there was a way to wrap it in a way that retains the same "static generation" look and feel. I actually am not a fan of having variables declared in every test function.

I think that something along the lines of this would work even:

Config: resource.GenerateConfig(testAccResourceVSphereConfigHere),

Where resource.GenerateConfig is a generator that takes a text/template with a lightweight, Packer-ish way of accessing environment variables.

testAccSomeConfig = `
variable "foo" {
  default = "{{env TEST_PROVDER_ENV_VAR}}"
}
`

In any case, such things are kind of out of scope here 😛

[paddycarver]

Is this really supposed to execute the config and not check anything?

[vancluever]

This is necessary for steps with PlanOnly to ensure that the destroy step does not attempt to plan/refresh off of the broken configuration and run into the same issue. I think this might be a bug in the test framework but I'm not 100% sure, as the destroy step is only supposed to run if a state exists as referenced here, and it's odd to me as to why a bad plan would have a state versus a bad apply. In any case, the second step just runs with a completely blank config to clear out the state so any subsequent destroy steps have nothing to do.

An example of what happens when that step does not run:

    --- FAIL: TestAccResourceVSphereVirtualMachine/clone_into_empty_cluster,_no_environment_browser_(fails_on_plan) (5.54s)
    	testing.go:573: Error destroying resource! WARNING: Dangling resources
    		may exist. The full state and error is shown below.
    		
    		Error: Error refreshing: 1 error(s) occurred:
    		
    		* vsphere_virtual_machine.vm: 1 error(s) occurred:
    		
    		* vsphere_virtual_machine.vm: cannot find OS family for guest ID "ubuntu64Guest": compute resource "ComputeResource:domain-c178 @ /hashi-dc/host/hashi-cluster2" is missing an Environment Browser. Check host, cluster, and vSphere license health of all associated resources and try again
    		
    		State: <nil>

I've removed it from the apply test (creation from scratch) and will push.

[paddycarver]

My questions have been adequately answered to satisfy me that they're design decisions and not oversights.

I don't have the context or experience here to gauge those design decisions, but the code seems ok to me.

[vancluever]

Thanks @paddycarver!

[paddycarver]

(This approval was meant to convey "looks good to me within the limits of my ability to determine whether something is good or not", but I've been awful at communicating today, so apologies if it came off as something else.)