Please take a look.


https://codereview.appspot.com/7014047/diff/6019/elb/elb.go
File elb/elb.go (right):

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode24
elb/elb.go:24: // The creation of a Load Balancer may differ inside EC2
and VPC.
On 2013/02/28 21:39:19, niemeyer wrote:
> It's not clear what this means.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode27
elb/elb.go:27: type CreateLoadBalancer struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> I think we can short names in this package. This is the elb package,
meaning
> Elastic Load Balancer. Everything here is about load balancers.

> I suggest s/CreateLoadBalancer/Create/ here. Will provide further
suggestions on
> this line below.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode36
elb/elb.go:36: // Listener to configure in Load Balancer.
On 2013/02/28 21:39:19, niemeyer wrote:
> // A Listener represents a process that listens for client connection
> // requests, and maps a port and protocol in the load balancer,
> // to a port and protocol on the backend instances.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode47
elb/elb.go:47: // Response to a CreateLoadBalance request.
On 2013/02/28 21:39:19, niemeyer wrote:
> // CreateResp represents a response to a Create request.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode50
elb/elb.go:50: type CreateLoadBalancerResp struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> s/CreateLoadBalancerResp/CreateResp/

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode58
elb/elb.go:58: // Creates a Load Balancer in Amazon.
On 2013/02/28 21:39:19, niemeyer wrote:
> // Create creates a new load balancer.

> It creates in Amazon or whoever else the endpoint represents. We have
other docs
> like this, but we should fix them over time.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode61
elb/elb.go:61: func (elb *ELB) CreateLoadBalancer(options
*CreateLoadBalancer) (resp *CreateLoadBalancerResp, err error) {
On 2013/02/28 21:39:19, niemeyer wrote:
> s/CreateLoadBalancer/Create/g

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode70
elb/elb.go:70: // Deletes a Load Balancer.
On 2013/02/28 21:39:19, niemeyer wrote:
> // Delete deletes the load balancer with name.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode84
elb/elb.go:84:
On 2013/02/28 21:39:19, niemeyer wrote:
> // RegisterInstancesResp represents a response to a RegisterInstances
request.
> //
> // See http://goo.gl/x9hru for more details.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode86
elb/elb.go:86: InstanceIds []string
`xml:"RegisterInstancesWithLoadBalancerResult>Instances>member>InstanceId"`
On 2013/02/28 21:39:19, niemeyer wrote:
> OMG.. beautiful XML element name.

yeah.. :|

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode89
elb/elb.go:89: // Register N instances with a given Load Balancer.
On 2013/02/28 21:39:19, niemeyer wrote:
> // RegisterInstances registers into the named load balancer all
> // the EC2 instances with the provided instance ids.

> Note how as a convention in Go code, the first word in a method
documentation is
> always the method name itself.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode92
elb/elb.go:92: func (elb *ELB)
RegisterInstancesWithLoadBalancer(instanceIds []string, lbName string)
(resp *RegisterInstancesResp, err error) {
On 2013/02/28 21:39:19, niemeyer wrote:
> s/RegisterInstancesWithLoadBalancer/RegisterInstances/

> Also, can we please have the lbName name first in the parameter list?
The thing
> we're operating on generally comes first, as a common rule of thumb.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode109
elb/elb.go:109: // Deregister N instances from a given Load Balancer.
On 2013/02/28 21:39:19, niemeyer wrote:
> // DeregisterInstances drops from the named load balancer all
> // the EC2 instances with the provided instance ids.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode112
elb/elb.go:112: func (elb *ELB)
DeregisterInstancesFromLoadBalancer(instanceIds []string, lbName string)
(resp *SimpleResp, err error) {
On 2013/02/28 21:39:19, niemeyer wrote:
> s/DeregisterInstancesFromLoadBalancer/DeregisterInstances/

> and inverting the parameters as mentioned above.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode113
elb/elb.go:113: // TODO: change params order and use ..., e.g (lbName
string, instanceIds ...string)
On 2013/02/28 21:39:19, niemeyer wrote:
> Same as above.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode133
elb/elb.go:133: type LoadBalancerDescription struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> It's a bit fuzzy to me if this is a LoadBalancerInfo, or it's in fact
the
> LoadBalancer itself. Maybe we should keep LoadBalancer reserved for an
object we
> can use to manipulate things? Not sure.. I'll be thinking about this
while this
> review goes through.
I share that doubt with you, I used LoadBalancerDescription to follow
amazon's api convention, although for me it is ok to use LoadBalancer.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode185
elb/elb.go:185: type Instance struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> Hmm.. doesn't feel like this is a proper type to have here. This is
> ec2.Instance.

Done.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode189
elb/elb.go:189: type ListenerDescription struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> Also a bit of a strange type. How is PolicyName part of a listener
description
> and not part of a listener? Why do we need both a listener description
and a
> listener?
Yeah, it's amazon that makes those distinctions, when you call
DescribeLoadBalancers action, instead of a list of Listeners you'll get
a list of ListenerDescriptions...

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode226
elb/elb.go:226: type InstanceState struct {
On 2013/02/28 21:39:19, niemeyer wrote:
> Same comment as for the Instance type.

The ec2.InstanceState type has different fields, with different xml
element names, so we actually need this type here... :/

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode236
elb/elb.go:236: func (elb *ELB) DescribeInstanceHealth(lbName string,
instanceIds ...string) (*DescribeInstanceHealthResp, error) {
On 2013/02/28 21:39:19, niemeyer wrote:
> Can you please apply the same comments regarding the documentation as
the others
> above?

> Also, as described above, I think this should take instanceIds
[]string instead.

> I'll stop the review here for now, as this is a pretty large branch.
I'll come
> back to it once these initial points are addressed/discussed, if
that's alright.

Yeap!

https://codereview.appspot.com/7014047/