api/common: shared Life implementation

api/common: shared Life implementation

This is intended for imminent use by the deployer; hence the changes to
common.PasswordChanger and the rhymingimplementation of LifeGetter. In
short: the set of units accessible to the deployer needs to be figured
out by looking in the db (the Authorizer's entity could be stale, and I
don't want to mix Refresh()y concerns in there), so the auth funcs are
generated as late as possible.

https://code.launchpad.net/~fwereade/juju-core/api-common-life/+merge/173721

(do not edit description out of merge proposal)

[fwereade, 2013-07-09 13:43:05]

Please take a look.

[dimitern, 2013-07-09 14:52:00]

Very nice! LGTM with just a few stylistic suggestions (now that we agreed to
conform to the imports separation).

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life_te...
File state/apiserver/common/life_test.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:7: "fmt"
\n after

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:9: "launchpad.net/juju-core/errors"
\n before

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
File state/apiserver/common/password_test.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password_test.go:8: . "launchpad.net/gocheck"
\n before and after

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password_test.go:13: stdtesting "testing"
move to top with "fmt"

https://codereview.appspot.com/11041043/diff/1/state/apiserver/machine/agent_...
File state/apiserver/machine/agent_test.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/machine/agent_...
state/apiserver/machine/agent_test.go:99: c.Assert(results, DeepEquals,
params.ErrorResults{
maybe put a comment like:
// s.agent is machine-1's agent and thus authorized to change the password
?

[jameinel, 2013-07-09 20:12:14]

So I think your cover letter means that while right now GetCanChange is the
same, you expect it to be different (and actually do a DB lookup) in your next
patch.

All the functionality LGTM, it is just slightly hard to see the need for the
change since the next bit is coming later.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life.go
File state/apiserver/common/life.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life.go...
state/apiserver/common/life.go:23: func NewLifeGetter(st *state.State,
getCanRead GetAuthFunc) *LifeGetter {
Internally LifeGetter is using a liferGetter.

Would you want to push that up into NewLifeGetter?

That would let you call "NewLifeGetter" with a mock.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life_te...
File state/apiserver/common/life_test.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:37: lg := common.NewMockLifeGetter(st,
getCanRead)
I think if you move the interface up to NewLifeGetter you don't need to
implement the NewMockLifeGetter.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
File state/apiserver/common/password.go (left):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password.go:32: if !pc.canChange(param.Tag) {
I'm still trying to fully understand this.
Why is it 'fresher' to use getCanChange here rather than just canChange?

Looking here:
https://codereview.appspot.com/11041043/patch/1/1009

The "auth" object is being passed into the NewAgentAPI function, so it is the
same 'auth' object that you were already using.

Are you expecting there will be cases where you have a fresher Auth state?

https://codereview.appspot.com/11041043/diff/1/state/apiserver/machine/agent.go
File state/apiserver/machine/agent.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/machine/agent....
state/apiserver/machine/agent.go:27: return func(tag string) bool {
// go 1.1: method ?

[fwereade, 2013-07-10 13:44:55]

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life.go
File state/apiserver/common/life.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life.go...
state/apiserver/common/life.go:23: func NewLifeGetter(st *state.State,
getCanRead GetAuthFunc) *LifeGetter {
On 2013/07/09 20:12:14, jameinel wrote:
> Internally LifeGetter is using a liferGetter.
> 
> Would you want to push that up into NewLifeGetter?
> 
> That would let you call "NewLifeGetter" with a mock.

And if I did the same for NewPasswordChanger I could drop export_test. I only
did it this way for consistency's sake; happy to change both. Those interfaces
do have ugly names, but I guess it's for the best.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life_te...
File state/apiserver/common/life_test.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:7: "fmt"
On 2013/07/09 14:52:00, dimitern wrote:
> \n after

Done.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:9: "launchpad.net/juju-core/errors"
On 2013/07/09 14:52:00, dimitern wrote:
> \n before

Done.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:37: lg := common.NewMockLifeGetter(st,
getCanRead)
On 2013/07/09 20:12:14, jameinel wrote:
> I think if you move the interface up to NewLifeGetter you don't need to
> implement the NewMockLifeGetter.

Done.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
File state/apiserver/common/password.go (left):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password.go:32: if !pc.canChange(param.Tag) {
On 2013/07/09 20:12:14, jameinel wrote:
> I'm still trying to fully understand this.
> Why is it 'fresher' to use getCanChange here rather than just canChange?
> 
> Looking here:
> https://codereview.appspot.com/11041043/patch/1/1009
> 
> The "auth" object is being passed into the NewAgentAPI function, so it is the
> same 'auth' object that you were already using.
> 
> Are you expecting there will be cases where you have a fresher Auth state?

Yeah, the specific case I'll be dealing with is getting unit life (and setting
unit password) from a deployer: the machine running that deployer only gets to
touch units assigned (directly or indirectly) to that machine. This set of units
can change at any time over the lifetime of the conn, but the auth entity itself
never gets refreshed.

If an authorizer *were* to set up a watch and expose always the latest known
version of its entity directly, that might be an alternative, but that seemed
like a bit of a leap... and it'd be a bit tricky to fit that into the Authorizer
methods anyway, because roundtrips; and fixing that will be tricky, because
cache invalidation.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
File state/apiserver/common/password_test.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password_test.go:8: . "launchpad.net/gocheck"
On 2013/07/09 14:52:00, dimitern wrote:
> \n before and after

Done.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password_test.go:13: stdtesting "testing"
On 2013/07/09 14:52:00, dimitern wrote:
> move to top with "fmt"

Done.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/machine/agent.go
File state/apiserver/machine/agent.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/machine/agent....
state/apiserver/machine/agent.go:27: return func(tag string) bool {
On 2013/07/09 20:12:14, jameinel wrote:
> // go 1.1: method ?

Done.

https://codereview.appspot.com/11041043/diff/1/state/apiserver/machine/agent_...
File state/apiserver/machine/agent_test.go (right):

https://codereview.appspot.com/11041043/diff/1/state/apiserver/machine/agent_...
state/apiserver/machine/agent_test.go:99: c.Assert(results, DeepEquals,
params.ErrorResults{
On 2013/07/09 14:52:00, dimitern wrote:
> maybe put a comment like:
> // s.agent is machine-1's agent and thus authorized to change the password
> ?

I think it's covered by "Create a machiner api for machine 1" in setup.