Additional SCSI controller types support

[dkalleg]

This allows the user to specify new controller types. Before when
specifying 'scsi', govmomi defaults to lsilogic-parallel. This patch
allows the user to now specify 'scsi-lsi-parallel', 'scsi-buslogic',
scsi-paravirtual', and 'scsi-lsi-sas'. Resolves issue
#7202

[sputnik13]

It would be great if this could be merged for 0.7.0 or a point release soon thereafter. We cannot use terraform to create Windows VMs on vSphere at this time because LSI Parallel (which is the only scsi controller type supported by terraform at this time) is not supported by Windows 2012 R2 out of the box.

[stack72]

I think we need to do something about this ValidateFunc. We need to refactor it to something like declaring a []string of values and then checking that the value is in the slice. The cyclomatic complexity of if && && && etc doesn't really read the best :)

[dkalleg]

Good point. I added a []string with each controller type and the validate func now loops over it. I wish golang had the python-ism of "if x in y" instead of having to loop through. Apparently that is a go-ism. See what you think about this patch.

[stack72]

Hey @dkalleg

Just a FYI, I wrote a validateFunc that looks like this:

func validateArmStorageBlobType(v interface{}, k string) (ws []string, errors []error) {
    value := strings.ToLower(v.(string))
    validTypes := map[string]struct{}{
        "blob": struct{}{},
        "page": struct{}{},
    }

    if _, ok := validTypes[value]; !ok {
        errors = append(errors, fmt.Errorf("Blob type %q is invalid, must be %q or %q", value, "blob", "page"))
    }
    return
}

Notice the use of map and not having to iterate over it ;)

P.

[dkalleg]

I like not having to iterate! To do this wouldn't I have to redeclare the set of controller types inside of the validateFunc? My mentality was to have them defined just once, so if a new controller type is supported later, it would only have to be added in one place.

Is this about what you were thinking:

ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
    value := v.(string)
    validTypes := map[string]struct{}{
        "scsi": struct{}{},
        "scsi-lsi-parallel": struct{}{},
        "scsi-buslogic": struct{}{},
        "scsi-paravirtual": struct{}{},
        "scsi-lsi-sas": struct{}{},
        "ide": struct{}{},
    }
    if _, ok := validTypes[value]; !ok {
        errors = append(errors, fmt.Errorf("Supported values for 'controller_type are %v", strings.Join(DiskControllerTypes, ", ")))
    }

Do you find it better to redeclare the list to prevent an iteration here? I'm on the fence so I'll let you decide.

[stack72]

This looks good as is - i'm just nit-picking :)

[sputnik13]

bit of a moot point since it's merged already, but... the use of a map may improve lookup performance, but for a small set like this it's probably negligible and it wouldn't matter much unless the operation was frequent. Also, there's value in having DiskControllerTypes as an exported list for users of the module. Turning the list in to a map in that case would be detrimental as DiskControllerTypes would have to be iterated as a map given there's no key extraction facility for maps in go.

[dkalleg]

Thanks @stack72 !

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.