Code review comment for lp:~dimitern/juju-core/400-lp-1307513-multiple-nics-same-mac

Please take a look.

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

https://codereview.appspot.com/89260044/diff/1/state/machine.go#newcode570
state/machine.go:570: var doc networkInterfaceDoc
On 2014/04/18 16:02:54, fwereade wrote:
> On 2014/04/18 15:37:27, dimitern wrote:
> > On 2014/04/18 14:41:18, fwereade wrote:
> > > hey, I'm feeling more strongly about this now: please assert that
the
> machine
> > is
> > > Dead in this method, just so that if (when) someone tries to use
it from
> > > elsewhere they get something pointing out the race with adding
interfaces.
> >
> > Done.

> Sorry, I expressed myself poorly. The op is nice, and could/should
happily stay
> (other invariants *should* make it unnecessary, but +1 to defence in
depth), but
> what I'm really looking for is a quick check against m.doc.Life so we
can abort
> with an error before even running the txn.

As discussed, I'm adding a check for m.doc.Life == Dead at the
beginning.

https://codereview.appspot.com/89260044/diff/10021/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/89260044/diff/10021/state/api/params/internal.go#newcode320
state/api/params/internal.go:320: ProviderId instance.NetworkId
On 2014/04/18 16:02:54, fwereade wrote:
> hmm, instance.NetworkId doesn't feel quite right -- I'd say
environs.NetworkId,
> with the expectation that soon enough we'll want an environs/network
package and
> we can just call it network.Id.

> Or if you want to start the package with a single type, and start
gradually
> migrating stuff over to it as you go in future CLs, that would also
work for me.

I tried adding it to environs, but there are import loops. I like the
idea of having environs/network package, so I added that and moved
environs.NetworkInfo there.

https://codereview.appspot.com/89260044/diff/10021/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/89260044/diff/10021/state/machine.go#newcode1063
state/machine.go:1063: if err =
m.st.networkInterfaces.FindId(doc.Id).One(nil); err == nil {
On 2014/04/18 16:02:54, fwereade wrote:
> .One(&doc) perhaps? I suspect you're grabbing the data anyway, even if
you're
> not storing it, so you may as well return a doc that definitely
completely
> matches what's in the db.

> I know it doesn't make a difference *now* but it feels like an
encouragement to
> subtle bugs.

Done.

https://codereview.appspot.com/89260044/