cmd/jujud: make provisioner use StateInfo from environment.

cmd/jujud: make provisioner use StateInfo from environment.

This means we can pass in a localhost address to the provisioning
agent and it will nonetheless use the correct address for zookeeper
when firing up clients.

Some rearrangement of testing code was necessary too.

https://code.launchpad.net/~rogpeppe/juju-core/provisioner-stateinfo/+merge/111598

(do not edit description out of merge proposal)

[rog, 2012-06-22 13:55:11]

Please take a look.

[niemeyer, 2012-06-22 14:19:10]

Looks appropriate. A few comments:

https://codereview.appspot.com/6336047/diff/2001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6336047/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:58: origInfo *state.Info
As we talked, this doesn't seem necessary. Just overwrite info.

https://codereview.appspot.com/6336047/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:130: return
Weird. I'm sure I had this in the review. Thanks for catching it.

https://codereview.appspot.com/6336047/diff/2001/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6336047/diff/2001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:87: // TODO check that instance is started with
the correct StateInfo.
The issue is being fixed in this CL. Tests should come with it, as the TODO will
be ignored forever otherwise.

Doesn't have to be done here, by the way. This test may be done in a test by
itself, or on a sensible existing test below.

[rog, 2012-06-22 16:16:22]

Please take a look.

https://codereview.appspot.com/6336047/diff/2001/cmd/jujud/provisioning.go
File cmd/jujud/provisioning.go (right):

https://codereview.appspot.com/6336047/diff/2001/cmd/jujud/provisioning.go#ne...
cmd/jujud/provisioning.go:58: origInfo *state.Info
On 2012/06/22 14:19:10, niemeyer wrote:
> As we talked, this doesn't seem necessary. Just overwrite info.

Done.

https://codereview.appspot.com/6336047/diff/2001/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6336047/diff/2001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:87: // TODO check that instance is started with
the correct StateInfo.
On 2012/06/22 14:19:10, niemeyer wrote:
> The issue is being fixed in this CL. Tests should come with it, as the TODO
will
> be ignored forever otherwise.
> 
> Doesn't have to be done here, by the way. This test may be done in a test by
> itself, or on a sensible existing test below.

Done.

[niemeyer, 2012-06-22 16:28:53]

LGTM, thanks.

https://codereview.appspot.com/6336047/diff/5001/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6336047/diff/5001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:31: dummy.Reset()
This should be on TearDown rather than here.

https://codereview.appspot.com/6336047/diff/5001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:35: op := make(chan dummy.Operation, 20)
Let's please use something like 512 then. If it's not supposed to block we have
no reason to not use that memory for the short period that the test is running,
in exchange for not having to debug a test below next year when the number of
operations go over 20.

https://codereview.appspot.com/6336047/diff/5001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:107: // ignore
Comment may go now.

https://codereview.appspot.com/6336047/diff/5001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:203: // Use an equivalent but textually different
address and check
Nice idea.

[rog, 2012-06-22 16:34:14]

*** Submitted:

cmd/jujud: make provisioner use StateInfo from environment.

This means we can pass in a localhost address to the provisioning
agent and it will nonetheless use the correct address for zookeeper
when firing up clients.

Some rearrangement of testing code was necessary too.

R=niemeyer
CC=
https://codereview.appspot.com/6336047

https://codereview.appspot.com/6336047/diff/5001/cmd/jujud/provisioning_test.go
File cmd/jujud/provisioning_test.go (right):

https://codereview.appspot.com/6336047/diff/5001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:31: dummy.Reset()
On 2012/06/22 16:28:53, niemeyer wrote:
> This should be on TearDown rather than here.

Done.

https://codereview.appspot.com/6336047/diff/5001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:35: op := make(chan dummy.Operation, 20)
On 2012/06/22 16:28:53, niemeyer wrote:
> Let's please use something like 512 then. If it's not supposed to block we
have
> no reason to not use that memory for the short period that the test is
running,
> in exchange for not having to debug a test below next year when the number of
> operations go over 20.

Done.

https://codereview.appspot.com/6336047/diff/5001/cmd/jujud/provisioning_test....
cmd/jujud/provisioning_test.go:107: // ignore
On 2012/06/22 16:28:53, niemeyer wrote:
> Comment may go now.

Done.