[skip ci] Define REST API handlers for inspecting VCHs by zjs · Pull Request #6318 · vmware/vic

zjs · 2017-09-13T20:15:01Z

Introduce a pair of handlers for accessing information about the VCH with a given identifier within a vSphere target or datacenter.

There are ways in which this work is incomplete, marked by TODO:

Malformed managed object references are ignored
Malformed or unexpected PKI material is ignored

I think these are acceptable for now. We can address proper handling of invalid data returned by lower layers later, in a holistic way.

Resolves #6029.

jzt · 2017-09-13T23:03:16Z

Lots of these files are in need of copyright headers.

jzt · 2017-09-13T23:09:38Z

@zjs I was thinking back on the portlayer when looking at this and wondering if we couldn't also benefit here from a refactor in order to organize the handlers into discrete chunks. Its a little different here since the portlayer is broken into each of its different component handlers (containers, images, storage, etc), but we could perhaps set configure_vic_machine.go up in a similar scheme. We could have a list of handlers like this and basically call configure for each one. That way maybe we could have a much cleaner configure_vic_machine.go with a collection of files by operation, like: vch_get.go, vch_list.go, vch_create.go, etc.

Probably not necessary now but maybe if we start thinking about it we can avoid a big refactor later on down the line.

zjs · 2017-09-14T00:04:51Z

We could have a list of handlers like this and basically call configure for each one.

When I started writing configure_vic_machine.go, I noticed that there seemed to be multiple approaches used by different projects. I followed the pattern Ivan recommended.

The impression I got was that the portlayer approach ended up being unnecessarily complex; it makes the configure_*.go file a bit shorter, but it's much less clear which handler will be invoked for a given API call since that information is now spread out across all of the Configure methods.

I'm not attached to any specific pattern, and there may be advantages to the portlayer approach I'm not aware of.

jzt · 2017-09-14T16:11:27Z

lib/apiservers/service/restapi/configure_vic_machine.go

+	// GET /container/target/{target}/datacenter/{datacenter}/vch
+	api.GetTargetTargetDatacenterDatacenterVchHandler = &handlers.VCHDatacenterListGet{}
+
+	// POST /container/target/{target}/datacenter/{datacenter}/vch


Minor, but maybe we could edit the spec such that the /container/target/{target}/datacenter/{datacenter}/vch pattern becomes something like /container/target/{address}/datacenter/{name}/vch so we end up with method names like api.GetTargetAddressDatacenterNameVchHandler instead of api.GetTargetTargetDatacenterDatacenterVchHandler.

Again, pretty minor - not worth the time spent on it if you don't see it as a long term improvement.

I really don't like the method names, and am open to changing the spec.

I worry that including vague things like {name} or {id} in the path might limit us in the future. If we start with a path like /foo/{name} and then want to add a sub-resource bar, we can't have /foo/{name}/bar/{name}, so we're stuck being inconsistent (with something like /foo/{name}/bar/{barName}) or refactoring (to something like /foo/{fooName}/bar/{barName}) and likely breaking clients using generated bindings.

To avoid painting ourselves into a corner like that, I think we'd want to qualify any generally-applicable parameters up front. (So {address} is probably okay as-is, but {name} or {id} should probably be qualified.)

I also think readability of the generated documentation is at least as important as readability of the generated go code, so we should keep that in mind as we consider changes.

Given that, maybe we should compare both the generated documentation and generate method names for the current style (/foo/{foo}) with some potential new styles like /foo/{fooId}?

I agree with your concerns around {name} and {id}, especially in cases where you could supply a name or an id (datacenters/datastores for example?). I like the /foo/{fooID} suggestion as fooID can imply whatever it is you are using to identify a particular foo, and is not restricted to being a name or a UUID or whatever, but can be anything.

zjs · 2017-09-14T17:39:16Z

Lots of these files are in need of copyright headers.

This should be fixed across all of my pending PRs.

jzt · 2017-09-14T18:00:58Z

@zjs once this is merged I will use it to build from when I take one of the tasks from the epic.

emlin · 2017-09-15T15:51:28Z