Added owner arg to AddService()

Added owner arg to AddService()

Added owner arg to AddService() and added AddTestingService() helper

https://code.launchpad.net/~mattyw/juju-core/add-owner-to-service/+merge/191192

Requires: https://code.launchpad.net/~mattyw/juju-core/add-ownertag-to-service/+merge/189123

(do not edit description out of merge proposal)

[mattyw, 2013-11-04 11:30:15]

Please take a look.

[fwereade, 2013-11-06 10:11:34]

The diff is very dirty, and I'm not sure this is quite finished?

https://codereview.appspot.com/14699043/diff/3001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14699043/diff/3001/state/state.go#newcode705
state/state.go:705: func (st *State) AddService(name, owner string, ch *Charm)
(service *Service, err error) {
I suspect we should be doing something with the owner param ;p.

[mattyw, 2013-11-06 10:26:25]

On 2013/11/06 10:11:34, fwereade wrote:
> The diff is very dirty, and I'm not sure this is quite finished?
> 
> https://codereview.appspot.com/14699043/diff/3001/state/state.go
> File state/state.go (right):
> 
> https://codereview.appspot.com/14699043/diff/3001/state/state.go#newcode705
> state/state.go:705: func (st *State) AddService(name, owner string, ch *Charm)
> (service *Service, err error) {
> I suspect we should be doing something with the owner param ;p.

Sorry, thought I'd mentioned in irc that I don't think it's done, but thought a
review would help start a discussion about where to go next. What do you mean by
dirty?

Thanks

Matt

[mattyw, 2013-11-29 12:53:03]

Please take a look.

[fwereade, 2013-12-02 09:21:39]

Thanks for the ridiculously huge diff

https://codereview.appspot.com/14699043/diff/43001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/14699043/diff/43001/juju/conn.go#newcode237
juju/conn.go:237: service, err := conn.State.AddService(args.ServiceName,
"admin-owner", args.Charm)
"admin-owner" is not a tag. "user-admin" would be, but this makes me nervous
that there's no actual check for the sanity of the supplied value.

https://codereview.appspot.com/14699043/diff/43001/state/apiserver/client/api...
File state/apiserver/client/api_test.go (right):

https://codereview.appspot.com/14699043/diff/43001/state/apiserver/client/api...
state/apiserver/client/api_test.go:223: 
may as well lose those blank lines

https://codereview.appspot.com/14699043/diff/43001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14699043/diff/43001/state/state.go#newcode507
state/state.go:507: OwnerTag:      owner,
Yeah, needs sanity checks please -- just a docExists on the appropriate user doc
should be fine.

[fwereade, 2013-12-02 09:27:28]

sorry, didn't finish typing.

So, thanks for the massive diff -- and *if* you promise to follow up with
user-doc checks *imminently* I'm fine landing this as-is so you don't end up
having to manage it diverging again. But if you do that, please make the check
your top priority. So, LGTM, but very conditional.

(and remind me -- you did do explicit state tests for docs without ownertag
fields in a previous branch, didn't you?)

[mattyw, 2013-12-03 14:22:00]

Please take a look.

[mattyw, 2013-12-03 14:51:01]

Please take a look.

[fwereade, 2013-12-04 09:11:51]

Sorry, just a couple more. Sorry this one's been going so long.

https://codereview.appspot.com/14699043/diff/83001/state/state_test.go
File state/state_test.go (left):

https://codereview.appspot.com/14699043/diff/83001/state/state_test.go#oldcod...
state/state_test.go:770: }
Needs a couple of tests for crazy tags -- eg "user-doesnotexist", "machine-3",
"lolnotatagatall"

https://codereview.appspot.com/14699043/diff/83001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/14699043/diff/83001/state/user.go#newcode160
state/user.go:160: }
There's a top-level "names" package where this should go -- check there for
what's conventional.

[mattyw, 2013-12-04 11:57:37]

Please take a look.

[fwereade, 2013-12-04 14:45:36]

Code looks good, couldn't see tests for bad/missing owners.

https://codereview.appspot.com/14699043/diff/103001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14699043/diff/103001/state/state.go#newcode510
state/state.go:510: if err != nil || kind != names.UserTagKind {
FWIW, if you specify kind, I'm pretty sure the additional check is redundant.
But it can't hurt.

[mattyw, 2013-12-04 18:34:20]

Please take a look.

[fwereade, 2013-12-05 16:02:48]

https://codereview.appspot.com/14699043/diff/123001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14699043/diff/123001/state/state.go#newcode541
state/state.go:541: return nil, fmt.Errorf("adding service failed %v", err)
There's no longer just one possible reason for the addition to fail, but
txn.ErrAborted doesn't tell us exactly what happened, and we shouldn't really
expose that to users. It's reasonable to check for either existing service, or
missing user, and assume that if the one we checked doesn't hold the problem
must be the other one.

FWIW, there's an ErrorContextf at the top of the method, so as is this'd return
"cannot add service: adding service failed <unhelpful-internal-complaint>".

[mattyw, 2013-12-11 16:08:07]

Please take a look.

[fwereade, 2013-12-12 08:58:04]

I feel a little bit more guilty every time I do this -- but what do you think of
the below? I'm around to chat if necessary.

https://codereview.appspot.com/14699043/diff/143001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14699043/diff/143001/state/state.go#newcode553
state/state.go:553: return nil, fmt.Errorf("service already exists")
return nil, ueErr?

The whole thing might be better expressed as:

if userExists, err := st.checkUserExists(ownerId); err != nil {
    return nil, err
} else if !userExists {
    return nil, fmt.Errorf("unknown user %q", ownerId)
}
return nil, fmt.Errorf("service already exists")

https://codereview.appspot.com/14699043/diff/143001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/14699043/diff/143001/state/user.go#newcode31
state/user.go:31: return false, nil
Sorry, I didn't spot this before -- the txn mechanism is a big ol' hammer to use
in this situation, and doesn't actually give us any consistency benefits here
anyway -- you can just do a straight query without all that writing and
blocking.

func (st *State) checkUserExists(name string) (bool, error) {
    if count, err := st.users.FindId(name).Count(); err != nil {
    return false, err
} else if count == 0 {
    return false, nil
}
return true, nil

https://codereview.appspot.com/14699043/diff/143001/state/user_test.go
File state/user_test.go (right):

https://codereview.appspot.com/14699043/diff/143001/state/user_test.go#newcode57
state/user_test.go:57: c.Assert(e, gc.Equals, false)
This test should be fine as is though.

[mattyw, 2013-12-12 15:24:13]

Please take a look.

[fwereade, 2013-12-12 15:27:00]

LGTM, thanks.

[mattyw, 2013-12-12 18:02:07]

Please take a look.