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

Thanks, LGTM with trivials.

https://codereview.appspot.com/89260044/diff/1/environs/broker.go
File environs/broker.go (right):

https://codereview.appspot.com/89260044/diff/1/environs/broker.go#newcode51
environs/broker.go:51: ProviderId string
On 2014/04/18 15:37:26, dimitern wrote:
> On 2014/04/18 14:41:18, fwereade wrote:
> > We could make this clearer if we had a type for it, like instance.Id
-- as it
> is
> > I think ProviderId is not much of a win over NetworkId. But
`NetworkId
> > NetworkId` should be less confusing.

> I wanted to make the code use consistent naming - NetworkId is only
used here. I
> like the idea of type NetworkId string and then perhaps use ProviderId
NetworkId
> as a compromise?

Yeah, that works for me. Thanks.

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 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.

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
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.

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 {
.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.

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