created adapter_type variable for network interfaces #193
Conversation
There was a problem hiding this comment.
Hey @shelson - thanks for this PR! This definitely seems like an easy win ahead of the refactor coming soon.
I put some general feedback in the PR. There's a few things you can do using both the TF API and Go to make this better.
Thanks!
| @@ -382,6 +387,21 @@ func resourceVSphereVirtualMachine() *schema.Resource { | |||
| Type: schema.TypeString, | |||
| Optional: true, | |||
| ForceNew: true, | |||
| Default: "vmxnet3", | |||
| ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) { | |||
There was a problem hiding this comment.
You can use StringInSlice from the validation package in TF core to make this much easier on the eyes.
The syntax would be here:
ValidateFunc: validation.StringInSlice(virtualMachineNetworkAdapterTypeAllowedValues, false)
| @@ -36,6 +36,11 @@ var DiskControllerTypes = []string{ | |||
| "ide", | |||
| } | |||
|
|
|||
| var NetworkAdapterTypes = []string{ | |||
There was a problem hiding this comment.
This variable should be unexported (starts with a lowercase letter, ensures that no package outside of vsphere can use it - basically a private variable), and be specific enough to avoid collisions.
Suggestion: virtualMachineNetworkAdapterTypeAllowedValues.
Also, you might want to check here to see if there is any appropriate constants you can use.
| @@ -1144,6 +1167,12 @@ func resourceVSphereVirtualMachineRead(d *schema.ResourceData, meta interface{}) | |||
| log.Printf("[DEBUG] Device list %+v", deviceList) | |||
| for _, device := range deviceList { | |||
| networkInterface := make(map[string]interface{}) | |||
| adapter_type_string := fmt.Sprintf("%T", device) | |||
There was a problem hiding this comment.
Here, rather than printing the type out, you should be using the more spec-native type assertion.
The code below would look like this:
if _, ok := device.(*types.VirtualE1000); ok {
networkInterface["adapter_type"] = "e1000"
} else {
networkInterface["adapter_type"] = "vmxnet3"
}You can also use the switch type assertion format as well.
…nd shortened validation function drastically
|
Hey @shelson, thanks for the update! Next issue: the tests are failing because you haven't run the changes through Depending on the IDE/editor you are using this is set up different ways - if you can tell me I might be able to help. Also see here: https://blog.golang.org/go-fmt-your-code |
|
Thanks for the awesome feedback - I really appreciate you taking the time to help me get this in a shippable state. I'll work on the go-fmt thing in the morning as it's getting late here now. Thanks again. |
Now that adapter_type has been exposed properly, document it. The adapter types will be improved on during the refactor.
|
LGTM to me now @shelson! Merging now. Thanks for the contribution! |
ghost
locked and limited conversation to collaborators
I needed this functionality and noticed that it was already commented with "TODO". It's my first foray into go so the following might be better worked out another way:
adapter_type_string := fmt.Sprintf("%T", device)
if adapter_type_string == "*types.VirtualE1000" {
Otherwise though it seems to work just fine.