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#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#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.
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 go:570: var doc networkInterfaceDoc
state/machine.
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 params/ internal. go (right):
File state/api/
https:/ /codereview. appspot. com/89260044/ diff/10021/ state/api/ params/ internal. go#newcode320 params/ internal. go:320: ProviderId instance.NetworkId
state/api/
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 NetworkInfo there.
idea of having environs/network package, so I added that and moved
environs.
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 go:1063: if err = erfaces. FindId( doc.Id) .One(nil) ; err == nil {
state/machine.
m.st.networkInt
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/