Code review comment for lp:~dimitern/juju-core/394-network-name-to-id-and-add-tags

Looking very good, but I think the txn bits need a bit of work; and I
think the NotFound changes are bad. What's the motivation there?

https://codereview.appspot.com/86010044/diff/30001/environs/broker.go
File environs/broker.go (right):

https://codereview.appspot.com/86010044/diff/30001/environs/broker.go#newcode44
environs/broker.go:44: NetworkName string
If we're commenting fields (+1 to that) let's have blank lines before
comments.

https://codereview.appspot.com/86010044/diff/30001/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/86010044/diff/30001/errors/errors.go#newcode38
errors/errors.go:38: return ok || (err != nil &&
strings.HasSuffix(err.Error(), " not found"))
On 2014/04/10 15:48:10, rog wrote:
> why?
> this seems wrong to me.

Yeah, I'm not too keen on this myself.

https://codereview.appspot.com/86010044/diff/30001/errors/errors.go#newcode127
errors/errors.go:127: return ok || (err != nil &&
strings.HasSuffix(err.Error(), " already exists"))
On 2014/04/10 15:48:10, rog wrote:
> same here.

Yeah, I has a scared.

https://codereview.appspot.com/86010044/diff/30001/state/address.go
File state/address.go (right):

https://codereview.appspot.com/86010044/diff/30001/state/address.go#newcode152
state/address.go:152: NetworkScope instance.NetworkScope
`bson:",omitempty"`
hmm. Scope should really be on Network, shouldn't it...

That's probably not one for this CL, though.

https://codereview.appspot.com/86010044/diff/30001/state/address.go#newcode162
state/address.go:162: NetworkScope instance.NetworkScope
`bson:",omitempty"`
ditto

https://codereview.appspot.com/86010044/diff/30001/state/apiserver/provisioner/provisioner.go
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/86010044/diff/30001/state/apiserver/provisioner/provisioner.go#newcode421
state/apiserver/provisioner/provisioner.go:421: stateNetworks[i] =
state.NetworkParams{
I'm wondering whether NetworkInfo is less overloaded -- the params
package confuses the issue a bit

https://codereview.appspot.com/86010044/diff/30001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/86010044/diff/30001/state/machine.go#newcode829
state/machine.go:829: type NetworkParams struct {
s/available on an instance/ I think -- and move these to networks.go?

https://codereview.appspot.com/86010044/diff/30001/state/machine.go#newcode1052
state/machine.go:1052: for i := 0; i < 5; i++ {
On 2014/04/10 15:48:10, rog wrote:
> similar question to State.AddNetwork, why are we iterating here?

We shouldn't be iterating like this anyway, I just realised -- if
there's any reason to iterate, there's reason to reread state inside the
loop.

https://codereview.appspot.com/86010044/diff/30001/state/machine.go#newcode1072
state/machine.go:1072: // For some reason when using unique indices with
mgo, and
On 2014/04/10 15:48:10, rog wrote:
> The only output from a transaction is ErrAborted - we never get to
know whether
> the actual operations succeeded or not (because they might have been
executed on
> another machine).

> BTW if we asserted that the doc was missing, how could the insertion
fail?

Yeah, nil means that the transaction didn't fail -- not necessarily that
it succeeded. Hitherto this has only really been an issue wrt Insert vs
Update -- inserts are ignored if the doc exists, updates ignored if it
doesn't.

But, yeah -- if the mac address is the _id, how come the DocMissing is
not being triggered?

https://codereview.appspot.com/86010044/diff/30001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/86010044/diff/30001/state/state.go#newcode1055
state/state.go:1055: switch err {
if err == "oh noes connection fell over", we should return it instead of
just retrying repeatedly. Probably the case for the other construction
like this too.

https://codereview.appspot.com/86010044/