Code review comment for lp:~flaviamissi/goamz/goamz

Great work, Flavia. A few initial comments:

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.
It's not clear what this means.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode27
elb/elb.go:27: type CreateLoadBalancer struct {
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.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode36
elb/elb.go:36: // Listener to configure in Load Balancer.
// 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.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode47
elb/elb.go:47: // Response to a CreateLoadBalance request.
// CreateResp represents a response to a Create request.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode50
elb/elb.go:50: type CreateLoadBalancerResp struct {
s/CreateLoadBalancerResp/CreateResp/

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode58
elb/elb.go:58: // Creates a Load Balancer in Amazon.
// 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.

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) {
s/CreateLoadBalancer/Create/g

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode70
elb/elb.go:70: // Deletes a Load Balancer.
// Delete deletes the load balancer with name.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode84
elb/elb.go:84:
// RegisterInstancesResp represents a response to a RegisterInstances
request.
//
// See http://goo.gl/x9hru for more details.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode86
elb/elb.go:86: InstanceIds []string
`xml:"RegisterInstancesWithLoadBalancerResult>Instances>member>InstanceId"`
OMG.. beautiful XML element name.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode89
elb/elb.go:89: // Register N instances with a given Load Balancer.
// 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.

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) {
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.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode93
elb/elb.go:93: // TODO: change params order and use ..., e.g (lbName
string, instanceIds ...string)
I suggest not doing that. This kind of call convention is useful mainly
when you will *type* the multiple arguments, such as in:

     log.Print("Hey", "there")

That'll pretty much never be the case with this package. People will
have lists of ids and so on, in the code itself, and will pretty much
always be operating them with slices. So it's actually more convenient
to have them as slices in the parameter list too.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode109
elb/elb.go:109: // Deregister N instances from a given Load Balancer.
// DeregisterInstances drops from the named load balancer all
// the EC2 instances with the provided instance ids.

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) {
s/DeregisterInstancesFromLoadBalancer/DeregisterInstances/

and inverting the parameters as mentioned above.

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)
Same as above.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode133
elb/elb.go:133: type LoadBalancerDescription struct {
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.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode156
elb/elb.go:156: func (elb *ELB) DescribeLoadBalancers(names ...string)
(*DescribeLoadBalancerResp, error) {
Also wondering about this one in the same context above.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode185
elb/elb.go:185: type Instance struct {
Hmm.. doesn't feel like this is a proper type to have here. This is
ec2.Instance.

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode189
elb/elb.go:189: type ListenerDescription struct {
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?

https://codereview.appspot.com/7014047/diff/6019/elb/elb.go#newcode226
elb/elb.go:226: type InstanceState struct {
Same comment as for the Instance type.

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) {
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.

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