juju/ec2: add code to accept an ec2 region in the configuration file.

https://code.launchpad.net/~rogpeppe/juju/go-juju-ec2-region/+merge/84256

(do not edit description out of merge proposal)

[niemeyer, 2011-12-02 16:13:05]

Please propose it again so that we get the real diff.

[niemeyer, 2011-12-06 02:37:32]

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go
File juju/ec2/config.go (right):

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go#newcode27
juju/ec2/config.go:27: "us-west-1":      aws.USWest,
This changes is a bit precipitated. First, we're not able to deploy anywhere, so
we don't really care about region fine-tuning yet. Then, the *environment*
configuration is not the proper place for region selection. Please have a look
at the placement/constraint specification or thread in the mailing list for more
details on how we'll have to handle that at some point.

[rog, 2011-12-06 09:31:00]

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go
File juju/ec2/config.go (right):

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go#newcode27
juju/ec2/config.go:27: "us-west-1":      aws.USWest,
On 2011/12/06 02:37:32, niemeyer wrote:
> This changes is a bit precipitated. First, we're not able to deploy anywhere,
so
> we don't really care about region fine-tuning yet. Then, the *environment*
> configuration is not the proper place for region selection. Please have a look
> at the placement/constraint specification or thread in the mailing list for
more
> details on how we'll have to handle that at some point.

it's not the fine tuning i'm concerned with here - i need to  a valid EC2 url
that i can use to contact EC2 and create a bootstrap machine (the next step).
this checking matches the checking done in environment/config.py. what would you
suggest i do otherwise?

[niemeyer, 2011-12-06 11:37:56]

LGTM

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go
File juju/ec2/config.go (right):

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go#newcode21
juju/ec2/config.go:21: // TODO move these known strings into goamz/aws?
Yes, that sounds like a good plan, but let's get it going here first.

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go#newcode27
juju/ec2/config.go:27: "us-west-1":      aws.USWest,
Indeed, my apologies. I've read region, but thought about zones. That's
completely unrelated, and I shouldn't have sent the review so late yesterday.
Sorry.

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go#newcode30
juju/ec2/config.go:30: // AddRegion adds an AWS region to the set of known
regions.
If we actually want to make the map tweakable, just publish the map
(s/regions/Regions). That said, I'd simply remove AddRegion and RemoveRegion for
now, and move the regions map onto goamz/aws later.

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go#newcode66
juju/ec2/config.go:66: func maybeString(x interface{}) string {
This would be more useful as:

maybeString(x interface{}, deflt string) string {...}

Saves an "if" above.

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config_test.go
File juju/ec2/config_test.go (right):

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config_test.go#newc...
juju/ec2/config_test.go:56: c.Check(ec2env.config, Equals, t.config)
This is testing the internal implementation, which can change without affecting
the application. This is a bit unfortunate, and I'd rather not have this as a
whitebox test, but can't think of a better idea right now.

[rog, 2011-12-06 12:22:46]

(test changed to use FitsTypeOf too.)

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go
File juju/ec2/config.go (right):

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go#newcode30
juju/ec2/config.go:30: // AddRegion adds an AWS region to the set of known
regions.
On 2011/12/06 11:37:57, niemeyer wrote:
> If we actually want to make the map tweakable, just publish the map
> (s/regions/Regions). That said, I'd simply remove AddRegion and RemoveRegion
for
> now, and move the regions map onto goamz/aws later.

Done.

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config.go#newcode66
juju/ec2/config.go:66: func maybeString(x interface{}) string {
On 2011/12/06 11:37:57, niemeyer wrote:
> This would be more useful as:
> 
> maybeString(x interface{}, deflt string) string {...}
> 
> Saves an "if" above.

Done.

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config_test.go
File juju/ec2/config_test.go (right):

https://codereview.appspot.com/5449065/diff/5001/juju/ec2/config_test.go#newc...
juju/ec2/config_test.go:56: c.Check(ec2env.config, Equals, t.config)
On 2011/12/06 11:37:57, niemeyer wrote:
> This is testing the internal implementation, which can change without
affecting
> the application. This is a bit unfortunate, and I'd rather not have this as a
> whitebox test, but can't think of a better idea right now.

the difficulty is that from the point of view of the application, this is all
inside the black box - but we need to test what's going on inside it. i think
that white box testing is ok here for that reason.

another possibility would be to export the environ and environProvider types
from ec2, something i'd considered anyway.

[niemeyer, 2011-12-06 12:29:02]

LGTM, thanks!

[niemeyer, 2011-12-13 21:58:46]

This was merged already.