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

mostly looking good, but (1) IsVirtual on network still seems ill-formed
to me; and (2) are we explicitly ignoring compatibility with earlier
1.19? these new fields in state worry me a little, the people who want
to work with networks will likely be upgrading from there...

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

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

https://codereview.appspot.com/89260044/diff/1/state/api/params/internal.go#newcode331
state/api/params/internal.go:331: IsVirtual bool
I'm less certain about this one. If it's possible for virtual/physical
interfaces to be mixed, this is meaningless; if it's not possible, it's
not necessary on the interface definition. Right?

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

https://codereview.appspot.com/89260044/diff/1/state/machine.go#newcode1077
state/machine.go:1077: panic("unknown error while adding network
interface")
This is on the state server, let's not panic even if it should never
happen.

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