Code review comment for lp://staging/~dimitern/juju-core/027-state-supports-nonced-provisioning

Please take a look.

https://codereview.appspot.com/8561044/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode407
state/machine.go:407: func (m *Machine) SetProvisioned(id InstanceId,
nonce string) error {
On 2013/04/09 13:26:27, fwereade wrote:
> Please check that neither id nor nonce are "".

I wanted to postpone that, in case there is a use case in tests to reset
the provisioned state, by passing "", "". But this is not needed now, so
I'll add the check, thanks.

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode410
state/machine.go:410: {{"instanceid", ""}},
On 2013/04/09 13:26:27, fwereade wrote:
> Hmm. I think it might be better to simplify this -- just assert that
instanceid
> and nonce are empty, and leave the rest of the error handling as is.

> I'm worried that this is a situation in which arbitrarily delayed
transactions
> can have real-world effects, but I can't see what to do about it. One
to think
> about...

I raised my concerns online about replaying transactions, etc. I'll do
it.

https://codereview.appspot.com/8561044/diff/1/state/machine.go#newcode418
state/machine.go:418: errMsg := "cannot set instance id of machine %q:
%v"
On 2013/04/09 13:26:27, fwereade wrote:
> defer trivial.ErrorContextf(...)

Ha! One place where using it is actually a good idea :)

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner.go#newcode272
worker/provisioner/provisioner.go:272: }
On 2013/04/09 13:26:27, fwereade wrote:
> nonce := "fake_nonce"

Done.

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner.go#newcode274
worker/provisioner/provisioner.go:274: inst, err :=
p.environ.StartInstance(m.Id(), "fake_nonce", m.Series(), cons,
stateInfo, apiInfo)
On 2013/04/09 13:26:27, fwereade wrote:
> s/"fake_nonce"/nonce/

Done.

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner.go#newcode280
worker/provisioner/provisioner.go:280: if err1 :=
m.SetStatus(params.MachineError, err.Error()); err1 != nil {
On 2013/04/09 13:26:27, fwereade wrote:
> s/err1/err/, I think?

We'll lose the context of the previous error, won't we?

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner.go#newcode287
worker/provisioner/provisioner.go:287: // TODO(dimitern) generate an
unique random nonce in a follow-up.
On 2013/04/09 13:31:41, TheMue wrote:
> Take trivial.NewUUID() for it.

That's coming in the follow-up :)

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner.go#newcode288
worker/provisioner/provisioner.go:288: if err :=
m.SetProvisioned(inst.Id(), ""); err != nil {
On 2013/04/09 13:26:27, fwereade wrote:
> I think SetProvisioned with an empty nonce should fail, really.
s/""/nonce/
> anyway.

Done.

https://codereview.appspot.com/8561044/diff/1/worker/provisioner/provisioner.go#newcode292
worker/provisioner/provisioner.go:292: // killed again.
On 2013/04/09 13:26:27, fwereade wrote:
> We should try to stop the instance right away anyway, I think. TODO it
maybe?

We'll do that a bit later in the loop. But I guess it's worth a TODO.

https://codereview.appspot.com/8561044/