ec2: Implement AccountAttributes + tests

ec2: Implement AccountAttributes + tests

Added a new AccountAttributes ec2 method, that implements
DescribeAccountAttributes API call. Also extended ec2test
package to simulate the call and added SetInitialAttributes
to initialize the test server's internal attributes map.

https://code.launchpad.net/~dimitern/goamz/100-discover-default-vpc/+merge/218855

(do not edit description out of merge proposal)

[dimitern, 2014-05-08 17:32:19]

Please take a look.

[natefinch, 2014-05-08 18:17:42]

LGTM with a little cleanup.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1017
ec2/ec2.go:1017: // See http://goo.gl/hBc28j for more details.
please use real urls, not shortened ones, then the reader has at least some clue
what the url is about.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1040
ec2/ec2.go:1040: // attributes. The only supported values are SupportedPlatforms
and
This makes it sound like there are other values with are not supported, but
looking at amazon's docs, these are the only values.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1044
ec2/ec2.go:1044: func (ec2 *EC2) AccountAttributes(attrNames ...string) (resp
*AccountAttributesResp, err error) {
If it were me, I'd make the attribute names a different type:
type AccountAttrName string

const (
     SupportedPlatforms AccountAttrName = "supported-platforms"
)
func (ec2 *EC2) AccountAttributes(attrNames ...AccountAttrName)
(*AccountAttributesResp, error)

This is the typical Go way of making poor-man's enums.

I think it makes it more clear that you shouldn't just pass in any old string
(and in fact, a string won't work.... though unfortunately, string-literals will
still work).

Also, there's no need to name these return values.  It's best to avoid naming
return values unless it adds clarity, or is necessary for the implementation,
neither of which seem to be true here.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2_test.go
File ec2/ec2_test.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/ec2_test.go#newcode813
ec2/ec2_test.go:813: attrName := ec2.SupportedPlatforms
assigning to attrName seems to just make this code harder to read.  I think just
passing it in directly and checking against it directly is more clear.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/ec2t_test.go#newcode156
ec2/ec2t_test.go:156: c.Assert(attr.Values[0], Not(Equals), "")
can't we check for the specific value we set up above?

https://codereview.appspot.com/99030044/diff/1/ec2/responses_test.go
File ec2/responses_test.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/responses_test.go#newcode925
ec2/responses_test.go:925: // http://goo.gl/hBc28j
as with the other URLs, please make these full URLs as well (yes I know they're
crazy long).

[niemeyer, 2014-05-08 22:05:04]

LGTM with the type name change.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1015
ec2/ec2.go:1015: // All supported AccountAttributes options.
I would drop that const list for the time being. Allow people to use the
attribute names directly instead. Nothing in the code below seems to limit the
supported set of attributes.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1017
ec2/ec2.go:1017: // See http://goo.gl/hBc28j for more details.
On 2014/05/08 18:17:41, nate.finch wrote:
> please use real urls, not shortened ones, then the reader has at least some
clue
> what the url is about.

We're explicitly saying what it is: it contains more information about that
method. The reason for shortening is because these URLs are long and overly
verbose, and that pattern is already being used in this package, so let's not
change it like that. Either all or none.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1023
ec2/ec2.go:1023: // Attribute describes an Amazon account attribute.
// AccountAttribute holds information about an account attribute.

The type name should be changed: this is an account attribute, specifically. Not
an EC2 attribute.

There's also no reason to say "an Amazon" or "for Amazon" in most of these
cases. This is goamz, so everything here is about Amazon.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1040
ec2/ec2.go:1040: // attributes. The only supported values are SupportedPlatforms
and
On 2014/05/08 18:17:41, nate.finch wrote:
> This makes it sound like there are other values with are not supported, but
> looking at amazon's docs, these are the only values.

It would be fine to say that if there was actually any restriction, but looking
at the code, I don't see why these would be the only supported values. If Amazon
adds a new one, they are immediately supported too, right? If so, I agree with
Nate that the sentence should go away.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1044
ec2/ec2.go:1044: func (ec2 *EC2) AccountAttributes(attrNames ...string) (resp
*AccountAttributesResp, err error) {
On 2014/05/08 18:17:41, nate.finch wrote:
> This is the typical Go way of making poor-man's enums.

There's nothing poor about these enums. They are actually better than most
languages support for typed sequential constants.

That said, I suggest getting rid of the const set entirely, at least for the
time being. There are just two values, seldom used, and the code is not
restricted to them. Just allow people to use strings until we know better.

> Also, there's no need to name these return values.  It's best to avoid naming
> return values unless it adds clarity, or is necessary for the implementation,
> neither of which seem to be true here.

+1

https://codereview.appspot.com/99030044/diff/1/ec2/responses_test.go
File ec2/responses_test.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/responses_test.go#newcode925
ec2/responses_test.go:925: // http://goo.gl/hBc28j
On 2014/05/08 18:17:41, nate.finch wrote:
> as with the other URLs, please make these full URLs as well (yes I know
they're
> crazy long).

Again, please no.

[natefinch, 2014-05-09 14:57:19]

On 2014/05/08 22:05:04, niemeyer wrote:
> We're explicitly saying what it is: it contains more information about that
> method. The reason for shortening is because these URLs are long and overly
> verbose, and that pattern is already being used in this package, so let's not
> change it like that. Either all or none.

Yeah, Dimiter made the same point.  I still prefer full URLs, but since the rest
of the code is done this way, and since I'm evidently in the minority, that's
ok.

> That said, I suggest getting rid of the const set entirely, at least for the
> time being. There are just two values, seldom used, and the code is not
> restricted to them. Just allow people to use strings until we know better.

I don't know that there's a reason to get rid of these two well-known values. 
By keeping the parameter on the function a string, you can still pass in any
value you want, but if you want to use one of these values, you can.  I don't
see a reason to remove them and make people look up the string in amazon's help
docs.

[dimitern, 2014-05-09 15:02:00]

Please take a look.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1015
ec2/ec2.go:1015: // All supported AccountAttributes options.
On 2014/05/08 22:05:04, niemeyer wrote:
> I would drop that const list for the time being. Allow people to use the
> attribute names directly instead. Nothing in the code below seems to limit the
> supported set of attributes.

Done.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1017
ec2/ec2.go:1017: // See http://goo.gl/hBc28j for more details.
On 2014/05/08 18:17:41, nate.finch wrote:
> please use real urls, not shortened ones, then the reader has at least some
clue
> what the url is about.

I just followed the rest of the doc comments format, and AWS API URLs are really
long, hence the shortening.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1023
ec2/ec2.go:1023: // Attribute describes an Amazon account attribute.
On 2014/05/08 22:05:04, niemeyer wrote:
> // AccountAttribute holds information about an account attribute.
> 
> The type name should be changed: this is an account attribute, specifically.
Not
> an EC2 attribute.
> 
> There's also no reason to say "an Amazon" or "for Amazon" in most of these
> cases. This is goamz, so everything here is about Amazon.

Done.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1040
ec2/ec2.go:1040: // attributes. The only supported values are SupportedPlatforms
and
On 2014/05/08 18:17:41, nate.finch wrote:
> This makes it sound like there are other values with are not supported, but
> looking at amazon's docs, these are the only values.

Removed the sentence.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1040
ec2/ec2.go:1040: // attributes. The only supported values are SupportedPlatforms
and
On 2014/05/08 22:05:04, niemeyer wrote:
> On 2014/05/08 18:17:41, nate.finch wrote:
> > This makes it sound like there are other values with are not supported, but
> > looking at amazon's docs, these are the only values.
> 
> It would be fine to say that if there was actually any restriction, but
looking
> at the code, I don't see why these would be the only supported values. If
Amazon
> adds a new one, they are immediately supported too, right? If so, I agree with
> Nate that the sentence should go away.

That's true - we don't restrict the values here, and if Amazon adds new values
we'll support them automatically.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2.go#newcode1044
ec2/ec2.go:1044: func (ec2 *EC2) AccountAttributes(attrNames ...string) (resp
*AccountAttributesResp, err error) {
On 2014/05/08 22:05:04, niemeyer wrote:
> On 2014/05/08 18:17:41, nate.finch wrote:
> > This is the typical Go way of making poor-man's enums.
> 
> There's nothing poor about these enums. They are actually better than most
> languages support for typed sequential constants.
> 
> That said, I suggest getting rid of the const set entirely, at least for the
> time being. There are just two values, seldom used, and the code is not
> restricted to them. Just allow people to use strings until we know better.
> 
> > Also, there's no need to name these return values.  It's best to avoid
naming
> > return values unless it adds clarity, or is necessary for the
implementation,
> > neither of which seem to be true here.
> 
> +1

Constants removed and named return values dropped.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2_test.go
File ec2/ec2_test.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/ec2_test.go#newcode813
ec2/ec2_test.go:813: attrName := ec2.SupportedPlatforms
On 2014/05/08 18:17:41, nate.finch wrote:
> assigning to attrName seems to just make this code harder to read.  I think
just
> passing it in directly and checking against it directly is more clear.

Done.

https://codereview.appspot.com/99030044/diff/1/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/ec2t_test.go#newcode156
ec2/ec2t_test.go:156: c.Assert(attr.Values[0], Not(Equals), "")
On 2014/05/08 18:17:41, nate.finch wrote:
> can't we check for the specific value we set up above?

Not really, because this test is used both for the live suite and local server
suite, so we'll either get "none" or "vpc-1a2b3c4e" or something from EC2 or the
value we set in the local server "vpc-xxxxxx". I didn't want to have 2 identical
separate test cases for local and live tests.

https://codereview.appspot.com/99030044/diff/1/ec2/responses_test.go
File ec2/responses_test.go (right):

https://codereview.appspot.com/99030044/diff/1/ec2/responses_test.go#newcode925
ec2/responses_test.go:925: // http://goo.gl/hBc28j
On 2014/05/08 18:17:41, nate.finch wrote:
> as with the other URLs, please make these full URLs as well (yes I know
they're
> crazy long).

This is for consistency with the rest of the code.

[dimitern, 2014-05-09 15:04:40]

*** Submitted:

ec2: Implement AccountAttributes + tests

Added a new AccountAttributes ec2 method, that implements
DescribeAccountAttributes API call. Also extended ec2test
package to simulate the call and added SetInitialAttributes
to initialize the test server's internal attributes map.

R=nate.finch, niemeyer
CC=
https://codereview.appspot.com/99030044