Implement API handler for VCH deletion [specific ci=Group23-VIC-Machine-Service]

[zjs]

Introduce a pair of handlers for deleting VCHs within a vSphere target or datacenter. By default, deletion includes the VCH and powered off containers. Deletion may optionally include powered on containers and/or volume stores as well.

If any containers remain, the VCH is not deleted. If the VCH is not deleted, the response includes a non-2xx status code.

Define a suite of end-to-end tests which verify the intended deletion behavior. End-to-end tests do not attempt to verify the behavior of concurrent operations.

Fixes #6137
Fixes #6139

[zjs]

CI was successful prior to rebase: https://ci.vcna.io/vmware/vic/14524

[anchal-agrawal]

This can be collapsed to:

if err = deleteVCH(op, d, params.DeletionSpecification); err != nil {
    ...
}

[zjs]

It can be collapsed, but I don't think doing so improves readability.

With the current structure, err is not as tightly scoped as it could be, but the overall flow is clear. By collapsing the assignments into initialization statements of the conditional, I think it becomes harder to read the overall flow of the function.

I think it's best to limit our use of optional initialization statements to calls where the sole purpose of the statement is the conditional. Typically, these are calls which are free of side-effects.

This allows someone skimming the code to easily see what's happening. In the following example, it's clear that the overall flow is to check something, do something (and handle any errors), validate something, call a method, and then do something else (and handle any errors).

if total := a+b+c; total <= 0 {
   ...
}

d, err := doThingOne(a, b, c)
if err != nil {
   ...
}

if err = validateResult(d); err != nil {
   ...
}

d.callThisMethod()

err = doThingThree(e)
if err != nil {
    ...
}

We can re-write this, collapsing more aggressively, and have the same behavior.

if total := a+b+c; total <= 0 {
   ...
}

d, err := doThingOne(a, b, c)
if err != nil {
   ...
}

if err = validateResult(d); err != nil {
   ...
}

d.callThisMethod()

if err = doThingThree(e); err != nil {
    ...
}

However, this structure emphasizes the conditional nature of doThingThree (i.e., that it may result in an error) instead of emphasizing that we're doing something (and then handling an error).

The difference is subtle, but I think it improves readability.

Are there advantages to the collapsed version I may be overlooking?

[anchal-agrawal]

Sounds good! I brought this up purely for idiomatic purposes - if variables assigned in an if block aren't used outside of it and an error is being checked in the said block, we can perform the assignment and the check on the same line. However, if it makes more sense to separate them for readability, we can do that.

[anchal-agrawal]

This can be collapsed to:

if err = deleteVCH(op, d, params.DeletionSpecification); err != nil {
    ...
}

[anchal-agrawal]

This error block can be collapsed too.

[anchal-agrawal]

This error block can be collapsed too.

[anchal-agrawal]

Where is "all" coming from? We should consider making it a constant since we're using it in two places here and it may be referenced in other places in the future.

[zjs]

Good catch! "all" is defined in Swagger, and is captured in a generated constant.

I could use that here, but that seems conceptually wrong (since this code shouldn't "know about" swagger).

I'll look for a better solution.

[zjs]

What do you think of the approach in b5b48d0? I don't love it, but it avoids "magic constants", provides type safety, avoids coupling the management layer to swagger, and is easily extended if we introduce more granular deletion specifications.

[anchal-agrawal]

Looks great - thanks!

[mdharamadas1]

lgtm