names;api/uniter: UnitService; minor other fixes

names;api/uniter: UnitService; minor other fixes

Adds names.UnitService() call that converts a
unit name to its service name.
Removed UniterAPI.GetService. After a discussion
it was decided we don't need it: UniterAPI.Life
supports service tags, and with the new names call
we can convert the unit tag to a service name
without the need for an API call. uniter.Unit's
ServiceName() and Service() methods changed as
agreed.

https://code.launchpad.net/~dimitern/juju-core/109-names-servicefromunittag/+merge/181016

(do not edit description out of merge proposal)

[dimitern, 2013-08-20 10:53:25]

Please take a look.

[dimitern, 2013-08-20 10:55:38]

Please take a look.

[fwereade, 2013-08-20 11:03:07]

Couple of thoughts.

https://codereview.appspot.com/13112045/diff/4001/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13112045/diff/4001/state/api/uniter/unit.go#ne...
state/api/uniter/unit.go:101: return service, service.Refresh()
I'd prefer a nil return on error.

https://codereview.appspot.com/13112045/diff/4001/state/api/uniter/unit.go#ne...
state/api/uniter/unit.go:115: return names.ServiceFromUnitTag(u.tag)
Might it make more sense to just convert tags directly? Going via name seems a
bit roundabout.

[rog, 2013-08-20 11:06:56]

LGTM with some thoughts about ServiceFromUnitTag.

https://codereview.appspot.com/13112045/diff/4001/names/service.go
File names/service.go (right):

https://codereview.appspot.com/13112045/diff/4001/names/service.go#newcode30
names/service.go:30: func ServiceFromUnitTag(tag string) string {
I think I might put this in unit.go and call it "UnitService". Something like
this, perhaps:

// UnitService returns the name of the service
// that the unit is associated with.
// It panics if unitName is not a valid unit name.
func UnitService(unitName string) string {
    s := validUnit.MatchStringSubmatch(unitName)
    if s == nil {
       panic("invalid unit name")
    }
    return s[1]
}

(you'd have to add a capturing group to ServiceSnippet)

Then all the tag parsing stuff is left in one place.
I think this is just as general (you can call ParseTag before calling
UnitService)

https://codereview.appspot.com/13112045/diff/4001/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13112045/diff/4001/state/api/uniter/unit.go#ne...
state/api/uniter/unit.go:101: return service, service.Refresh()
On 2013/08/20 11:03:07, fwereade wrote:
> I'd prefer a nil return on error.

+1

https://codereview.appspot.com/13112045/diff/4001/state/api/uniter/unit.go#ne...
state/api/uniter/unit.go:115: return names.ServiceFromUnitTag(u.tag)
On 2013/08/20 11:03:07, fwereade wrote:
> Might it make more sense to just convert tags directly? Going via name seems a
> bit roundabout.

It goes via the name internally, so I've made a slightly different suggestion.

[rog, 2013-08-20 11:09:30]

https://codereview.appspot.com/13112045/diff/4001/names/service.go
File names/service.go (right):

https://codereview.appspot.com/13112045/diff/4001/names/service.go#newcode30
names/service.go:30: func ServiceFromUnitTag(tag string) string {
>     s := validUnit.MatchStringSubmatch(unitName)

FindStringSubmatch, of course.

[dimitern, 2013-08-20 11:23:40]

Please take a look.

https://codereview.appspot.com/13112045/diff/4001/names/service.go
File names/service.go (right):

https://codereview.appspot.com/13112045/diff/4001/names/service.go#newcode30
names/service.go:30: func ServiceFromUnitTag(tag string) string {
On 2013/08/20 11:06:56, rog wrote:
> I think I might put this in unit.go and call it "UnitService". Something like
> this, perhaps:
> 
> // UnitService returns the name of the service
> // that the unit is associated with.
> // It panics if unitName is not a valid unit name.
> func UnitService(unitName string) string {
>     s := validUnit.MatchStringSubmatch(unitName)
>     if s == nil {
>        panic("invalid unit name")
>     }
>     return s[1]
> }
> 
> (you'd have to add a capturing group to ServiceSnippet)
> 
> Then all the tag parsing stuff is left in one place.
> I think this is just as general (you can call ParseTag before calling
> UnitService)

Done.

https://codereview.appspot.com/13112045/diff/4001/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13112045/diff/4001/state/api/uniter/unit.go#ne...
state/api/uniter/unit.go:101: return service, service.Refresh()
On 2013/08/20 11:03:07, fwereade wrote:
> I'd prefer a nil return on error.

Oh, ofc, silly me! Done.

https://codereview.appspot.com/13112045/diff/4001/state/api/uniter/unit.go#ne...
state/api/uniter/unit.go:115: return names.ServiceFromUnitTag(u.tag)
On 2013/08/20 11:03:07, fwereade wrote:
> Might it make more sense to just convert tags directly? Going via name seems a
> bit roundabout.

As agreed online, went with rog's suggestion.

[rog, 2013-08-20 11:30:05]

LGTM with a couple of thoughts below.

https://codereview.appspot.com/13112045/diff/12001/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13112045/diff/12001/state/api/uniter/unit.go#n...
state/api/uniter/unit.go:119: _, unitName, err := names.ParseTag(u.tag,
names.UnitTagKind)
maybe implement Unit.Name and use that?

https://codereview.appspot.com/13112045/diff/12001/state/api/uniter/unit.go#n...
state/api/uniter/unit.go:121: // TODO: panic?
definitely panic-worthy, yes.

[dimitern, 2013-08-20 11:34:34]

Please take a look.

https://codereview.appspot.com/13112045/diff/12001/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13112045/diff/12001/state/api/uniter/unit.go#n...
state/api/uniter/unit.go:119: _, unitName, err := names.ParseTag(u.tag,
names.UnitTagKind)
On 2013/08/20 11:30:06, rog wrote:
> maybe implement Unit.Name and use that?

Done.

https://codereview.appspot.com/13112045/diff/12001/state/api/uniter/unit.go#n...
state/api/uniter/unit.go:121: // TODO: panic?
On 2013/08/20 11:30:06, rog wrote:
> definitely panic-worthy, yes.

Done.

[dimitern, 2013-08-20 11:36:32]

Please take a look.