names: Fix lp:1206628 (unit-name-to-tag issues)

names: Fix lp:1206628 (unit-name-to-tag issues)

This fixes the conversions from a unit name to
a tag and vice versa, which caused incorrect
behavior with services with dashes in the name,
like "rabbitmq-server".

https://code.launchpad.net/~dimitern/juju-core/082-bug-1206628-unit-name-from-tag/+merge/177850

(do not edit description out of merge proposal)

[dimitern, 2013-07-31 14:05:30]

Please take a look.

[fwereade, 2013-07-31 14:10:49]

LGTM

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

https://codereview.appspot.com/12165043/diff/1/names/unit_test.go#newcode67
names/unit_test.go:67: c.Logf("%d. %q", i, test.pattern)
The "test %d: %s" spelling is pretty common, and my eye at least is used to
picking it out of noisy output

https://codereview.appspot.com/12165043/diff/1/names/unit_test.go#newcode70
names/unit_test.go:70: return names.UnitTag(test.pattern)
testUnitTag := func() { names.UnitTag(pattern) }

[rog, 2013-07-31 14:18:14]

LGTM with some minor remarks.

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

https://codereview.appspot.com/12165043/diff/1/names/unit.go#newcode17
names/unit.go:17: // Replace only the last "/" with "-".
i think this was ok as it was before - if we've got a slash in a service name,
then we're already screwed (and we *really* don't want slashes creeping into
tags).

perhaps just

   if !IsUnit(unitName) { 
       panic(...not valid name)
   }
   return makeTag(UnitTagKind, strings.Replace(unitName, "/", "-", -1))

https://codereview.appspot.com/12165043/diff/1/names/unit.go#newcode33
names/unit.go:33: // Replace only the last "-" with "/".
s/./, as it is valid for service names to contain hyphens./
?

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

https://codereview.appspot.com/12165043/diff/1/names/unit_test.go#newcode70
names/unit_test.go:70: return names.UnitTag(test.pattern)
On 2013/07/31 14:10:49, fwereade wrote:
> testUnitTag := func() { names.UnitTag(pattern) }

ahem
testUnitTag := func() string { return names.UnitTag(pattern) }

tbh, i don't mind whether it's all on one line or not.

[dimitern, 2013-07-31 14:22:59]

Please take a look.

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

https://codereview.appspot.com/12165043/diff/1/names/unit.go#newcode17
names/unit.go:17: // Replace only the last "/" with "-".
On 2013/07/31 14:18:14, rog wrote:
> i think this was ok as it was before - if we've got a slash in a service name,
> then we're already screwed (and we *really* don't want slashes creeping into
> tags).
> 
> perhaps just
> 
>    if !IsUnit(unitName) { 
>        panic(...not valid name)
>    }
>    return makeTag(UnitTagKind, strings.Replace(unitName, "/", "-", -1))

The code assumed too much, so I decided to fix both places.

https://codereview.appspot.com/12165043/diff/1/names/unit.go#newcode33
names/unit.go:33: // Replace only the last "-" with "/".
On 2013/07/31 14:18:14, rog wrote:
> s/./, as it is valid for service names to contain hyphens./
> ?

Done.

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

https://codereview.appspot.com/12165043/diff/1/names/unit_test.go#newcode67
names/unit_test.go:67: c.Logf("%d. %q", i, test.pattern)
On 2013/07/31 14:10:49, fwereade wrote:
> The "test %d: %s" spelling is pretty common, and my eye at least is used to
> picking it out of noisy output

Done. Will also change other similar c.Logf calls in the same package.

https://codereview.appspot.com/12165043/diff/1/names/unit_test.go#newcode70
names/unit_test.go:70: return names.UnitTag(test.pattern)
On 2013/07/31 14:10:49, fwereade wrote:
> testUnitTag := func() { names.UnitTag(pattern) }

Done.