state: add AgentEntity

state: add AgentEntity

It collects together the methods we expect an
agent to use on Machine and Unit.

https://code.launchpad.net/~rogpeppe/juju-core/356-state-agent-entity/+merge/177892

(do not edit description out of merge proposal)

[rog, 2013-07-31 16:06:28]

Please take a look.

[dimitern, 2013-07-31 16:13:39]

Looks good so far, but I have some comments below

https://codereview.appspot.com/12175043/diff/3001/state/apiserver/common/pass...
File state/apiserver/common/password_test.go (right):

https://codereview.appspot.com/12175043/diff/3001/state/apiserver/common/pass...
state/apiserver/common/password_test.go:115: // Any AgentEntity methods we don't
implement on fakeEntity
I'm not sure this is a good idea. Why panic when we can have a compile-time
error, when trying to use an unimplemented method?

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

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode555
state/state.go:555: return nil, fmt.Errorf("entity %q is not an entity with an
agent", tag)
"%q is not an entity with an agent" ? 

Or maybe:
"entity %q does not have(support?) an agent"?

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

https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
state/state_test.go:1554: m, err := s.State.AddMachine("series",
state.JobHostUnits)
machine, err := .. please

https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
state/state_test.go:1559: e, err := s.State.AgentEntity(m.Tag())
entity, err := .. please

[dimitern, 2013-07-31 16:20:51]

After a discussion online, LGTM with the previous suggestions, except the one
about embedded AgentEntity.

On 2013/07/31 16:13:39, dimitern wrote:
> Looks good so far, but I have some comments below
> 
>
https://codereview.appspot.com/12175043/diff/3001/state/apiserver/common/pass...
> File state/apiserver/common/password_test.go (right):
> 
>
https://codereview.appspot.com/12175043/diff/3001/state/apiserver/common/pass...
> state/apiserver/common/password_test.go:115: // Any AgentEntity methods we
don't
> implement on fakeEntity
> I'm not sure this is a good idea. Why panic when we can have a compile-time
> error, when trying to use an unimplemented method?

Ignore this.

> 
> https://codereview.appspot.com/12175043/diff/3001/state/state.go
> File state/state.go (right):
> 
> https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode555
> state/state.go:555: return nil, fmt.Errorf("entity %q is not an entity with an
> agent", tag)
> "%q is not an entity with an agent" ? 
> 
> Or maybe:
> "entity %q does not have(support?) an agent"?
> 
> https://codereview.appspot.com/12175043/diff/3001/state/state_test.go
> File state/state_test.go (right):
> 
>
https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
> state/state_test.go:1554: m, err := s.State.AddMachine("series",
> state.JobHostUnits)
> machine, err := .. please
> 
>
https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
> state/state_test.go:1559: e, err := s.State.AgentEntity(m.Tag())
> entity, err := .. please

[rog, 2013-07-31 17:06:44]

Please take a look.

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

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode555
state/state.go:555: return nil, fmt.Errorf("entity %q is not an entity with an
agent", tag)
On 2013/07/31 16:13:40, dimitern wrote:
> "%q is not an entity with an agent" ? 
> 
> Or maybe:
> "entity %q does not have(support?) an agent"?

Done.

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

https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
state/state_test.go:1554: m, err := s.State.AddMachine("series",
state.JobHostUnits)
On 2013/07/31 16:13:40, dimitern wrote:
> machine, err := .. please

Done.

https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
state/state_test.go:1559: e, err := s.State.AgentEntity(m.Tag())
On 2013/07/31 16:13:40, dimitern wrote:
> entity, err := .. please

Done.

[fwereade, 2013-07-31 20:59:37]

I'm pretty certain PasswordChanger's scope shouldn't be narrowed like this, so
that bit NOT LGTM. Otherwise just quibbles really.

https://codereview.appspot.com/12175043/diff/3001/state/apiserver/common/pass...
File state/apiserver/common/password.go (right):

https://codereview.appspot.com/12175043/diff/3001/state/apiserver/common/pass...
state/apiserver/common/password.go:14: st           AgentEntityGetter
I don't think this is right at all. All you've done here is stop PasswordChanger
from being able to work with users...

https://codereview.appspot.com/12175043/diff/3001/state/apiserver/common/pass...
File state/apiserver/common/password_test.go (right):

https://codereview.appspot.com/12175043/diff/3001/state/apiserver/common/pass...
state/apiserver/common/password_test.go:115: // Any AgentEntity methods we don't
implement on fakeEntity
On 2013/07/31 16:13:40, dimitern wrote:
> I'm not sure this is a good idea. Why panic when we can have a compile-time
> error, when trying to use an unimplemented method?

Well, apart from the fact that auth and agenthood are only coincidentally
connected, this *would* I think be a good idea. In testing SetStatus or
EnsureDead fakes like this will be really convenient -- panics on unexpected
usage are ideal, and there's no implementation burden at all.

https://codereview.appspot.com/12175043/diff/3001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader.go (left):

https://codereview.appspot.com/12175043/diff/3001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader.go:145: entity :=
entity0.(state.SetAgentTooler)
Yeah, can we just drop the AgentTooler concept entirely? They're just part of
the set of methods implemented by every entity whose responsibilities are
fulfilled by an agent; and AFAICS there's nothing about those methods that's
relevant to anything not under that category.

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

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode477
state/state.go:477: MongoPassworder
If this interface had to be created in order to put it in here, I'm not sure its
existence independent of AgentEntity is justified.

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode478
state/state.go:478: AgentTooler
I don't think this interface has any independent value (much like Remover).
Doesn't need to change here, but expect my followup to drop stuff like that.

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode479
state/state.go:479: Annotator
Don't think this is relevant to agentness at all.

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode547
state/state.go:547: func (st *State) AgentEntity(tag string) (AgentEntity,
error) {
doc

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode555
state/state.go:555: return nil, fmt.Errorf("entity %q is not an entity with an
agent", tag)
On 2013/07/31 16:13:40, dimitern wrote:
> "%q is not an entity with an agent" ? 
> 
> Or maybe:
> "entity %q does not have(support?) an agent"?

I went with "does not support agent operations". As you like :).

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

https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
state/state_test.go:1550: _ state.AgentEntity = (*state.Unit)(nil)
Shouldn't these be by the definitions of Machine and Unit?

https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
state/state_test.go:1569: c.Assert(e, gc.IsNil)
Explicit tests for units too, please.

[rog, 2013-08-01 15:41:38]

Please take a look.

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

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode477
state/state.go:477: MongoPassworder
On 2013/07/31 20:59:37, fwereade wrote:
> If this interface had to be created in order to put it in here, I'm not sure
its
> existence independent of AgentEntity is justified.

For exported interfaces, experience suggests it's a good idea to have either all
explicit methods or all embedded interfaces. Combining the two means there's no
way to access the agent-tools-specific methods on their own even if we want
them. Even though I can see no current reason that we might want to do that, I'd
prefer to stick to that principle if possible (I can't see a particular down
side).

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode478
state/state.go:478: AgentTooler
On 2013/07/31 20:59:37, fwereade wrote:
> I don't think this interface has any independent value (much like Remover).
> Doesn't need to change here, but expect my followup to drop stuff like that.

As above.

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode479
state/state.go:479: Annotator
On 2013/07/31 20:59:37, fwereade wrote:
> Don't think this is relevant to agentness at all.

Done.

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode555
state/state.go:555: return nil, fmt.Errorf("entity %q is not an entity with an
agent", tag)
On 2013/07/31 20:59:37, fwereade wrote:
> On 2013/07/31 16:13:40, dimitern wrote:
> > "%q is not an entity with an agent" ? 
> > 
> > Or maybe:
> > "entity %q does not have(support?) an agent"?
> 
> I went with "does not support agent operations". As you like :).

+1

Done.

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

https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
state/state_test.go:1550: _ state.AgentEntity = (*state.Unit)(nil)
On 2013/07/31 20:59:37, fwereade wrote:
> Shouldn't these be by the definitions of Machine and Unit?

Ah I'd forgotten that. In go core, that's frowned upon. 

Done.

https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
state/state_test.go:1569: c.Assert(e, gc.IsNil)
On 2013/07/31 20:59:37, fwereade wrote:
> Explicit tests for units too, please.

Is that actually useful? We're not testing Unit-specific logic here. This test
gives 100% coverage of the logic in AgentEntity itself, and the static type
checks (and the unit-specific tests) cover the rest AFAICS.

What am I missing?

[fwereade, 2013-08-02 10:52:30]

LGTM

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

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode477
state/state.go:477: MongoPassworder
On 2013/08/01 15:41:38, rog wrote:
> On 2013/07/31 20:59:37, fwereade wrote:
> > If this interface had to be created in order to put it in here, I'm not sure
> its
> > existence independent of AgentEntity is justified.
> 
> For exported interfaces, experience suggests it's a good idea to have either
all
> explicit methods or all embedded interfaces. Combining the two means there's
no
> way to access the agent-tools-specific methods on their own even if we want
> them. Even though I can see no current reason that we might want to do that,
I'd
> prefer to stick to that principle if possible (I can't see a particular down
> side).

The only downside is the multiplication of apparently-unnecessary entities. Not
too bothered, it just seemed a bit off to me.

https://codereview.appspot.com/12175043/diff/3001/state/state.go#newcode478
state/state.go:478: AgentTooler
On 2013/08/01 15:41:38, rog wrote:
> On 2013/07/31 20:59:37, fwereade wrote:
> > I don't think this interface has any independent value (much like Remover).
> > Doesn't need to change here, but expect my followup to drop stuff like that.
> 
> As above.

Likewise, its very existence gives the impression that it's meaningful to get or
set tools of, or to remove, anything that isn't implemented by an agent. But
quibble quibble, not really a big dealAFAICS.

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

https://codereview.appspot.com/12175043/diff/3001/state/state_test.go#newcode...
state/state_test.go:1569: c.Assert(e, gc.IsNil)
On 2013/08/01 15:41:38, rog wrote:
> On 2013/07/31 20:59:37, fwereade wrote:
> > Explicit tests for units too, please.
> 
> Is that actually useful? We're not testing Unit-specific logic here. This test
> gives 100% coverage of the logic in AgentEntity itself, and the static type
> checks (and the unit-specific tests) cover the rest AFAICS.
> 
> What am I missing?

As discussed online, that the test thus assumes the underlying State.entity
implementation, and that it's therefore brittle in the face of changes by
inexperienced devs. I guess I'm ok exporting Entity() alone, testing that hard,
and just casting to appropriate types where it's used... I'd really prefer
keeping the existing model and testing the various implementations explicitly,
to avoid all the downcasting elsewhere, but /shrug. I'll leave it to your
judgment, and we can iterate if and when we have to.