Implemented PublicAddress() and PrivateAddress()

Implemented PublicAddress() and PrivateAddress()

Minimal implementation of fetching public/private address for EnvironProvider for openstack, with tests.

https://code.launchpad.net/~dimitern/juju-core/openstack-stub-provider-addresses/+merge/135700

Requires: https://code.launchpad.net/~dimitern/juju-core/openstack-stub-provider-config/+merge/135454

(do not edit description out of merge proposal)

[dimitern, 2012-11-22 14:13:01]

Please take a look.

[TheMue, 2012-11-22 14:31:57]

Only two comments regarding EC2 references.

https://codereview.appspot.com/6843106/diff/1/environs/openstack/export_test.go
File environs/openstack/export_test.go (right):

https://codereview.appspot.com/6843106/diff/1/environs/openstack/export_test....
environs/openstack/export_test.go:26: // know that the ec2test server doesn't
get better with time,
Is the EC2 reference here correct? Looks like c'n'p. ;)

https://codereview.appspot.com/6843106/diff/1/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/6843106/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:174: //
http://docs.amazonwebservices.com/AWSEC2/latest/UserGuide/AESDG-chapter-insta...
Here again a reference to EC2. But maybe only I don't get the connection between
EC2 and OpenStack. In this case don't care.

[rog, 2012-11-22 15:11:48]

this looks fine. one thought though - you won't need these methods for quite a
long time (essentially until you've got bootstrap working properly), so this
code will go untested in the while for longer than desirable. might i suggest
putting this branch on hold until later?

https://codereview.appspot.com/6843106/diff/1/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/6843106/diff/1/environs/openstack/provider.go#...
environs/openstack/provider.go:30: var shortAttempt = trivial.AttemptStrategy{
does openstack have the same eventual-consistency issues as ec2? if not, you
probably won't need this at all.

[dimitern, 2012-11-22 15:13:09]

On 2012/11/22 15:11:48, rog wrote:
> this looks fine. one thought though - you won't need these methods for quite a
> long time (essentially until you've got bootstrap working properly), so this
> code will go untested in the while for longer than desirable. might i suggest
> putting this branch on hold until later?
> 
> https://codereview.appspot.com/6843106/diff/1/environs/openstack/provider.go
> File environs/openstack/provider.go (right):
> 
>
https://codereview.appspot.com/6843106/diff/1/environs/openstack/provider.go#...
> environs/openstack/provider.go:30: var shortAttempt = trivial.AttemptStrategy{
> does openstack have the same eventual-consistency issues as ec2? if not, you
> probably won't need this at all.

Yes it does, handling this as with EC2 seems correct for me.

[dimitern, 2012-11-22 15:18:28]

Please take a look.

[niemeyer, 2012-11-22 17:04:38]

Very nice. LGTM with a trivial suggestion:

https://codereview.appspot.com/6843106/diff/6001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/6843106/diff/6001/environs/openstack/provider....
environs/openstack/provider.go:178: defer trivial.ErrorContextf(&err, "cannot
get %q", uri)
We don't seem to need ErrorContextf in this function. There's a single exit
point, so return fmt.Errorf(...) at the end when err != nil should work well.

ErrorContextf is a bit of a convoluted beast, so it's best to avoid it whenever
the straightforward logic is.. well.. straightforward.

[dimitern, 2012-11-22 17:17:15]

*** Submitted:

Implemented PublicAddress() and PrivateAddress()

Minimal implementation of fetching public/private address for EnvironProvider
for openstack, with tests.

R=TheMue, rog, niemeyer
CC=
https://codereview.appspot.com/6843106

[dimitern, 2012-11-22 17:42:57]

Please take a look.