state: add InjectMachine

state: add InjectMachine

...whcih is AddMachine + SetInstanceId, as in lp:1025925

https://code.launchpad.net/~fwereade/juju-core/state-inject-machine/+merge/141179

(do not edit description out of merge proposal)

[fwereade, 2012-12-23 12:46:05]

Please take a look.

[dimitern, 2013-01-03 22:03:03]

On 2012/12/23 12:46:05, fwereade wrote:
> Please take a look.

LGTM

[jameinel, 2013-01-06 10:28:54]

I'm not 100% clear on the background for this, but the code matches what you
said it did, and the test coverage is sufficient. (I'd personally write it as
several tests each asserting only 1 thing, but that doesn't seem to be the
standard methodology in that test suite.)

LGTM

https://codereview.appspot.com/7005054/diff/1/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/7005054/diff/1/cmd/jujud/bootstrap.go#newcode52
cmd/jujud/bootstrap.go:52: m, err :=
st.InjectMachine(state.InstanceId(c.InstanceId), state.JobManageEnviron)
Unrelated to your change, why the need to cast an InstanceId into another
InstanceId? Shouldn't there be a common type somewhere?

[fwereade, 2013-01-07 08:38:08]

*** Submitted:

state: add InjectMachine

...whcih is AddMachine + SetInstanceId, as in lp:1025925

R=dimitern, jameinel
CC=
https://codereview.appspot.com/7005054

https://codereview.appspot.com/7005054/diff/1/cmd/jujud/bootstrap.go
File cmd/jujud/bootstrap.go (right):

https://codereview.appspot.com/7005054/diff/1/cmd/jujud/bootstrap.go#newcode52
cmd/jujud/bootstrap.go:52: m, err :=
st.InjectMachine(state.InstanceId(c.InstanceId), state.JobManageEnviron)
On 2013/01/06 10:28:54, jameinel wrote:
> Unrelated to your change, why the need to cast an InstanceId into another
> InstanceId? Shouldn't there be a common type somewhere?

The InstanceId field is a string; didn't really seem worth making a new
gnuflag.Value for this one case when we could just cast it here.

[rog, 2013-01-07 09:09:31]

too late i know, but a comment below.

in addition i find myself wondering (again) if we could just use the zero
InstanceId as invalid, and have a method InstanceId.IsValid; then the current
AddMachine (used only once in the real code) could simply be AddMachine("").

anyway, that's a distraction, please ignore me.

https://codereview.appspot.com/7005054/diff/4002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7005054/diff/4002/state/state.go#newcode151
state/state.go:151: // instance, configure to run the supplied jobs.
i'm not entirely sure about InjectMachine as a name here - I expected it to be
very different from AddMachine, but it's almost identical. "AddExistingMachine"
is longer, but perhaps might be clearer?

also:
s/machine, corresponding/machine corresponding/
s/configure/configured/