environ/ec2: Implement Addresses() method

environ/ec2: Implement Addresses() method

Refactor DNSName into the new generic Addresses method,
including mapping for both DNSName and PrivateDNSName, but
not ip addresses yet.

Note this requires a new version of goamz for the test to
pass, as a hardcoded string for DNSName in the testing
server is asserted against.

https://code.launchpad.net/~gz/juju-core/ec2_addresses/+merge/179724

(do not edit description out of merge proposal)

[gz, 2013-08-12 15:09:21]

Please take a look.

[jameinel, 2013-08-13 13:50:28]

I'd like Nate to have a finger in this, just to help him follow what is going on
in this space. But LGTM.

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

https://codereview.appspot.com/12767043/diff/1/environs/ec2/ec2.go#newcode100
environs/ec2/ec2.go:100: }
If we are fetching the instance information again, should we be setting
inst.Instance = insts[0].(*ec2Instance)
before we return it?

Should this just be an internal "Refresh()" call?

https://codereview.appspot.com/12767043/diff/1/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/12767043/diff/1/environs/ec2/local_test.go#new...
environs/ec2/local_test.go:353: Value:        fmt.Sprintf("%s.testing.invalid",
inst.Id()),
I'm guessing this is the goamz change?

Is it possible to query goamz directly for the name rather than formatting it
ourselves? (Then we can be compatible even if the specific mapping changes in
the future.)

[fwereade, 2013-08-13 16:15:27]

+1 to jam's comments, but LGTM when he's happy.

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

https://codereview.appspot.com/12767043/diff/1/environs/ec2/ec2.go#newcode100
environs/ec2/ec2.go:100: }
On 2013/08/13 13:50:28, jameinel wrote:
> If we are fetching the instance information again, should we be setting
> inst.Instance = insts[0].(*ec2Instance)
> before we return it?
> 
> Should this just be an internal "Refresh()" call?

This sounds sane to me.

https://codereview.appspot.com/12767043/diff/1/environs/ec2/ec2.go#newcode111
environs/ec2/ec2.go:111: NetworkName:  "",
Can we lose the empty field?

[gz, 2013-08-13 16:59:09]

On 2013/08/13 13:50:28, jameinel wrote:
> I'd like Nate to have a finger in this, just to help him follow what is going
on
> in this space. But LGTM.

Forgot to use --author on commit, but this is one of the branches Nate and I
paired on.

[gz, 2013-08-13 16:59:20]

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

https://codereview.appspot.com/12767043/diff/1/environs/ec2/ec2.go#newcode100
environs/ec2/ec2.go:100: }
On 2013/08/13 13:50:28, jameinel wrote:
> If we are fetching the instance information again, should we be setting
> inst.Instance = insts[0].(*ec2Instance)
> before we return it?
> 
> Should this just be an internal "Refresh()" call?

The same (nearly) question came up with the openstack version. Currently the
logic is like this because it's what DNSName does, so this is the least-breaking
option. However, refreshing internally doesn't feel like the right interface
once everything else is sorted out. Factoring it into a Refresh() seems
reasonable for now.

https://codereview.appspot.com/12767043/diff/1/environs/ec2/ec2.go#newcode109
environs/ec2/ec2.go:109: Value:        ec2Inst.DNSName,
Hm, need to actually verify DNSName exists before doing this, requerying the
instances doesn't ensure it'll be there yet.

https://codereview.appspot.com/12767043/diff/1/environs/ec2/ec2.go#newcode111
environs/ec2/ec2.go:111: NetworkName:  "",
On 2013/08/13 16:15:27, fwereade wrote:
> Can we lose the empty field?

Ah yes, it's a struct not a function call so can just omit it...

https://codereview.appspot.com/12767043/diff/1/environs/ec2/local_test.go
File environs/ec2/local_test.go (right):

https://codereview.appspot.com/12767043/diff/1/environs/ec2/local_test.go#new...
environs/ec2/local_test.go:353: Value:        fmt.Sprintf("%s.testing.invalid",
inst.Id()),
On 2013/08/13 13:50:28, jameinel wrote:
> I'm guessing this is the goamz change?
> 
> Is it possible to query goamz directly for the name rather than formatting it
> ourselves? (Then we can be compatible even if the specific mapping changes in
> the future.)

Ideally. The ec2test server is pretty basic though, and currently everything's
just hardcoded. Fixing that to be less limited would be great, but a fair bit of
extra churn.

[gz, 2013-08-15 10:50:00]

Please take a look.