Code review comment for lp:~hduran-8/juju-core/add_state_network_check_on_specified_machine

couple of comments, let's chat live

https://codereview.appspot.com/86430043/diff/40001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/86430043/diff/40001/state/unit.go#newcode869
state/unit.go:869: if unit.doc.MachineId != machine.Id() {
This in particular needs to go inside the loop -- unit and machine
assignments can change.

(also, should we be checking that the machine's principals are empty if
unused is true? I think we should...)

https://codereview.appspot.com/86430043/diff/40001/state/unit.go#newcode881
state/unit.go:881: if err := unit.st.supportsUnitPlacement(); err != nil
{
I don't think this is mutable state, is it? we can check it outside the
loop.

https://codereview.appspot.com/86430043/diff/40001/state/unit.go#newcode923
state/unit.go:923: }
Everything from here up to the top of the loop feels like it should all
be inside the assignToMachineOps method -- calculating the ops and
verifying that they're sane should go hand in hand. (this applies to
stuff like the life checks too -- check them while building the txn,
don't just build it blind, wait for it to fail, and then look up why.)

https://codereview.appspot.com/86430043/diff/40001/state/unit.go#newcode940
state/unit.go:940: case unit.doc.Life != Alive:
You shouldn't need this code here. It should all be checked as we
construct the transaction -- we have access to latest-known-state up at
the top of the loop, and we can and should check unit life etc *before*
we try to run a transaction asserting that it's the case.

Apart from anything else, you're inspecting old state here -- it doesn't
necessarily have any bearing on why the txn mechanism rejected the ops.

https://codereview.appspot.com/86430043/