Starting working on getting identities into juju.

Starting working on getting identities into juju.



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

(do not edit description out of merge proposal)

[fwereade, 2013-10-04 10:16:41]

Looks great, a few comments here and there

https://codereview.appspot.com/14389043/diff/1/cmd/juju/helptool_test.go
File cmd/juju/helptool_test.go (left):

https://codereview.appspot.com/14389043/diff/1/cmd/juju/helptool_test.go#oldc...
cmd/juju/helptool_test.go:43: "relation-list",
a drive-by that got this info from jujuc.CommandNames would be much appreciated
:)

https://codereview.appspot.com/14389043/diff/1/state/api/params/params_test.go
File state/api/params/params_test.go (right):

https://codereview.appspot.com/14389043/diff/1/state/api/params/params_test.g...
state/api/params/params_test.go:61: json: `["service","change",{"CharmURL":
"cs:quantal/name","Name":"Benji","Exposed":true,"Life":"dying","OwnerTag":"","MinUnits":42,"Constraints":{"arch":"arm",
"mem": 1024},"Config": {"hello":"goodbye","foo":false}}]`,
not sure about this -- should it be empty? all the other values look at least
somewhat real

https://codereview.appspot.com/14389043/diff/1/state/api/uniter/uniter.go
File state/api/uniter/uniter.go (right):

https://codereview.appspot.com/14389043/diff/1/state/api/uniter/uniter.go#new...
state/api/uniter/uniter.go:186: return "foo", nil
I'm a bit baffled by this. Dead code?

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

https://codereview.appspot.com/14389043/diff/1/state/state.go#newcode693
state/state.go:693: func (st *State) GetServiceOwner(name string) string {
I'm not sure this delivers much value really -- I think I only spotted one place
we'd need it?

https://codereview.appspot.com/14389043/diff/1/worker/uniter/context.go
File worker/uniter/context.go (right):

https://codereview.appspot.com/14389043/diff/1/worker/uniter/context.go#newco...
worker/uniter/context.go:145: func (ctx *HookContext) OwnerTag() (string, bool)
{ //TODO Return error
I'd kinda rather get that information earlier and just construct the HookContext
with the correct value to begin with -- we do currently try to guarantee that
we'll show a consistent view of the world inside any given hook, and while
owners can't change today I'd prefer to be ready for them to do so in future.

https://codereview.appspot.com/14389043/diff/1/worker/uniter/jujuc/context.go
File worker/uniter/jujuc/context.go (right):

https://codereview.appspot.com/14389043/diff/1/worker/uniter/jujuc/context.go...
worker/uniter/jujuc/context.go:57: OwnerTag() (string, bool)
If you do as suggested this can just return string.

And, since it should always be valid anyway, might be nice to define it up next
to UnitName instead of after all these conditional or errory ones.

https://codereview.appspot.com/14389043/diff/1/worker/uniter/jujuc/owner-get.go
File worker/uniter/jujuc/owner-get.go (right):

https://codereview.appspot.com/14389043/diff/1/worker/uniter/jujuc/owner-get....
worker/uniter/jujuc/owner-get.go:31: Purpose: "print the owner of the service",
get information about the owner of the service?

https://codereview.appspot.com/14389043/diff/1/worker/uniter/jujuc/owner-get....
worker/uniter/jujuc/owner-get.go:50: func (c *OwnerGetCommand) Run(ctx
*cmd.Context) error {
if c.Key != "tag" {
    return some error
}
return c.out.write(ctx, c.ctx.OwnerTag())

?

[mattyw, 2013-10-07 08:49:27]

Please take a look.

[fwereade, 2013-10-07 09:48:40]

Nearly there, just a couple more thoughts.

https://codereview.appspot.com/14389043/diff/9001/cmd/juju/helptool_test.go
File cmd/juju/helptool_test.go (right):

https://codereview.appspot.com/14389043/diff/9001/cmd/juju/helptool_test.go#n...
cmd/juju/helptool_test.go:37: expectedNames := jujuc.CommandNames()
<3

https://codereview.appspot.com/14389043/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14389043/diff/9001/state/state.go#newcode693
state/state.go:693: func (st *State) GetServiceOwner(name string) string {
GetServiceOwnerTag</pedant>

https://codereview.appspot.com/14389043/diff/9001/state/state.go#newcode697
state/state.go:697: return ""
Hmmmm. I really don't like swallowing errors like this. What led us to this
solution?

https://codereview.appspot.com/14389043/diff/9001/worker/uniter/context.go
File worker/uniter/context.go (right):

https://codereview.appspot.com/14389043/diff/9001/worker/uniter/context.go#ne...
worker/uniter/context.go:69: apiAddrs []string, serviceOwner string)
(*HookContext, error) {
I'm starting to rather dislike this parameter list -- maybe stick it all in a
struct? (sorry, should have been done long before we hit 7 args ;))

[mattyw, 2013-10-10 09:42:21]

Please take a look.

[fwereade, 2013-10-10 09:50:20]

Important thought I completely missed before... sorry.

https://codereview.appspot.com/14389043/diff/16001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/14389043/diff/16001/state/megawatcher.go#newco...
state/megawatcher.go:128: }
Sorry, this didn't click at all. Since backingService is just a serviceDoc, it's
probably easiest to just use the serviceDoc directly -- we have all the
information we need readily available, and should just put the method onto
serviceDoc and call through directly both here and in the Service method. Then
there's no need for the state method, and no possibility of error either.

[mattyw, 2013-10-28 17:36:04]

Please take a look.

[fwereade, 2013-11-05 15:34:31]

I think at least one of us lost context while we were busy. Thoughts?

https://codereview.appspot.com/14389043/diff/24001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/14389043/diff/24001/state/service.go#newcode609
state/service.go:609: return s.doc.OwnerTag
Sorry, I'm missing context -- but didn't we say this should be returning
user-admin if empty?

https://codereview.appspot.com/14389043/diff/24001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14389043/diff/24001/state/state.go#newcode624
state/state.go:624: OwnerTag:      "user-admin",
I don't think this is right. Old documents will be returning "", and the world
will become confused. I think the right place to make the substitution is in
GetOwnerTag -- or rather the serviceDoc method I seem to recall we discussed?

https://codereview.appspot.com/14389043/diff/24001/worker/uniter/jujuc/owner-...
File worker/uniter/jujuc/owner-get.go (right):

https://codereview.appspot.com/14389043/diff/24001/worker/uniter/jujuc/owner-...
worker/uniter/jujuc/owner-get.go:31: Purpose: "print information about the owner
of the service",
We should include *some* documentation indicating that "tag" is the only
reasonable param at the moment

[mattyw, 2013-11-12 17:30:28]

Please take a look.

[fwereade, 2013-11-13 13:12:37]

Nearly there -- apart from the below, the big thing we're missing is a test that
explicitly clears the ownertag field in mongo and checks for sanity. (fwiw it's
currently always empty, because ownerTag is not exported -- but if you fixed
that, handling for the empty case would be untested.)

https://codereview.appspot.com/14389043/diff/44001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/14389043/diff/44001/state/service.go#newcode46
state/service.go:46: ownerTag      string
This needs to be exported, or it won't get serialized. And I'd quite like to
keep TxnRevno as the last field defined -- it's different to the others in a way
I can't quite characterise cleanly at the moment.

https://codereview.appspot.com/14389043/diff/44001/state/service.go#newcode610
state/service.go:610: if tag == "" {
Stick a SCHEMACHANGE comment in here -- we're using that to tag areas like this
that we'd like to fix when we can.

https://codereview.appspot.com/14389043/diff/44001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/14389043/diff/44001/state/state.go#newcode624
state/state.go:624: ownerTag:      "user-admin",
Since we've got the ""-handling in place everywhere, maybe it's best to skip
this until we have the ability to set it on AddService?

https://codereview.appspot.com/14389043/diff/44001/worker/uniter/jujuc/owner-...
File worker/uniter/jujuc/owner-get.go (right):

https://codereview.appspot.com/14389043/diff/44001/worker/uniter/jujuc/owner-...
worker/uniter/jujuc/owner-get.go:30: Args:    "<setting>",
Doc: `The only valid value for <setting> is currently "tag"`,

[mattyw, 2013-11-15 12:53:46]

Please take a look.

[mattyw, 2013-11-15 13:45:06]

Please take a look.

[william.reade, 2013-11-18 10:43:44]

Latest changes LGTM, thanks for bearing with me.