cloud/container: Initial import

cloud/container: Initial import

[jbd, 2014-11-15 22:59:22]

The initial import of the Container Engine API client. See https://cloud.google.com/container-engine/docs/v1beta1 for the API reference. There are tweaks to be made, and docs to be enhanced. Once we agree on the interfaces, I'll get to them.

[jbd, 2014-11-15 23:00:26]

CC'ing jbeda and bburns for the API review.

[crawshaw, 2014-11-16 14:43:30]

I'm on vacation until Thursday, so here's a cursory review. Generally looks good. Is there a testing story? Does container engine have a lightweight fake implementation you can run against? https://codereview.appspot.com/178750043/diff/20001/cloud.go File cloud.go (right): https://codereview.appspot.com/178750043/diff/20001/cloud.go#newcode61 cloud.go:61: vals["container_service"], _ = container.New(c) This registration system seems backwards. To use one of these APIs, you have to depend on packages for all of them. This package could have: var services = make(map[string]func(context.Context) interface{}) func Register(name string, fn func(context.Context) interface{}) { services[name] = fn } in WithContext you'd have: for name, fn := range services { vals[name] = fn(c) } with each API package having something like: func init() { cloud.Register("pubsub_service", New) } https://codereview.appspot.com/178750043/diff/20001/container/container.go File container/container.go (right): https://codereview.appspot.com/178750043/diff/20001/container/container.go#ne... container/container.go:15: // Package container contains a Google Container Engine API client. how about: // Package container is a Google Container Engine client. https://codereview.appspot.com/178750043/diff/20001/container/container.go#ne... container/container.go:40: OpDone = OpStatus("done") I don't see any name conflicts that require the Op* or Status* prefixes on the constant values. https://codereview.appspot.com/178750043/diff/20001/container/container.go#ne... container/container.go:89: // ContainerIpv4CIDR is the IP addresses of the container pods in Typically written IPv4 (see net package). And below.

[jbeda_google, 2014-11-17 00:39:21]

I'll defer to Brendan and the golang folks here. On Sun, Nov 16, 2014 at 6:43 AM, <crawshaw@golang.org> wrote: > I'm on vacation until Thursday, so here's a cursory review. Generally > looks good. > > Is there a testing story? Does container engine have a lightweight fake > implementation you can run against? > > > https://codereview.appspot.com/178750043/diff/20001/cloud.go > File cloud.go (right): > > https://codereview.appspot.com/178750043/diff/20001/cloud.go#newcode61 > cloud.go:61: vals["container_service"], _ = container.New(c) > This registration system seems backwards. To use one of these APIs, you > have to depend on packages for all of them. > > This package could have: > > var services = make(map[string]func(context.Context) interface{}) > > func Register(name string, fn func(context.Context) interface{}) { > services[name] = fn > } > > in WithContext you'd have: > > for name, fn := range services { > vals[name] = fn(c) > } > > with each API package having something like: > > func init() { > cloud.Register("pubsub_service", New) > } > > https://codereview.appspot.com/178750043/diff/20001/container/container.go > File container/container.go (right): > > https://codereview.appspot.com/178750043/diff/20001/ > container/container.go#newcode15 > container/container.go:15: // Package container contains a Google > Container Engine API client. > how about: > > // Package container is a Google Container Engine client. > > https://codereview.appspot.com/178750043/diff/20001/ > container/container.go#newcode40 > container/container.go:40: OpDone = OpStatus("done") > I don't see any name conflicts that require the Op* or Status* prefixes > on the constant values. > > https://codereview.appspot.com/178750043/diff/20001/ > container/container.go#newcode89 > container/container.go:89: // ContainerIpv4CIDR is the IP addresses of > the container pods in > Typically written IPv4 (see net package). And below. > > https://codereview.appspot.com/178750043/ > 

[jbd, 2014-11-20 20:10:39]

https://codereview.appspot.com/178750043/diff/20001/cloud.go File cloud.go (right): https://codereview.appspot.com/178750043/diff/20001/cloud.go#newcode61 cloud.go:61: vals["container_service"], _ = container.New(c) On 2014/11/16 14:43:30, crawshaw wrote: > This registration system seems backwards. To use one of these APIs, you have to > depend on packages for all of them. > > This package could have: > > var services = make(map[string]func(context.Context) interface{}) > > func Register(name string, fn func(context.Context) interface{}) { > services[name] = fn > } > > in WithContext you'd have: > > for name, fn := range services { > vals[name] = fn(c) > } > > with each API package having something like: > > func init() { > cloud.Register("pubsub_service", New) > } package init-time initialization won't work, because we get the authorization header from the context. The services should be initiated from the packages lazily and should be added to the context for the reuse. I'll send it in another another CL. https://codereview.appspot.com/178750043/diff/20001/container/container.go File container/container.go (right): https://codereview.appspot.com/178750043/diff/20001/container/container.go#ne... container/container.go:15: // Package container contains a Google Container Engine API client. On 2014/11/16 14:43:30, crawshaw wrote: > how about: > > // Package container is a Google Container Engine client. Done. https://codereview.appspot.com/178750043/diff/20001/container/container.go#ne... container/container.go:40: OpDone = OpStatus("done") On 2014/11/16 14:43:30, crawshaw wrote: > I don't see any name conflicts that require the Op* or Status* prefixes on the > constant values. Unified them. Since the API is pretty new and immature, I'd assume than there will be eventually conflicts in the future. In order not to break any interfaces, I preferred to go with prefixes. https://codereview.appspot.com/178750043/diff/20001/container/container.go#ne... container/container.go:89: // ContainerIpv4CIDR is the IP addresses of the container pods in On 2014/11/16 14:43:30, crawshaw wrote: > Typically written IPv4 (see net package). And below. Done.

[crawshaw, 2014-11-20 20:16:42]

https://codereview.appspot.com/178750043/diff/20001/cloud.go File cloud.go (right): https://codereview.appspot.com/178750043/diff/20001/cloud.go#newcode61 cloud.go:61: vals["container_service"], _ = container.New(c) On 2014/11/20 20:10:39, jbd wrote: > On 2014/11/16 14:43:30, crawshaw wrote: > > This registration system seems backwards. To use one of these APIs, you have > to > > depend on packages for all of them. > > > > This package could have: > > > > var services = make(map[string]func(context.Context) interface{}) > > > > func Register(name string, fn func(context.Context) interface{}) { > > services[name] = fn > > } > > > > in WithContext you'd have: > > > > for name, fn := range services { > > vals[name] = fn(c) > > } > > > > with each API package having something like: > > > > func init() { > > cloud.Register("pubsub_service", New) > > } > > package init-time initialization won't work, because we get the authorization > header from the context. The services should be initiated from the packages > lazily and should be added to the context for the reuse. > > I'll send it in another another CL. In the code I have here, the function somethingservice.New is registered with the cloud package. The function is not invoked until WithContext is invoked, so you have the appropriate c.

[jbd, 2014-11-20 20:17:54]

> Is there a testing story? Does container engine have a lightweight fake implementation you can run against? Since there is nothing more interesting than proto-to-proto conversions, we mainly depend on integration tests across the other clients. In the Container API's case, there is no lightweight staging API. We might spin a cluster with two machines which adds another 5-7 mins to the entire time required to pass the integration tests.

[crawshaw, 2014-11-20 20:22:41]

On 2014/11/20 20:17:54, jbd wrote: > > Is there a testing story? Does container engine have a lightweight fake > implementation you can run against? > > Since there is nothing more interesting than proto-to-proto conversions, we > mainly depend on integration tests across the other clients. > > In the Container API's case, there is no lightweight staging API. We might spin > a cluster with two machines which adds another 5-7 mins to the entire time > required to pass the integration tests. Wow, an API users can't write tests against. Ouch.

[crawshaw, 2014-11-20 20:24:32]

LGTM https://codereview.appspot.com/178750043/diff/40001/container/container.go File container/container.go (right): https://codereview.appspot.com/178750043/diff/40001/container/container.go#ne... container/container.go:61: // StatusError, StatusProvisioning, StatusRunning or StatusStopping. Out of date comment? https://codereview.appspot.com/178750043/diff/40001/container/container.go#ne... container/container.go:151: // OpDone or OpPending. Looks to be out of date.

[jbd, 2014-11-20 20:24:48]

> In the code I have here, the function somethingservice.New is registered with the cloud package. The function is not invoked until WithContext is invoked, so you have the appropriate c. Sorry, I should have read the code before replying. In any case, cloud wont import the generated packages once we lazily initiate them from the client packages. In container.go, func rawService(ctx context.Context) *raw.Service { // see if there is a container_service, return it // if not, create a new one and put in to the values map // return the service }

[jbd, 2014-11-20 20:26:10]

> Wow, an API users can't write tests against. Ouch. You can mock the responses at the RoundTripper level. It's a P4 at the moment though.