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

Please take a look.

https://codereview.appspot.com/86010044/diff/20001/names/network_test.go
File names/network_test.go (right):

https://codereview.appspot.com/86010044/diff/20001/names/network_test.go#newcode23
names/network_test.go:23: c.Assert(names.IsNetwork("42"), jc.IsTrue)
On 2014/04/09 15:34:48, fwereade wrote:
> Let's not let users do this. Same restrictions as service names
perhaps?

This is now changed, as we have two separate fields for Name and
ProviderId, the former is validated more strictly (alphanumeric with
only dashes allowed as separators, like in MAAS, but MAAS also allows
"---" or "a--b-", which I decided to forbid).

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

https://codereview.appspot.com/86010044/diff/20001/state/api/params/internal.go#newcode316
state/api/params/internal.go:316: Tag string
On 2014/04/09 15:34:48, fwereade wrote:
> Hmm, doesn't this need both the tag and the id?

Added ProviderId as well.

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

https://codereview.appspot.com/86010044/diff/20001/state/apiserver/client/client.go#newcode295
state/apiserver/client/client.go:295: includeNetworks, err :=
networkTagsToIds(args.IncludeNetworks)
On 2014/04/09 15:34:48, fwereade wrote:
> tags to names, not ids, surely?

> I've been thinking of names as juju vocab, ids as provider vocab, and
tags as
> derived from names. Am I confused?

OK, now I got you. I'll add name and id (both the same for now -
provider-specific until we change that) to networks doc and other
places.

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

https://codereview.appspot.com/86010044/diff/20001/state/apiserver/provisioner/provisioner.go#newcode464
state/apiserver/provisioner/provisioner.go:464: // Convert network ids
to tags before returning.
On 2014/04/09 15:34:48, fwereade wrote:
> Doesn't this need to be ids? They're for consumption by the provider,
right? Or
> will the provisioner be using the tags to look up the networks and get
the ids
> to pass on to StartInstance?

Dropped that and added comment. Since networks do not exist before
provisioning, we can't really look up the names in
include/excludeNetworks from state to get their provider ids, so i'm
just returning names here.

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

https://codereview.appspot.com/86010044/diff/20001/state/networkinterfaces.go#newcode23
state/networkinterfaces.go:23: NetworkId string
On 2014/04/09 15:34:48, fwereade wrote:
> This is an internal reference to a juju entity. Shouldn't it be using
the name?

Fair enough, NetworkName is the juju entity id, will use it here.
NetworkId is provider-specific (but for now both are the same), and tag
is created from the name.

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

https://codereview.appspot.com/86010044/diff/20001/state/networks.go#newcode39
state/networks.go:39: }
On 2014/04/09 15:34:48, fwereade wrote:
> It might make things clearer if we had an explicit Name() func here
that just
> happened to return the provider id?

> Would be clearest of all tbh of we had separate Name and ProviderId
fields...

Done.

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

https://codereview.appspot.com/86010044/diff/20001/state/state.go#newcode1021
state/state.go:1021: // TODO(dimitern) Start using sequences for id,
like for machines. For
On 2014/04/09 15:34:48, fwereade wrote:
> huh? surely names need to be user-specified?

See how my brain-dumping TODOs help clarify misunderstandings :)
Comment dropped.

https://codereview.appspot.com/86010044/diff/20001/state/state.go#newcode1062
state/state.go:1062: func (st *State) Network(id string) (*Network,
error) {
On 2014/04/09 15:34:48, fwereade wrote:
> I don't think we ever want to identify a network by provider id, do
we? state
> talks juju language -- we don't ever look up a machine by instance id,
do we?

Changed to name.

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