names: New package

names: New package

This introduces a new juju-core/names pacakge,
which contains all name and tag related functions
shared between state and API: IsUnitName, UnitTag,
UnitNameFromTag, MachineTag, MachineIdFromTag,
IsServiceName, etc.

Because of the pacakge name, some functions were
renamed: names.IsUnit, IsService, UnitFromTag,
all refer to names.

In addition, a change was made to these two
functions: UnitNameFromTag and MachineIdFromTag.
Both of them now return (string, error), rather
than just string. The error return is used in
case the passed tag string has an invalid format.
Because of this change, some places needed slight
refactoring, otherwise no other changes where made.

https://code.launchpad.net/~dimitern/juju-core/081-common-names/+merge/177416

(do not edit description out of merge proposal)

[dimitern, 2013-07-29 15:26:43]

Please take a look.

[rog, 2013-07-29 16:28:44]

very nice direction. some comments below.

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go#newcode64
cmd/juju/addmachine.go:64: c.MachineId = containerSpec[sep+1:]
it seems to me like the above code (with its knowledge
of the colon-separated syntax of a container spec)
should live in names too.

https://codereview.appspot.com/12034043/diff/1/names/constants.go
File names/constants.go (right):

https://codereview.appspot.com/12034043/diff/1/names/constants.go#newcode13
names/constants.go:13: ServiceSnippet       =
"[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*"
it would be quite nice to see some better tests of these regexps - a
table-driven test of valid and invalid names might be good.

perhaps one such test could incorporate a variety of 
tests for the (currently almost untested?) IsUnitName, IsServiceName, etc
functions.

this is also important for regression testing, as we can easily break the API by
breaking these regexps.

https://codereview.appspot.com/12034043/diff/1/names/constants.go#newcode17
names/constants.go:17: ContainerSpecSnippet = "(([a-z])*:)?"
what's the reason for the inner set of brackets here?

also, do we want to allow ":46" (i'm not sure what
a "new container" spec implies) ?

https://codereview.appspot.com/12034043/diff/1/names/environ.go
File names/environ.go (right):

https://codereview.appspot.com/12034043/diff/1/names/environ.go#newcode6
names/environ.go:6: func EnvironTag(name string) string {
// EnvironTag returns the tag of an environment
// with the given name.
?

https://codereview.appspot.com/12034043/diff/1/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12034043/diff/1/names/machine.go#newcode25
names/machine.go:25: // MachineTag returns the tag for the machine wi th the
given id.
s/wi th/with/

https://codereview.appspot.com/12034043/diff/1/names/machine.go#newcode34
names/machine.go:34: // tag.
// tag, or an error if it's not the tag of a machine.

https://codereview.appspot.com/12034043/diff/1/names/machine.go#newcode37
names/machine.go:37: return "", fmt.Errorf("invalid machine tag format: %v",
tag)
"%q is not a valid machine tag" ?

https://codereview.appspot.com/12034043/diff/1/names/machine.go#newcode42
names/machine.go:42: id = strings.Replace(id, "-", "/", -1)
i wonder if we should check that it's a valid machine id here,
as this function is often the gatekeeper between externally-provided
tags and internally used names. same goes for the other XFromTag
functions.

https://codereview.appspot.com/12034043/diff/1/names/unit.go
File names/unit.go (right):

https://codereview.appspot.com/12034043/diff/1/names/unit.go#newcode19
names/unit.go:19: // UnitNameFromTag returns the unit name that was used to
create the tag.
// or an error if the tag is not of a unit.

?

https://codereview.appspot.com/12034043/diff/1/names/unit.go#newcode22
names/unit.go:22: return "", fmt.Errorf("invalid unit tag format: %v", tag)
"%q is not a valid unit tag" ?

https://codereview.appspot.com/12034043/diff/1/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/12034043/diff/1/state/api/deployer/unit.go#new...
state/api/deployer/unit.go:27: return ""
panic? i know that's not its previous behaviour,
but i suspect things are going very very wrong
if we get an error here.

https://codereview.appspot.com/12034043/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/12034043/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:104: result.Results[i].StringsWatcherId =
d.resources.Register(watch)
i think this has slipped past the bounds of reasonableness.

could we please factor out the body of this for loop into its
own function, along the same lines as UpgraderAPI.setOneAgentTools?

then we can return early rather than nesting indents.

https://codereview.appspot.com/12034043/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/12034043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner.go:77: if m.auth.AuthOwner(entity.Tag) {
similarly here.

https://codereview.appspot.com/12034043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/12034043/diff/1/state/state.go#newcode568
state/state.go:568: i := strings.Index(tag, "-")
i'd prefer to define names.TagKind to
return the kind of a given tag.
and perhaps also define (in names) some
tag kinds rather than exporting the Prefix constants,
which really require knowledge of the syntax
to use correctly.

e.g.

func TagKind(tag string) (string, error)

const (
   MachineTagKind = "machine"
   UnitTagKind = "unit"
   etc
)

[fwereade, 2013-07-30 11:29:44]

Generally agree with rog's comments, just a couple of my own.

https://codereview.appspot.com/12034043/diff/1/names/unit.go
File names/unit.go (right):

https://codereview.appspot.com/12034043/diff/1/names/unit.go#newcode32
names/unit.go:32: func IsUnitName(name string) bool {
less stuttery: names.IsUnit, names.IsService, names.IsUnitTag, etc?

https://codereview.appspot.com/12034043/diff/1/names/unit_test.go
File names/unit_test.go (right):

https://codereview.appspot.com/12034043/diff/1/names/unit_test.go#newcode22
names/unit_test.go:22: tag, err = names.UnitNameFromTag("foo")
...names.UnitFromTag?

[dimitern, 2013-07-30 14:38:36]

Please take a look.

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go#newcode64
cmd/juju/addmachine.go:64: c.MachineId = containerSpec[sep+1:]
On 2013/07/29 16:28:44, rog wrote:
> it seems to me like the above code (with its knowledge
> of the colon-separated syntax of a container spec)
> should live in names too.

Not really, this syntax here is specific to the command: "lxc:<id>" means start
an LXC container in machine <id>.

https://codereview.appspot.com/12034043/diff/1/names/constants.go
File names/constants.go (right):

https://codereview.appspot.com/12034043/diff/1/names/constants.go#newcode13
names/constants.go:13: ServiceSnippet       =
"[a-z][a-z0-9]*(-[a-z0-9]*[a-z][a-z0-9]*)*"
On 2013/07/29 16:28:44, rog wrote:
> it would be quite nice to see some better tests of these regexps - a
> table-driven test of valid and invalid names might be good.
> 
> perhaps one such test could incorporate a variety of 
> tests for the (currently almost untested?) IsUnitName, IsServiceName, etc
> functions.
> 
> this is also important for regression testing, as we can easily break the API
by
> breaking these regexps.

Added table-based tests for machine, service and unit names.

https://codereview.appspot.com/12034043/diff/1/names/constants.go#newcode17
names/constants.go:17: ContainerSpecSnippet = "(([a-z])*:)?"
On 2013/07/29 16:28:44, rog wrote:
> what's the reason for the inner set of brackets here?
> 
> also, do we want to allow ":46" (i'm not sure what
> a "new container" spec implies) ?

Inner brackets means 0 or more repetitions of [a-z]. I suppose it could be
written as (\w*:)?, but the above is more restrictive wrt Unicode support in
regular expressions perhaps.
:46 shouldn't be allowed, you're right, I fixed the expression in the inner
brackets to use + instead.
New container spec is used with add-machine and similar commands that can work
on machines or containers.

https://codereview.appspot.com/12034043/diff/1/names/environ.go
File names/environ.go (right):

https://codereview.appspot.com/12034043/diff/1/names/environ.go#newcode6
names/environ.go:6: func EnvironTag(name string) string {
On 2013/07/29 16:28:44, rog wrote:
> // EnvironTag returns the tag of an environment
> // with the given name.
> ?

Done.

https://codereview.appspot.com/12034043/diff/1/names/environ.go#newcode6
names/environ.go:6: func EnvironTag(name string) string {
On 2013/07/29 16:28:44, rog wrote:
> // EnvironTag returns the tag of an environment
> // with the given name.
> ?

Done.

https://codereview.appspot.com/12034043/diff/1/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12034043/diff/1/names/machine.go#newcode25
names/machine.go:25: // MachineTag returns the tag for the machine wi th the
given id.
On 2013/07/29 16:28:44, rog wrote:
> s/wi th/with/

Done.

https://codereview.appspot.com/12034043/diff/1/names/machine.go#newcode34
names/machine.go:34: // tag.
On 2013/07/29 16:28:44, rog wrote:
> // tag, or an error if it's not the tag of a machine.

Done.

https://codereview.appspot.com/12034043/diff/1/names/machine.go#newcode37
names/machine.go:37: return "", fmt.Errorf("invalid machine tag format: %v",
tag)
On 2013/07/29 16:28:44, rog wrote:
> "%q is not a valid machine tag" ?

Done.

https://codereview.appspot.com/12034043/diff/1/names/machine.go#newcode42
names/machine.go:42: id = strings.Replace(id, "-", "/", -1)
On 2013/07/29 16:28:44, rog wrote:
> i wonder if we should check that it's a valid machine id here,
> as this function is often the gatekeeper between externally-provided
> tags and internally used names. same goes for the other XFromTag
> functions.

Done.

https://codereview.appspot.com/12034043/diff/1/names/unit.go
File names/unit.go (right):

https://codereview.appspot.com/12034043/diff/1/names/unit.go#newcode19
names/unit.go:19: // UnitNameFromTag returns the unit name that was used to
create the tag.
On 2013/07/29 16:28:44, rog wrote:
> // or an error if the tag is not of a unit.
> 
> ?

Done.

https://codereview.appspot.com/12034043/diff/1/names/unit.go#newcode22
names/unit.go:22: return "", fmt.Errorf("invalid unit tag format: %v", tag)
On 2013/07/29 16:28:44, rog wrote:
> "%q is not a valid unit tag" ?

Done.

https://codereview.appspot.com/12034043/diff/1/names/unit.go#newcode32
names/unit.go:32: func IsUnitName(name string) bool {
On 2013/07/30 11:29:44, fwereade wrote:
> less stuttery: names.IsUnit, names.IsService, names.IsUnitTag, etc?

Done, except for IsUnitTag - I assumed you meant UnitFromTag?

https://codereview.appspot.com/12034043/diff/1/names/unit_test.go
File names/unit_test.go (right):

https://codereview.appspot.com/12034043/diff/1/names/unit_test.go#newcode22
names/unit_test.go:22: tag, err = names.UnitNameFromTag("foo")
On 2013/07/30 11:29:44, fwereade wrote:
> ...names.UnitFromTag?

Done.

https://codereview.appspot.com/12034043/diff/1/names/unit_test.go#newcode22
names/unit_test.go:22: tag, err = names.UnitNameFromTag("foo")
On 2013/07/30 11:29:44, fwereade wrote:
> ...names.UnitFromTag?

Done.

https://codereview.appspot.com/12034043/diff/1/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/12034043/diff/1/state/api/deployer/unit.go#new...
state/api/deployer/unit.go:27: return ""
On 2013/07/29 16:28:44, rog wrote:
> panic? i know that's not its previous behaviour,
> but i suspect things are going very very wrong
> if we get an error here.

Done.

https://codereview.appspot.com/12034043/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/12034043/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:104: result.Results[i].StringsWatcherId =
d.resources.Register(watch)
On 2013/07/29 16:28:44, rog wrote:
> i think this has slipped past the bounds of reasonableness.
> 
> could we please factor out the body of this for loop into its
> own function, along the same lines as UpgraderAPI.setOneAgentTools?
> 
> then we can return early rather than nesting indents.

Done.

https://codereview.appspot.com/12034043/diff/1/state/apiserver/machine/machin...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/12034043/diff/1/state/apiserver/machine/machin...
state/apiserver/machine/machiner.go:77: if m.auth.AuthOwner(entity.Tag) {
On 2013/07/29 16:28:44, rog wrote:
> similarly here.

Done.

https://codereview.appspot.com/12034043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/12034043/diff/1/state/state.go#newcode568
state/state.go:568: i := strings.Index(tag, "-")
On 2013/07/29 16:28:44, rog wrote:
> i'd prefer to define names.TagKind to
> return the kind of a given tag.
> and perhaps also define (in names) some
> tag kinds rather than exporting the Prefix constants,
> which really require knowledge of the syntax
> to use correctly.
> 
> e.g.
> 
> func TagKind(tag string) (string, error)
> 
> const (
>    MachineTagKind = "machine"
>    UnitTagKind = "unit"
>    etc
> )
> 

Done.

[rog, 2013-07-30 15:10:11]

looking very much better, thanks!
one more round of suggestions and then i think we'll be done.

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go#newcode64
cmd/juju/addmachine.go:64: c.MachineId = containerSpec[sep+1:]
On 2013/07/30 14:38:37, dimitern wrote:
> On 2013/07/29 16:28:44, rog wrote:
> > it seems to me like the above code (with its knowledge
> > of the colon-separated syntax of a container spec)
> > should live in names too.
> 
> Not really, this syntax here is specific to the command: "lxc:<id>" means
start
> an LXC container in machine <id>.

it looks like the names package supports this syntax (e.g.
names.ContainerSpecSnippet) and so the parsing for it
should probably live there too.

https://codereview.appspot.com/12034043/diff/1/names/constants.go
File names/constants.go (right):

https://codereview.appspot.com/12034043/diff/1/names/constants.go#newcode17
names/constants.go:17: ContainerSpecSnippet = "(([a-z])*:)?"
On 2013/07/30 14:38:37, dimitern wrote:
> On 2013/07/29 16:28:44, rog wrote:
> > what's the reason for the inner set of brackets here?
> > 
> > also, do we want to allow ":46" (i'm not sure what
> > a "new container" spec implies) ?
> 
> Inner brackets means 0 or more repetitions of [a-z]. I suppose it could be
> written as (\w*:)?, but the above is more restrictive wrt Unicode support in
> regular expressions perhaps.

ok, the brackets are not necessary, as i suspected.
"([a-z]*:)?" would do the job just as well
and means that i won't waste time looking around
for places we might be using the inner brackets
as a capturing group.

https://codereview.appspot.com/12034043/diff/11001/names/constants.go
File names/constants.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/constants.go#newcode18
names/constants.go:18: UnitTagPrefix    = UnitTagKind + "-"
i don't think we need to export (or even define) these *Prefix constants.

if you feel the need, then perhaps:

func mkTag(kind, rest string) string {
    return kind + "-" + rest
}

would do the job.

https://codereview.appspot.com/12034043/diff/11001/names/constants.go#newcode29
names/constants.go:29: func TagKind(tag string) (string, error) {
tbh, i'd be tempted to do:

var validKinds = map[string]bool{
    UnitTagKind: true,
    etc
}

func TagKind(tag string) (string, error) {
    i := strings.Index(tag, "-")
    if i <= 0 || !validKinds[tag[0:i]] {
         return fmt.Errorf("%q is not a valid tag", tag)
    }
    return tag[0:i]
}

which is probably more efficient, and definitely easier
to add more kinds if need be.

in fact, looking at the way the prefixes are used around
the place, perhaps this function might work well too:

func splitTag(tag string) (kind, rest string, err error) {
    kind, err = TagKind(tag)
    if err != nil {
        return "", "", err
    }
    return kind, tag[len(kind)+1:], nil
}

then you could probably avoid all uses of the prefix constants.

then the name of this file would probably be better as "tagkind.go"

https://codereview.appspot.com/12034043/diff/11001/names/environ.go
File names/environ.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/environ.go#newcode8
names/environ.go:8: return EnvironTagPrefix + name
return mkTag(EnvironTagKind, name) ?

https://codereview.appspot.com/12034043/diff/11001/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/machine.go#newcode41
names/machine.go:41: func MachineIdFromTag(tag string) (string, error) {
kind, id, err := splitTag(tag)
if kind != MachineTagKind {
    return fmt.Errorf("%q is not a valid machine tag", tag)
}
// Put the slashes back.
id = etc...

?

https://codereview.appspot.com/12034043/diff/11001/names/unit.go
File names/unit.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/unit.go#newcode22
names/unit.go:22: if !strings.HasPrefix(tag, UnitTagPrefix) {
use splitTag ?

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/deployer/d...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/deployer/d...
state/apiserver/deployer/deployer.go:118: result.Results[i] = *entityResult
why not just return StringsWatchResult by value from watchOneMachineUnits? then
you won't need the if statement.

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/machine/ma...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/machine/ma...
state/apiserver/machine/machiner.go:102: if err == nil {
similar here.

[fwereade, 2013-07-30 15:22:53]

LGTM with a few thoughts.

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go#newcode64
cmd/juju/addmachine.go:64: c.MachineId = containerSpec[sep+1:]
On 2013/07/30 15:10:11, rog wrote:
> On 2013/07/30 14:38:37, dimitern wrote:
> > On 2013/07/29 16:28:44, rog wrote:
> > > it seems to me like the above code (with its knowledge
> > > of the colon-separated syntax of a container spec)
> > > should live in names too.
> > 
> > Not really, this syntax here is specific to the command: "lxc:<id>" means
> start
> > an LXC container in machine <id>.
> 
> it looks like the names package supports this syntax (e.g.
> names.ContainerSpecSnippet) and so the parsing for it
> should probably live there too.

Don't think so. In `lxc:0`, "lxc" identifies the (pseudo?) provider, and "0" is
a provider-specific placement directive that's in here because it's kinda
convenient and we haven't formalized the notion of the lxc provider yet. We
almost certainly shouldn't pull any more related logic into this particular
package.

https://codereview.appspot.com/12034043/diff/11001/names/constants.go
File names/constants.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/constants.go#newcode29
names/constants.go:29: func TagKind(tag string) (string, error) {
Quibble quibble bikeshed: if you extract the tag kind and switch on that, it
might look a bit cleaner.

https://codereview.appspot.com/12034043/diff/11001/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/machine.go#newcode20
names/machine.go:20: // IsMachineId returns whether id is a valid machine id.
I'd be somewhat inclined to to drop the "Id"... the machine id has ended up a
machine name, really, anyway, and while we may not have the emotional energy to
fix up the whole codebase it's probably reasonable to write new code as though
it's just a name. Thoughts?

https://codereview.appspot.com/12034043/diff/11001/names/machine.go#newcode41
names/machine.go:41: func MachineIdFromTag(tag string) (string, error) {
similarly s/Id// ?

https://codereview.appspot.com/12034043/diff/11001/names/machine_test.go
File names/machine_test.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/machine_test.go#newc...
names/machine_test.go:90: {pattern: "lxc:42/kvm/56/lxc/0", valid: true},
Good to have tests that leading 0s in numbers are rejected (but 0 is ok).

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/deployer/d...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/deployer/d...
state/apiserver/deployer/deployer.go:118: result.Results[i] = *entityResult
On 2013/07/30 15:10:11, rog wrote:
> why not just return StringsWatchResult by value from watchOneMachineUnits?
then
> you won't need the if statement.

I imagine because `return nil, err` is a lot prettier than `return
StringsWatchResult{}, err`. `var nothing StringsWatchResult` at the top, and
then `return nothing, err` might be nicer?

[dimitern, 2013-07-30 16:47:50]

Please take a look.

https://codereview.appspot.com/12034043/diff/11001/names/constants.go
File names/constants.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/constants.go#newcode18
names/constants.go:18: UnitTagPrefix    = UnitTagKind + "-"
On 2013/07/30 15:10:11, rog wrote:
> i don't think we need to export (or even define) these *Prefix constants.
> 
> if you feel the need, then perhaps:
> 
> func mkTag(kind, rest string) string {
>     return kind + "-" + rest
> }
> 
> would do the job.

Good idea, but I'll call it makeTag.

https://codereview.appspot.com/12034043/diff/11001/names/constants.go#newcode29
names/constants.go:29: func TagKind(tag string) (string, error) {
On 2013/07/30 15:10:11, rog wrote:
> tbh, i'd be tempted to do:
> 
> var validKinds = map[string]bool{
>     UnitTagKind: true,
>     etc
> }
> 
> func TagKind(tag string) (string, error) {
>     i := strings.Index(tag, "-")
>     if i <= 0 || !validKinds[tag[0:i]] {
>          return fmt.Errorf("%q is not a valid tag", tag)
>     }
>     return tag[0:i]
> }
> 
> which is probably more efficient, and definitely easier
> to add more kinds if need be.
> 
> in fact, looking at the way the prefixes are used around
> the place, perhaps this function might work well too:
> 
> func splitTag(tag string) (kind, rest string, err error) {
>     kind, err = TagKind(tag)
>     if err != nil {
>         return "", "", err
>     }
>     return kind, tag[len(kind)+1:], nil
> }
> 
> then you could probably avoid all uses of the prefix constants.
> 
> then the name of this file would probably be better as "tagkind.go"

All done.

https://codereview.appspot.com/12034043/diff/11001/names/constants.go#newcode29
names/constants.go:29: func TagKind(tag string) (string, error) {
On 2013/07/30 15:22:54, fwereade wrote:
> Quibble quibble bikeshed: if you extract the tag kind and switch on that, it
> might look a bit cleaner.

Done differently.

https://codereview.appspot.com/12034043/diff/11001/names/environ.go
File names/environ.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/environ.go#newcode8
names/environ.go:8: return EnvironTagPrefix + name
On 2013/07/30 15:10:11, rog wrote:
> return mkTag(EnvironTagKind, name) ?

Done.

https://codereview.appspot.com/12034043/diff/11001/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/machine.go#newcode20
names/machine.go:20: // IsMachineId returns whether id is a valid machine id.
On 2013/07/30 15:22:54, fwereade wrote:
> I'd be somewhat inclined to to drop the "Id"... the machine id has ended up a
> machine name, really, anyway, and while we may not have the emotional energy
to
> fix up the whole codebase it's probably reasonable to write new code as though
> it's just a name. Thoughts?

Done.

https://codereview.appspot.com/12034043/diff/11001/names/machine.go#newcode41
names/machine.go:41: func MachineIdFromTag(tag string) (string, error) {
On 2013/07/30 15:22:54, fwereade wrote:
> similarly s/Id// ?

Done.

https://codereview.appspot.com/12034043/diff/11001/names/machine.go#newcode41
names/machine.go:41: func MachineIdFromTag(tag string) (string, error) {
On 2013/07/30 15:10:11, rog wrote:
> kind, id, err := splitTag(tag)
> if kind != MachineTagKind {
>     return fmt.Errorf("%q is not a valid machine tag", tag)
> }
> // Put the slashes back.
> id = etc...
> 
> ?

Done.

https://codereview.appspot.com/12034043/diff/11001/names/machine_test.go
File names/machine_test.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/machine_test.go#newc...
names/machine_test.go:90: {pattern: "lxc:42/kvm/56/lxc/0", valid: true},
On 2013/07/30 15:22:54, fwereade wrote:
> Good to have tests that leading 0s in numbers are rejected (but 0 is ok).

Done.

https://codereview.appspot.com/12034043/diff/11001/names/unit.go
File names/unit.go (right):

https://codereview.appspot.com/12034043/diff/11001/names/unit.go#newcode22
names/unit.go:22: if !strings.HasPrefix(tag, UnitTagPrefix) {
On 2013/07/30 15:10:11, rog wrote:
> use splitTag ?

Done.

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/deployer/d...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/deployer/d...
state/apiserver/deployer/deployer.go:118: result.Results[i] = *entityResult
On 2013/07/30 15:10:11, rog wrote:
> why not just return StringsWatchResult by value from watchOneMachineUnits?
then
> you won't need the if statement.

Done.

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/deployer/d...
state/apiserver/deployer/deployer.go:118: result.Results[i] = *entityResult
On 2013/07/30 15:22:54, fwereade wrote:
> On 2013/07/30 15:10:11, rog wrote:
> > why not just return StringsWatchResult by value from watchOneMachineUnits?
> then
> > you won't need the if statement.
> 
> I imagine because `return nil, err` is a lot prettier than `return
> StringsWatchResult{}, err`. `var nothing StringsWatchResult` at the top, and
> then `return nothing, err` might be nicer?

Done.

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/machine/ma...
File state/apiserver/machine/machiner.go (right):

https://codereview.appspot.com/12034043/diff/11001/state/apiserver/machine/ma...
state/apiserver/machine/machiner.go:102: if err == nil {
On 2013/07/30 15:10:11, rog wrote:
> similar here.

Done.

[rog, 2013-07-30 17:36:46]

LGTM with two final suggestions.

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go#newcode64
cmd/juju/addmachine.go:64: c.MachineId = containerSpec[sep+1:]
On 2013/07/30 15:22:54, fwereade wrote:
> On 2013/07/30 15:10:11, rog wrote:
> > On 2013/07/30 14:38:37, dimitern wrote:
> > > On 2013/07/29 16:28:44, rog wrote:
> > > > it seems to me like the above code (with its knowledge
> > > > of the colon-separated syntax of a container spec)
> > > > should live in names too.
> > > 
> > > Not really, this syntax here is specific to the command: "lxc:<id>" means
> > start
> > > an LXC container in machine <id>.
> > 
> > it looks like the names package supports this syntax (e.g.
> > names.ContainerSpecSnippet) and so the parsing for it
> > should probably live there too.
> 
> Don't think so. In `lxc:0`, "lxc" identifies the (pseudo?) provider, and "0"
is
> a provider-specific placement directive that's in here because it's kinda
> convenient and we haven't formalized the notion of the lxc provider yet. We
> almost certainly shouldn't pull any more related logic into this particular
> package.

I still think that this logic should live in names.
apart from anything else, this code looks wrong when
containerSpec is a simple machine spec.
ParseSupportedContainerType might ensure
that it can't be, but it looks superficially as if we're calling
IsMachineOrNewContainer
and that we're using the fact that's returned true to
deduce that the containerSpec contains a ":".

just:

func SplitContainerSpec(containerSpec string) (containerType string, err error)

in names might do the job.

https://codereview.appspot.com/12034043/diff/24001/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12034043/diff/24001/names/machine.go#newcode12
names/machine.go:12: ContainerSpecSnippet = "(([a-z])+:)?"
please remove the unnecessary brackets - they only serve to confuse.

[dimitern, 2013-07-30 18:28:23]

Please take a look.

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/12034043/diff/1/cmd/juju/addmachine.go#newcode64
cmd/juju/addmachine.go:64: c.MachineId = containerSpec[sep+1:]
On 2013/07/30 17:36:47, rog wrote:
> On 2013/07/30 15:22:54, fwereade wrote:
> > On 2013/07/30 15:10:11, rog wrote:
> > > On 2013/07/30 14:38:37, dimitern wrote:
> > > > On 2013/07/29 16:28:44, rog wrote:
> > > > > it seems to me like the above code (with its knowledge
> > > > > of the colon-separated syntax of a container spec)
> > > > > should live in names too.
> > > > 
> > > > Not really, this syntax here is specific to the command: "lxc:<id>"
means
> > > start
> > > > an LXC container in machine <id>.
> > > 
> > > it looks like the names package supports this syntax (e.g.
> > > names.ContainerSpecSnippet) and so the parsing for it
> > > should probably live there too.
> > 
> > Don't think so. In `lxc:0`, "lxc" identifies the (pseudo?) provider, and "0"
> is
> > a provider-specific placement directive that's in here because it's kinda
> > convenient and we haven't formalized the notion of the lxc provider yet. We
> > almost certainly shouldn't pull any more related logic into this particular
> > package.
> 
> I still think that this logic should live in names.
> apart from anything else, this code looks wrong when
> containerSpec is a simple machine spec.
> ParseSupportedContainerType might ensure
> that it can't be, but it looks superficially as if we're calling
> IsMachineOrNewContainer
> and that we're using the fact that's returned true to
> deduce that the containerSpec contains a ":".
> 
> just:
> 
> func SplitContainerSpec(containerSpec string) (containerType string, err
error)
> 
> in names might do the job.

If ":" is not in containerSpec, then c.MachineId will be set to containerSpec
(just a machine id in this case), while before calling
ParseSupportedContainerType the containerSpec[:sep] will panic. There should be
tests for that, but I'm not comfortable mixing this logic in this CL, at least
it deserves a discussion first.

https://codereview.appspot.com/12034043/diff/24001/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12034043/diff/24001/names/machine.go#newcode12
names/machine.go:12: ContainerSpecSnippet = "(([a-z])+:)?"
On 2013/07/30 17:36:47, rog wrote:
> please remove the unnecessary brackets - they only serve to confuse.

Done.