environs: Add AllocateAddress(instId, netId) call

environs: Add AllocateAddress(instId, netId) call

This introduces a new Environ interface method which will
be used to allocate extra addresses for an instance. It is
the first step towards enabling container addressability for
EC2 and MAAS in a unified way.

Right now only the dummy provider has an implementation for
the new method (for testing), EC2 will be first to implement
it (in a follow-up, not to clutter the review).

https://code.launchpad.net/~dimitern/juju-core/430-environs-allocate-instance-address/+merge/218491

(do not edit description out of merge proposal)

[dimitern, 2014-05-06 18:48:56]

Please take a look.

[natefinch, 2014-05-06 19:17:45]

LGTM with a little test cleanup.

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:196: c.Assert(address, gc.DeepEquals, expectAddress)
Can you put these three lines inside the test itself, not inside the assert? 
It's confusing to have the assert function also have the code we're testing in
it.  I think it would be clearer to put the allocate statement and these two
asserts inside the main body of the test.

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:197: for {
why is this loop here? The only two options inside the select statement are
return or end the test with an error.

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:208: case <-time.After(15 * time.Second):
This timeout seems insanely long.  Can we just use testing.ShortWait?  This
should be immediate if it works at all, right?

[fwereade, 2014-05-06 19:41:54]

+1 to nate's comments; and I'd really like to see the network/address
integration you speak of; but I also understand that, in time, it will become
clearer exactly how that should be done. LGTM also.

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:176: expectAddress := instance.NewAddress("0.1.2.1",
instance.NetworkCloudLocal)
can we move these Address things into a network package?

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:196: c.Assert(address, gc.DeepEquals, expectAddress)
On 2014/05/06 19:17:45, nate.finch wrote:
> Can you put these three lines inside the test itself, not inside the assert? 
> It's confusing to have the assert function also have the code we're testing in
> it.  I think it would be clearer to put the allocate statement and these two
> asserts inside the main body of the test.  

or you could call it assertAllocateAddress, which would be more in line with
what you're doing (I think) but even then the "expectNetId" is pretty confusing
because you're not really *expecting* it at all.

[dimitern, 2014-05-06 20:26:55]

Please take a look.

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:176: expectAddress := instance.NewAddress("0.1.2.1",
instance.NetworkCloudLocal)
On 2014/05/06 19:41:54, fwereade wrote:
> can we move these Address things into a network package?

I'll do that, in another follow-up if you don't mind.

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:196: c.Assert(address, gc.DeepEquals, expectAddress)
On 2014/05/06 19:17:45, nate.finch wrote:
> Can you put these three lines inside the test itself, not inside the assert? 
> It's confusing to have the assert function also have the code we're testing in
> it.  I think it would be clearer to put the allocate statement and these two
> asserts inside the main body of the test.  

They were there initially, but it seemed repetitious at first. Will move them
back.

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:196: c.Assert(address, gc.DeepEquals, expectAddress)
On 2014/05/06 19:41:54, fwereade wrote:
> On 2014/05/06 19:17:45, nate.finch wrote:
> > Can you put these three lines inside the test itself, not inside the assert?

> > It's confusing to have the assert function also have the code we're testing
in
> > it.  I think it would be clearer to put the allocate statement and these two
> > asserts inside the main body of the test.  
> 
> or you could call it assertAllocateAddress, which would be more in line with
> what you're doing (I think) but even then the "expectNetId" is pretty
confusing
> because you're not really *expecting* it at all.

Now it should make more sense.

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:197: for {
On 2014/05/06 19:17:45, nate.finch wrote:
> why is this loop here? The only two options inside the select statement are
> return or end the test with an error.

It was shamelessly copied from the machine agent tests. For loop removed.

https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
environs/jujutest/tests.go:208: case <-time.After(15 * time.Second):
On 2014/05/06 19:17:45, nate.finch wrote:
> This timeout seems insanely long.  Can we just use testing.ShortWait?  This
> should be immediate if it works at all, right?

Done.

[natefinch, 2014-05-06 20:52:27]

On 2014/05/06 20:26:55, dimitern wrote:
> Please take a look.
> 
> https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go
> File environs/jujutest/tests.go (right):
> 
>
https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
> environs/jujutest/tests.go:176: expectAddress :=
instance.NewAddress("0.1.2.1",
> instance.NetworkCloudLocal)
> On 2014/05/06 19:41:54, fwereade wrote:
> > can we move these Address things into a network package?
> 
> I'll do that, in another follow-up if you don't mind.
> 
>
https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
> environs/jujutest/tests.go:196: c.Assert(address, gc.DeepEquals,
expectAddress)
> On 2014/05/06 19:17:45, nate.finch wrote:
> > Can you put these three lines inside the test itself, not inside the assert?

> > It's confusing to have the assert function also have the code we're testing
in
> > it.  I think it would be clearer to put the allocate statement and these two
> > asserts inside the main body of the test.  
> 
> They were there initially, but it seemed repetitious at first. Will move them
> back.
> 
>
https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
> environs/jujutest/tests.go:196: c.Assert(address, gc.DeepEquals,
expectAddress)
> On 2014/05/06 19:41:54, fwereade wrote:
> > On 2014/05/06 19:17:45, nate.finch wrote:
> > > Can you put these three lines inside the test itself, not inside the
assert?
> 
> > > It's confusing to have the assert function also have the code we're
testing
> in
> > > it.  I think it would be clearer to put the allocate statement and these
two
> > > asserts inside the main body of the test.  
> > 
> > or you could call it assertAllocateAddress, which would be more in line with
> > what you're doing (I think) but even then the "expectNetId" is pretty
> confusing
> > because you're not really *expecting* it at all.
> 
> Now it should make more sense.
> 
>
https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
> environs/jujutest/tests.go:197: for {
> On 2014/05/06 19:17:45, nate.finch wrote:
> > why is this loop here? The only two options inside the select statement are
> > return or end the test with an error.
> 
> It was shamelessly copied from the machine agent tests. For loop removed.
> 
>
https://codereview.appspot.com/91150044/diff/1/environs/jujutest/tests.go#new...
> environs/jujutest/tests.go:208: case <-time.After(15 * time.Second):
> On 2014/05/06 19:17:45, nate.finch wrote:
> > This timeout seems insanely long.  Can we just use testing.ShortWait?  This
> > should be immediate if it works at all, right?
> 
> Done.

That looks much better.  I actually prefer a little repetition in the tests if
it makes the code significantly easier to read and understand. simple "do x,
check err, check result" is very clear and obvious and it's almost impossible
for there to be an error in that code.

[dimitern, 2014-05-06 21:42:25]

Please take a look.