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

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with a couple of trivial thoughts below

https://codereview.appspot.com/8561044/diff/7001/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/8561044/diff/7001/state/apiserver/api_test.go#newcode912
state/apiserver/api_test.go:912: stm, err :=
s.State.AddMachine("series", state.JobHostUnits)
i have to say i found the logic here really hard to follow. i know the
logic comes straight from state, but i'd like a few comments. the use of
oldId and newId make things harder too, i think. i'd just use the string
constants.

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

https://codereview.appspot.com/8561044/diff/7001/state/machine.go#newcode439
state/machine.go:439: // an instance id and the given nonce.
i think i'd say "if the machine has been provisioned with the given
nonce", as it's not possible to call SetProvisioned *without* an
instance id.

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

« Back to merge proposal