Code review comment for lp:~allenap/gwacl/list-instances-2

Revision history for this message
Raphaël Badin (rvb) wrote :

Looks good.

I see you've changed ListInstances() to take a struct instead of the hosted service name as a string like all the other methods in the management API. Now I'm on the fence with this: I think we should change all the methods to take structs but maybe that's a change we should do all at once; on the other hand, starting to do it right now with new methods will save us time updating call sites in the provider later; and given how painful it is to juggle with dependencies versions in Go, maybe your choice is the right one after all…

[0]

> 216 lines (+26/-119) 3 files modified

Nice :)

[1]

30 + ServiceName string
31 +}
32 +
33 +// ListInstances returns a slice of all instances for all deployments for the
34 +// given service.

I'd keep saying "hosted service name" instead of just "service name" for consistency (and more importantly because that's how the doc refers to a service).

review: Approve

« Back to merge proposal