Replace EntityName with Tag

Replace EntityName with Tag

This branch refactors EntityName to Tag per William and Roger in order to aid in clarity.  Additionally, it adds a Tagger interface in state so that state.Authenticator() can return a TaggedAuthenticator and state.Annotator() can return a Tagged Annotator.  This was used to help clean up tests as well, as many were defining namedEntity interfaces in order to expose EntityName in tests.  Additionally, to be consistent, this branch changes (in a separate commit) the EntityName field in the state Info and api Info objects to Tag.  API params for login and annotations were also changed away from EntityName, and docs were updated accordingly.  There will be a subsequent GUI branch to accomodate for this if/after it lands.

https://code.launchpad.net/~makyo/juju-core/tagger-1156715/+merge/155310

(do not edit description out of merge proposal)

[matthew.scott, 2013-03-25 18:01:30]

Please take a look.

[fwereade, 2013-03-26 09:03:32]

A few suggestions for clarity's sake, but I think dropping EntityName is enough
of a win that this is a definite improvement regardless. LGTM with the below
changes (or with a good and recorded reason to skip them).

The only thing I'm not quite sure about is the use of Tag in an auth context --
might it be helpful to name it AuthTag in those places?

https://codereview.appspot.com/8018043/diff/2001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/8018043/diff/2001/cmd/jujud/deploy_test.go#new...
cmd/jujud/deploy_test.go:88: c.Check(deployerName, Equals, expectInfo.Tag)
It would be really helpful to fix instances of deployerName to be deployerTag.

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go
File environs/agent/agent.go (left):

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go#oldc...
environs/agent/agent.go:111: return requiredError("state entity name")
s/name/tag/

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go#oldc...
environs/agent/agent.go:123: return requiredError("API entity name")
Ditto.

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go#oldc...
environs/agent/agent.go:133: return fmt.Errorf("mismatched entity names")
Ditto.

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:87: // Tag returns the entity name that will be used to
connect to
"the tag of the entity on whose behalf the state connection will be made", or
something?

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (left):

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:433: return fmt.Errorf("entity name must be
blank when starting a state server")
s/name/tag/

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:436: return fmt.Errorf("entity name must be
blank when starting a state server")
Ditto.

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:455: return fmt.Errorf("entity name must match
started machine")
Ditto.

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:461: return fmt.Errorf("entity name must match
started machine")
Ditto.

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit_test.go:107: echo 'datadir:
/var/lib/juju\\nstateservercert:\\n[^']+stateserverkey:\\n[^']+mongoport:
37017\\napiport: 17070\\noldpassword: arble\\nstateinfo:\\n  addrs:\\n  -
localhost:37017\\n  cacert:\\n[^']+  tag: machine-0\\n  password:
""\\noldapipassword: ""\\napiinfo:\\n  addrs:\\n  - localhost:17070\\n 
cacert:\\n[^']+  tag: machine-0\\n  password: ""\\n' >
'/var/lib/juju/agents/machine-0/agent\.conf'
I *know* that this is a trivial rename -- but since we're changing the cloudinit
file, please verify that you can still bootstrap and deploy with the changes.

https://codereview.appspot.com/8018043/diff/2001/environs/dummy/environs.go
File environs/dummy/environs.go (left):

https://codereview.appspot.com/8018043/diff/2001/environs/dummy/environs.go#o...
environs/dummy/environs.go:557: return nil, fmt.Errorf("entity name must match
started machine")
s/name/tag/

https://codereview.appspot.com/8018043/diff/2001/environs/dummy/environs.go#o...
environs/dummy/environs.go:560: return nil, fmt.Errorf("entity name must match
started machine")
Ditto.

https://codereview.appspot.com/8018043/diff/2001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/8018043/diff/2001/state/apiserver/apiserver.go...
state/apiserver/apiserver.go:495: ename := u.root.user.authenticator().Tag()
s/name/tag/

?

https://codereview.appspot.com/8018043/diff/2001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/8018043/diff/2001/state/state_test.go#newcode1440
state/state_test.go:1440: return e.(state.Tagger), nil
Do you need this cast?

https://codereview.appspot.com/8018043/diff/2001/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/8018043/diff/2001/worker/deployer/simple.go#ne...
worker/deployer/simple.go:159: svcName := "jujud-" + ctx.deployerName + ":" +
tag
deployerTag throughout this package would be very helpful.

[matthew.scott, 2013-03-26 20:12:44]

Thanks for the review.  Changes noted below for comments.  I switched to AuthTag
for API authentication, but wasn't sure if there were additional places that
should be done.  As noted below, I tested bootstrapping, deploying, and API
calls after these changes.

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go
File environs/agent/agent.go (left):

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go#oldc...
environs/agent/agent.go:111: return requiredError("state entity name")
On 2013/03/26 09:03:32, fwereade wrote:
> s/name/tag/

Done.

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go#oldc...
environs/agent/agent.go:123: return requiredError("API entity name")
On 2013/03/26 09:03:32, fwereade wrote:
> Ditto.

Done.

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go#oldc...
environs/agent/agent.go:133: return fmt.Errorf("mismatched entity names")
On 2013/03/26 09:03:32, fwereade wrote:
> Ditto.

Done.

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go
File environs/agent/agent.go (right):

https://codereview.appspot.com/8018043/diff/2001/environs/agent/agent.go#newc...
environs/agent/agent.go:87: // Tag returns the entity name that will be used to
connect to
On 2013/03/26 09:03:32, fwereade wrote:
> "the tag of the entity on whose behalf the state connection will be made", or
> something?

Done.

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (left):

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:433: return fmt.Errorf("entity name must be
blank when starting a state server")
On 2013/03/26 09:03:32, fwereade wrote:
> s/name/tag/

Done.

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:436: return fmt.Errorf("entity name must be
blank when starting a state server")
On 2013/03/26 09:03:32, fwereade wrote:
> Ditto.

Done.

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:455: return fmt.Errorf("entity name must match
started machine")
On 2013/03/26 09:03:32, fwereade wrote:
> Ditto.

Done.

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:461: return fmt.Errorf("entity name must match
started machine")
On 2013/03/26 09:03:32, fwereade wrote:
> Ditto.

Done.

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/8018043/diff/2001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit_test.go:107: echo 'datadir:
/var/lib/juju\\nstateservercert:\\n[^']+stateserverkey:\\n[^']+mongoport:
37017\\napiport: 17070\\noldpassword: arble\\nstateinfo:\\n  addrs:\\n  -
localhost:37017\\n  cacert:\\n[^']+  tag: machine-0\\n  password:
""\\noldapipassword: ""\\napiinfo:\\n  addrs:\\n  - localhost:17070\\n 
cacert:\\n[^']+  tag: machine-0\\n  password: ""\\n' >
'/var/lib/juju/agents/machine-0/agent\.conf'
On 2013/03/26 09:03:32, fwereade wrote:
> I *know* that this is a trivial rename -- but since we're changing the
cloudinit
> file, please verify that you can still bootstrap and deploy with the changes.

Confirmed - I bootstrapped and deployed a mysql instance in order to test
annotations and logging in via websocket.

https://codereview.appspot.com/8018043/diff/2001/environs/dummy/environs.go
File environs/dummy/environs.go (left):

https://codereview.appspot.com/8018043/diff/2001/environs/dummy/environs.go#o...
environs/dummy/environs.go:557: return nil, fmt.Errorf("entity name must match
started machine")
On 2013/03/26 09:03:32, fwereade wrote:
> s/name/tag/
> 

Done.

https://codereview.appspot.com/8018043/diff/2001/environs/dummy/environs.go#o...
environs/dummy/environs.go:560: return nil, fmt.Errorf("entity name must match
started machine")
On 2013/03/26 09:03:32, fwereade wrote:
> Ditto.

Done.

https://codereview.appspot.com/8018043/diff/2001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/8018043/diff/2001/state/state_test.go#newcode1440
state/state_test.go:1440: return e.(state.Tagger), nil
On 2013/03/26 09:03:32, fwereade wrote:
> Do you need this cast?

Not anymore.  Done.

https://codereview.appspot.com/8018043/diff/2001/worker/deployer/simple.go
File worker/deployer/simple.go (right):

https://codereview.appspot.com/8018043/diff/2001/worker/deployer/simple.go#ne...
worker/deployer/simple.go:159: svcName := "jujud-" + ctx.deployerName + ":" +
tag
On 2013/03/26 09:03:32, fwereade wrote:
> deployerTag throughout this package would be very helpful.

Done.

[matthew.scott, 2013-03-26 20:17:55]

Please take a look.

[jameinel, 2013-03-28 16:20:01]

LGTM.

Some small questions, but nothing that I think needs to be done. I don't
personally know what the thing should be called, but this patch is clearly
mechanical and doing the appropriate rename.

https://codereview.appspot.com/8018043/diff/12001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/8018043/diff/12001/state/api/apiclient.go#newc...
state/api/apiclient.go:255: args := params.SetAnnotations{tag, pairs}
Most places are renaming entityName to tag. this place was entityId, is that
correct?

https://codereview.appspot.com/8018043/diff/12001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/8018043/diff/12001/state/api/params/params.go#...
state/api/params/params.go:94: AuthTag  string
Here is also the only place I've seen AuthTag, separate from Tag, and it used to
be EntityName just like the others.

I can see where it could be correct, I just want to verify, because I don't have
much context.

https://codereview.appspot.com/8018043/diff/12001/state/apiserver/api_test.go
File state/apiserver/api_test.go (left):

https://codereview.appspot.com/8018043/diff/12001/state/apiserver/api_test.go...
state/apiserver/api_test.go:605: }
The old api had Refresh(), we don't actually need that one? clearly the old docs
were wrong anyway, since there wasn't PasswordValid before either.

https://codereview.appspot.com/8018043/diff/12001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/8018043/diff/12001/state/state.go#newcode286
state/state.go:286: }
I'm curious on the reason to make these public interfaces. Mostly in that they
used to be private, was there a reason for that? This seems to help by avoiding
extra definitions (you have them when you need them).
So I think it is better, but I'm not sure if there was a reason they were
private before.

[matthew.scott, 2013-03-28 16:35:23]

Thanks again for the review.  No real changes to make, other than potentially
updating some comments to clarify. Responses below!

https://codereview.appspot.com/8018043/diff/12001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/8018043/diff/12001/state/api/apiclient.go#newc...
state/api/apiclient.go:255: args := params.SetAnnotations{tag, pairs}
On 2013/03/28 16:20:01, jameinel wrote:
> Most places are renaming entityName to tag. this place was entityId, is that
> correct?

Correct.  entityId was the parameter being sent before, but referred to the
EntityName field of an annotatable object.  The mis-match was fixed with the
refactor.

https://codereview.appspot.com/8018043/diff/12001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/8018043/diff/12001/state/api/params/params.go#...
state/api/params/params.go:94: AuthTag  string
On 2013/03/28 16:20:01, jameinel wrote:
> Here is also the only place I've seen AuthTag, separate from Tag, and it used
to
> be EntityName just like the others.
> 
> I can see where it could be correct, I just want to verify, because I don't
have
> much context.

Yes, though there is an outstanding question on this to fwereade, whether this
should be used more extensively in this refactor.

https://codereview.appspot.com/8018043/diff/12001/state/apiserver/api_test.go
File state/apiserver/api_test.go (left):

https://codereview.appspot.com/8018043/diff/12001/state/apiserver/api_test.go...
state/apiserver/api_test.go:605: }
On 2013/03/28 16:20:01, jameinel wrote:
> The old api had Refresh(), we don't actually need that one? clearly the old
docs
> were wrong anyway, since there wasn't PasswordValid before either.

Correct.  It doesn't appear that it was used anywhere below (however, a recent
branch by teknico may have changed this; will see during the merge).

https://codereview.appspot.com/8018043/diff/12001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/8018043/diff/12001/state/state.go#newcode286
state/state.go:286: }
On 2013/03/28 16:20:01, jameinel wrote:
> I'm curious on the reason to make these public interfaces. Mostly in that they
> used to be private, was there a reason for that? This seems to help by
avoiding
> extra definitions (you have them when you need them).
> So I think it is better, but I'm not sure if there was a reason they were
> private before.

Previously, the idea of an "entity" was more restricted to machines and units,
then users were added, and with the annotations work, it was muddied even
further, beyond the point where they could remain private.  The looser
definition and public interface do also help prevent duplication down the line,
especially in tests.

[matthew.scott, 2013-03-29 16:01:53]

*** Submitted:

Replace EntityName with Tag

This branch refactors EntityName to Tag per William and Roger in order to aid in
clarity.  Additionally, it adds a Tagger interface in state so that
state.Authenticator() can return a TaggedAuthenticator and state.Annotator() can
return a Tagged Annotator.  This was used to help clean up tests as well, as
many were defining namedEntity interfaces in order to expose EntityName in
tests.  Additionally, to be consistent, this branch changes (in a separate
commit) the EntityName field in the state Info and api Info objects to Tag.  API
params for login and annotations were also changed away from EntityName, and
docs were updated accordingly.  There will be a subsequent GUI branch to
accomodate for this if/after it lands.

R=fwereade, jameinel
CC=
https://codereview.appspot.com/8018043