ec2: Add support for AWS VPCs

ec2: Add support for AWS VPCs

Added the following new API calls:
- CreateVPC
- DeleteVPC
- VPCs
(and the associated types/responses).

Most new code is in vpc.go and vpc_test.go.

These are needed in order to support VPC-related
operations on EC2, which will come in later follow-ups.
Implementation complexity in ec2test package for the
new calls is minimal.

Some changes were needed in order to support these
and the upcoming API calls. For the new ones, AWS API
version 2013-10-15 (latest) is used, while for the
existing calls use the previous API version, as before
(2011-12-15).

Added tests, updated test doubles and tested live on
EC2.

After running the EC2 live tests numerous times, I
realized some tests are leaking security groups, so
I added a deleteGroups() tests helper that retries
to ensure all groups are deleted. Also improved the
VPC tests to include retrying as well, so the live
tests are more stable and clean up after themselves.

https://code.launchpad.net/~dimitern/goamz/vpc-api-calls/+merge/204514

Requires: https://code.launchpad.net/~dimitern/goamz/lp-1275406-lpcalserversuite-testinstanceinfo/+merge/204469

(do not edit description out of merge proposal)

[dimitern, 2014-02-03 15:20:34]

Please take a look.

[gz, 2014-02-03 21:40:54]

LGTM, a couple of comments.

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

https://codereview.appspot.com/49930045/diff/1/ec2/ec2.go#newcode925
ec2/ec2.go:925: func (ec2 *EC2) CreateVpc(cidrBlock string) (resp
*CreateVpcResp, err error) {
Do want to take and pass through the instanceTenancy argument here even if we
don't need it ourselves?

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

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go#newcode631
ec2/ec2t_test.go:631: resp2, err := s.ec2.CreateVpc("1.2.0.0/18")
Is this a safe cidr to be using?

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go#newcode659
ec2/ec2t_test.go:659: _, err = s.ec2.DeleteVpc(id1)
This feels race/eventual consistency prone, perhaps not an issue for vpcs or
delete calls specifically?

https://codereview.appspot.com/49930045/diff/1/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/ec2test/server.go#newcode217
ec2/ec2test/server.go:217: // Not done: tag, tag-key, tag-value.
Do we want an error not implemented type of thing rather than just not matching?

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

https://codereview.appspot.com/49930045/diff/1/ec2/responses_test.go#newcode594
ec2/responses_test.go:594: <CreateVpcResponse
xmlns="http://ec2.amazonaws.com/doc/2013-10-15/">
We probably don't want to mix and match schema versions in our example output
unless we really don't have a feature we need in an older schema.

[dimitern, 2014-02-04 09:00:18]

Please take a look.

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

https://codereview.appspot.com/49930045/diff/1/ec2/ec2.go#newcode925
ec2/ec2.go:925: func (ec2 *EC2) CreateVpc(cidrBlock string) (resp
*CreateVpcResp, err error) {
On 2014/02/03 21:40:54, gz wrote:
> Do want to take and pass through the instanceTenancy argument here even if we
> don't need it ourselves?

It's not supported as an argument in API version 2011-12-15.

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

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go#newcode631
ec2/ec2t_test.go:631: resp2, err := s.ec2.CreateVpc("1.2.0.0/18")
On 2014/02/03 21:40:54, gz wrote:
> Is this a safe cidr to be using?

Yes, it passes even live tests :)

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go#newcode659
ec2/ec2t_test.go:659: _, err = s.ec2.DeleteVpc(id1)
On 2014/02/03 21:40:54, gz wrote:
> This feels race/eventual consistency prone, perhaps not an issue for vpcs or
> delete calls specifically?

I run them multiple times live without issues so far.

https://codereview.appspot.com/49930045/diff/1/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/ec2test/server.go#newcode217
ec2/ec2test/server.go:217: // Not done: tag, tag-key, tag-value.
On 2014/02/03 21:40:54, gz wrote:
> Do we want an error not implemented type of thing rather than just not
matching?

It's technically not an error, but since this is just a test double, I don't
think it's a big deal to error out.

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

https://codereview.appspot.com/49930045/diff/1/ec2/responses_test.go#newcode594
ec2/responses_test.go:594: <CreateVpcResponse
xmlns="http://ec2.amazonaws.com/doc/2013-10-15/">
On 2014/02/03 21:40:54, gz wrote:
> We probably don't want to mix and match schema versions in our example output
> unless we really don't have a feature we need in an older schema.

You're right. I found the API docs for version 2011-12-15 and updated the
example responses, as well as the code to conform to that (i.e. not IsDefault
and no InstanceTenancy fields anymore).

[dimitern, 2014-02-04 09:37:29]

Please take a look.

[dimitern, 2014-02-05 13:25:44]

Please take a look.

[dimitern, 2014-02-05 13:27:47]

Please take a look.

[rog, 2014-02-05 14:29:14]

LGTM with some suggestions, mostly documentation related.

https://codereview.appspot.com/49930045/diff/40001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:212: return false, fmt.Errorf("tag, tag-key, and tag-value
filters not implemented")
fmt.Errorf("%s filter not implemented", attr)

?

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode122
ec2/ec2.go:122: if params["Version"] == "" {
Given that makeParams now always creates the Version
entry, why bother with this?

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode183
ec2/ec2.go:183: func makeParamsVPC(action string) map[string]string {
I'm not sure this really justifies itself over
just calling makeParamsForVersion(action, VPCAPIVersion)
when necessary.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode187
ec2/ec2.go:187: func makeParamsForVersion(version, action string)
map[string]string {
makeParamsWithVersion(action, version string) map[string]string
?

then makeParamsWithVersion is just makeParams with an extra argument.
(i *think* that With is more appropriate than For in this case).

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:1095: func collectIds(form url.Values, prefix string)
map[string]bool {
doc comment?

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode26
ec2/vpc.go:26: // AWS API version used for VPC-related calls.
Do we actually want to export this?

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode53
ec2/vpc.go:53: // The smallest VPC you can create uses a /28 netmask (16 IP
s/you can create/that can be created/

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode57
ec2/vpc.go:57: // The supported tenancy options for instances launched into the
// The instanceTenancy value holds the tenancy options for
// instances launched into the VPC - either DefaultTenancy
// or DedicatedTenancy.
//
// See http://goo.gl/nkwjvN for more details.

?

I think the details of the tenancy semantics can reasonably
be left to the amazon docs.

We should also document that instanceTenancy may be empty
or (my preference) just leave the description unchanged,
and lose the special case for instanceTenancy == "".

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode79
ec2/vpc.go:79: // DeleteVPC deletes the specified VPC. You must detach or delete
all
I'm not keen on the "you" phrasing (it might not be
the reader that's responsible for doing it).

I'd prefer something like this:

// DeleteVPC deletes the VPC with the specified id.
// All gateways and resources that are associated with
// the VPC must have been previously deleted,
// including instances running in the VPC, and non-default security
// groups and route tables associated with the VPC.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc_test.go
File ec2/vpc_test.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc_test.go#newcode77
ec2/vpc_test.go:77: // VPC tests run against either a local test server or live
on EC2.
s/run/to run/
?

[dimitern, 2014-02-06 11:32:22]

Please take a look.

https://codereview.appspot.com/49930045/diff/40001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:212: return false, fmt.Errorf("tag, tag-key, and tag-value
filters not implemented")
On 2014/02/05 14:29:14, rog wrote:
> fmt.Errorf("%s filter not implemented", attr)
> 
> ?

It was changed in later patch sets.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode122
ec2/ec2.go:122: if params["Version"] == "" {
On 2014/02/05 14:29:14, rog wrote:
> Given that makeParams now always creates the Version
> entry, why bother with this?

Just to be on the safe side, but fair enough. Removed.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode183
ec2/ec2.go:183: func makeParamsVPC(action string) map[string]string {
On 2014/02/05 14:29:14, rog wrote:
> I'm not sure this really justifies itself over
> just calling makeParamsForVersion(action, VPCAPIVersion)
> when necessary.

I'd prefer to leave it, because it's shorter to write - it's just a helper
anyway.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode187
ec2/ec2.go:187: func makeParamsForVersion(version, action string)
map[string]string {
On 2014/02/05 14:29:14, rog wrote:
> makeParamsWithVersion(action, version string) map[string]string
> ?
> 
> then makeParamsWithVersion is just makeParams with an extra argument.
> (i *think* that With is more appropriate than For in this case).

Done.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:1095: func collectIds(form url.Values, prefix string)
map[string]bool {
On 2014/02/05 14:29:14, rog wrote:
> doc comment?

Done.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode26
ec2/vpc.go:26: // AWS API version used for VPC-related calls.
On 2014/02/05 14:29:14, rog wrote:
> Do we actually want to export this?

Not really. Done.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode53
ec2/vpc.go:53: // The smallest VPC you can create uses a /28 netmask (16 IP
On 2014/02/05 14:29:14, rog wrote:
> s/you can create/that can be created/

Done.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode57
ec2/vpc.go:57: // The supported tenancy options for instances launched into the
On 2014/02/05 14:29:14, rog wrote:
> // The instanceTenancy value holds the tenancy options for
> // instances launched into the VPC - either DefaultTenancy
> // or DedicatedTenancy.
> //
> // See http://goo.gl/nkwjvN for more details.
> 
> ?
> 
> I think the details of the tenancy semantics can reasonably
> be left to the amazon docs.
> 
> We should also document that instanceTenancy may be empty
> or (my preference) just leave the description unchanged,
> and lose the special case for instanceTenancy == "".

I'll change the comment as suggested, but will change the if below to set it to
DefaultTenancy when empty, because we can't pass it as "" to AWS.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode79
ec2/vpc.go:79: // DeleteVPC deletes the specified VPC. You must detach or delete
all
On 2014/02/05 14:29:14, rog wrote:
> I'm not keen on the "you" phrasing (it might not be
> the reader that's responsible for doing it).
> 
> I'd prefer something like this:
> 
> // DeleteVPC deletes the VPC with the specified id.
> // All gateways and resources that are associated with
> // the VPC must have been previously deleted,
> // including instances running in the VPC, and non-default security
> // groups and route tables associated with the VPC.

Done.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc_test.go
File ec2/vpc_test.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc_test.go#newcode77
ec2/vpc_test.go:77: // VPC tests run against either a local test server or live
on EC2.
On 2014/02/05 14:29:14, rog wrote:
> s/run/to run/
> ?

Done.

[rog, 2014-02-06 14:42:07]

LGTM with a few further suggestions below.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode57
ec2/vpc.go:57: // The supported tenancy options for instances launched into the
On 2014/02/06 11:32:22, dimitern wrote:
> On 2014/02/05 14:29:14, rog wrote:
> > // The instanceTenancy value holds the tenancy options for
> > // instances launched into the VPC - either DefaultTenancy
> > // or DedicatedTenancy.
> > //
> > // See http://goo.gl/nkwjvN for more details.
> > 
> > ?
> > 
> > I think the details of the tenancy semantics can reasonably
> > be left to the amazon docs.
> > 
> > We should also document that instanceTenancy may be empty
> > or (my preference) just leave the description unchanged,
> > and lose the special case for instanceTenancy == "".
> 
> I'll change the comment as suggested, but will change the if below to set it
to
> DefaultTenancy when empty, because we can't pass it as "" to AWS.

If the user passes "" to the function, they deserve any error they get - the
function doesn't document that that's allowed (it actually explicitly documents
that instanceTenancy should be either DefaultTenancy or DedicatedTenancy)

ec2 doesn't accept "furble" either, but we don't check that, and I don't think
there's a need to.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go#newcode656
ec2/ec2t_test.go:656: func (s *ServerTests) errorCode(err error, code string)
bool {
When I've done this function before, I've implemented it as:

func errorCode(err error) string {
    if err, _ := err.(*ec2.Error); err != nil {
        return err.Code
    }
    return ""
}

then the call looks like:

  if errorCode(err) == "InvalidGroup.NotFound" {

I think that reads a little better, and it's more
flexible.

I'd actually suggest doing that, and then a subsequent
CL could change other tests to use it (I count about 5 places).

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2test/server.go#new...
ec2/ec2test/server.go:1096: // returns a map with the ids as keys for easier
lookup.
s/ for easier lookup././

No particular need for the rationalisation.

[gz, 2014-02-06 16:25:06]

LGTM.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2.go#newcode177
ec2/ec2.go:177: return makeParamsWithVersion(action, "2011-12-15")
I'd stick "2011-12-15" in a const too. Nice adaptation to allow version passing.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go#newcode627
ec2/ec2t_test.go:627: func (s *ServerTests) deleteGroups(c *C, groups
[]ec2.SecurityGroup) {
This is somewhat ick, but I like that there's enough logging going on so
failures that hit the timeout should be clear enough.

[dimitern, 2014-02-07 12:35:24]

Please take a look.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2.go#newcode177
ec2/ec2.go:177: return makeParamsWithVersion(action, "2011-12-15")
On 2014/02/06 16:25:07, gz wrote:
> I'd stick "2011-12-15" in a const too. Nice adaptation to allow version
passing.

Done.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go#newcode627
ec2/ec2t_test.go:627: func (s *ServerTests) deleteGroups(c *C, groups
[]ec2.SecurityGroup) {
On 2014/02/06 16:25:07, gz wrote:
> This is somewhat ick, but I like that there's enough logging going on so
> failures that hit the timeout should be clear enough.

Yeah, it's more resilient than just trying to delete them and not checking the
error :)

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go#newcode656
ec2/ec2t_test.go:656: func (s *ServerTests) errorCode(err error, code string)
bool {
On 2014/02/06 14:42:08, rog wrote:
> When I've done this function before, I've implemented it as:
> 
> func errorCode(err error) string {
>     if err, _ := err.(*ec2.Error); err != nil {
>         return err.Code
>     }
>     return ""
> }
> 
> then the call looks like:
> 
>   if errorCode(err) == "InvalidGroup.NotFound" {
> 
> I think that reads a little better, and it's more
> flexible.
> 
> I'd actually suggest doing that, and then a subsequent
> CL could change other tests to use it (I count about 5 places).
> 

Done.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2test/server.go#new...
ec2/ec2test/server.go:1096: // returns a map with the ids as keys for easier
lookup.
On 2014/02/06 14:42:08, rog wrote:
> s/ for easier lookup././
> 
> No particular need for the rationalisation.

Done.

[niemeyer, 2014-02-12 13:23:49]

LGTM with trivial points.

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2.go#newcode31
ec2/ec2.go:31: // VPC-related requests.
Moving the VPC API version here as well would make it more clear. The constant
isn't even used in vpc.go either way.

Also, is there a known reason not to use the new API version across the board?

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2t_test.go#newcode657
ec2/ec2t_test.go:657: func (s *ServerTests) errorCode(err error) string {
This can be a function (s is unnecessary).

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode8
ec2/vpc.go:8: // Written by Gustavo Niemeyer <gustavo.niemeyer@canonical.com>
Please drop this line. We should drop it from all the files.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode19
ec2/vpc.go:19: PendingState   = "pending"
The names of these constants do not look entirely sane. Although this is inside
the vpc.go file, this is living inside the ec2 package, which means it's
ec2.PendingState, and ec2.DefaultTenancy, which is a lot more broad than
"supported values for VPC state".

I suggest either fixing them to include the fact they are about VPC
(VPCPendingState, for example), or just dropping the constants until we have a
better idea.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode24
ec2/vpc.go:24: DedicatedTenancy = "dedicated"
Same as above.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode30
ec2/vpc.go:30: // VPC describes an Amazon Virtual Private Cloud (VPC).
s/an Amazon Virtual/a Virtual/

We might have other pre-existent instances of that in the documentation, but we
should eventually fix them as well to not refer to Amazon. First, because  every
single documentation entry refers to something coming out of Amazon, and second,
because this will work with whoever implements this API.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc_test.go
File ec2/vpc_test.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc_test.go#newcode8
ec2/vpc_test.go:8: // Written by Gustavo Niemeyer
<gustavo.niemeyer@canonical.com>
Please drop this line.

[dimitern, 2014-02-12 13:43:48]

*** Submitted:

ec2: Add support for AWS VPCs

Added the following new API calls:
- CreateVPC
- DeleteVPC
- VPCs
(and the associated types/responses).

Most new code is in vpc.go and vpc_test.go.

These are needed in order to support VPC-related
operations on EC2, which will come in later follow-ups.
Implementation complexity in ec2test package for the
new calls is minimal.

Some changes were needed in order to support these
and the upcoming API calls. For the new ones, AWS API
version 2013-10-15 (latest) is used, while for the
existing calls use the previous API version, as before
(2011-12-15).

Added tests, updated test doubles and tested live on
EC2.

After running the EC2 live tests numerous times, I
realized some tests are leaking security groups, so
I added a deleteGroups() tests helper that retries
to ensure all groups are deleted. Also improved the
VPC tests to include retrying as well, so the live
tests are more stable and clean up after themselves.

R=
CC=
https://codereview.appspot.com/49930045

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2.go#newcode31
ec2/ec2.go:31: // VPC-related requests.
On 2014/02/12 13:23:49, niemeyer wrote:
> Moving the VPC API version here as well would make it more clear. The constant
> isn't even used in vpc.go either way.
> 
> Also, is there a known reason not to use the new API version across the board?

I'll move it here. The reason for not using the newer API version is that some
things are changed, possibly in an incompatible way (responses include different
sub sections, more filters, certain arguments have changed). I didn't want to
make a massive change to all current API calls to make them compatible with
2013-10-15. I could do it in a follow-up, after the current chain have landed
though.

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2t_test.go#newcode657
ec2/ec2t_test.go:657: func (s *ServerTests) errorCode(err error) string {
On 2014/02/12 13:23:49, niemeyer wrote:
> This can be a function (s is unnecessary).

Done.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode8
ec2/vpc.go:8: // Written by Gustavo Niemeyer <gustavo.niemeyer@canonical.com>
On 2014/02/12 13:23:49, niemeyer wrote:
> Please drop this line. We should drop it from all the files.

I'll remove it from all files then.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode19
ec2/vpc.go:19: PendingState   = "pending"
On 2014/02/12 13:23:49, niemeyer wrote:
> The names of these constants do not look entirely sane. Although this is
inside
> the vpc.go file, this is living inside the ec2 package, which means it's
> ec2.PendingState, and ec2.DefaultTenancy, which is a lot more broad than
> "supported values for VPC state".
> 
> I suggest either fixing them to include the fact they are about VPC
> (VPCPendingState, for example), or just dropping the constants until we have a
> better idea.

I'll drop them and update the doc comments.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode24
ec2/vpc.go:24: DedicatedTenancy = "dedicated"
On 2014/02/12 13:23:49, niemeyer wrote:
> Same as above.

Done.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode30
ec2/vpc.go:30: // VPC describes an Amazon Virtual Private Cloud (VPC).
On 2014/02/12 13:23:49, niemeyer wrote:
> s/an Amazon Virtual/a Virtual/
> 
> We might have other pre-existent instances of that in the documentation, but
we
> should eventually fix them as well to not refer to Amazon. First, because 
every
> single documentation entry refers to something coming out of Amazon, and
second,
> because this will work with whoever implements this API.

Done.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc_test.go
File ec2/vpc_test.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc_test.go#newcode8
ec2/vpc_test.go:8: // Written by Gustavo Niemeyer
<gustavo.niemeyer@canonical.com>
On 2014/02/12 13:23:49, niemeyer wrote:
> Please drop this line.

Done.