Code review comment for lp:~flaviamissi/goamz/goamz
Revision history for this message
| flaviamissi (flaviamissi) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
Please take a look.
https:/
File elb/elb.go (right):
https:/
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:/
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/CreateLoadBal
suggestions on
> this line below.
Done.
https:/
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:/
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:/
elb/elb.go:50: type CreateLoadBalan
On 2013/02/28 21:39:19, niemeyer wrote:
> s/CreateLoadBal
Done.
https:/
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:/
elb/elb.go:61: func (elb *ELB) CreateLoadBalan
*CreateLoadBala
On 2013/02/28 21:39:19, niemeyer wrote:
> s/CreateLoadBal
Done.
https:/
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:/
elb/elb.go:84:
On 2013/02/28 21:39:19, niemeyer wrote:
> // RegisterInstanc
request.
> //
> // See http://
Done.
https:/
elb/elb.go:86: InstanceIds []string
`xml:"RegisterI
On 2013/02/28 21:39:19, niemeyer wrote:
> OMG.. beautiful XML element name.
yeah.. :|
https:/
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:/
elb/elb.go:92: func (elb *ELB)
RegisterInstanc
(resp *RegisterInstan
On 2013/02/28 21:39:19, niemeyer wrote:
> s/RegisterInsta
> 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:/
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:/
elb/elb.go:112: func (elb *ELB)
DeregisterInsta
(resp *SimpleResp, err error) {
On 2013/02/28 21:39:19, niemeyer wrote:
> s/DeregisterIns
> and inverting the parameters as mentioned above.
Done.
https:/
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:/
elb/elb.go:133: type LoadBalancerDes
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 LoadBalancerDes
amazon's api convention, although for me it is ok to use LoadBalancer.
https:/
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:/
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
DescribeLoadBal
a list of ListenerDescrip
https:/
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:/
elb/elb.go:236: func (elb *ELB) DescribeInstanc
instanceIds ...string) (*DescribeInsta
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/
that's alright.
Yeap!
https:/