|
|
|
state: implement the single FindEntity method
We rely on clients to do the type conversions
themselves if they want, rather than trying to
second-guess every possible interface that
a they might want to use.
Create a new state/interface.go file to hold interfaces
that entities can conform to.
https://code.launchpad.net/~rogpeppe/juju-core/363-agent-entity/+merge/178825
Requires: https://code.launchpad.net/~rogpeppe/juju-core/362-names-parse-tag/+merge/178619
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : state: implement the single FindEntity method #
Total comments: 9
Patch Set 3 : state: implement the single FindEntity method #Patch Set 4 : state: implement the single FindEntity method #Patch Set 5 : state: implement the single FindEntity method #Patch Set 6 : state: implement the single FindEntity method #Patch Set 7 : state: implement the single FindEntity method #
Total comments: 3
Patch Set 8 : state: implement the single FindEntity method #Patch Set 9 : state: implement the single FindEntity method #
Total comments: 2
Total messages: 8
|
rog
Please take a look.
|
12 years, 5 months ago (2013年08月06日 17:59:26 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
Looks really good, but I'm concerned about some test cases in apiserver/common being removed. Also needs merging trunk changes in. https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/life... File state/apiserver/common/life_test.go (left): https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/life... state/apiserver/common/life_test.go:55: "x3": {err: fmt.Errorf("x3 error")}, Please, do not remove this case. https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/remo... File state/apiserver/common/remove.go (right): https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/remo... state/apiserver/common/remove.go:28: func (r *Remover) removeEntity(entityp params.Entity) error { s/entityp params.Entity/tag string/ https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/remo... File state/apiserver/common/remove_test.go (left): https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/remo... state/apiserver/common/remove_test.go:67: "x5": {life: state.Dead, err: fmt.Errorf("x5 error")}, Don't remove this as well please. https://codereview.appspot.com/12551043/diff/3001/state/state.go File state/state.go (left): https://codereview.appspot.com/12551043/diff/3001/state/state.go#oldcode467 state/state.go:467: // Tagger represents an entity with a tag. Yay! https://codereview.appspot.com/12551043/diff/3001/state/state_test.go File state/state_test.go (left): https://codereview.appspot.com/12551043/diff/3001/state/state_test.go#oldcode... state/state_test.go:1538: // TODO(fwereade): when lp:1199352 (relation lacks Tag) is fixed, check Please, graft this TODO comment somewhere in the new code.
Please take a look. https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/life... File state/apiserver/common/life_test.go (left): https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/life... state/apiserver/common/life_test.go:55: "x3": {err: fmt.Errorf("x3 error")}, On 2013年08月07日 09:13:09, dimitern wrote: > Please, do not remove this case. Done. https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/remo... File state/apiserver/common/remove.go (right): https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/remo... state/apiserver/common/remove.go:28: func (r *Remover) removeEntity(entityp params.Entity) error { On 2013年08月07日 09:13:09, dimitern wrote: > s/entityp params.Entity/tag string/ Done. https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/remo... File state/apiserver/common/remove_test.go (left): https://codereview.appspot.com/12551043/diff/3001/state/apiserver/common/remo... state/apiserver/common/remove_test.go:67: "x5": {life: state.Dead, err: fmt.Errorf("x5 error")}, On 2013年08月07日 09:13:09, dimitern wrote: > Don't remove this as well please. Done. https://codereview.appspot.com/12551043/diff/3001/state/state_test.go File state/state_test.go (left): https://codereview.appspot.com/12551043/diff/3001/state/state_test.go#oldcode... state/state_test.go:1538: // TODO(fwereade): when lp:1199352 (relation lacks Tag) is fixed, check On 2013年08月07日 09:13:09, dimitern wrote: > Please, graft this TODO comment somewhere in the new code. Done.
LGTM with that TODO reinstated. https://codereview.appspot.com/12551043/diff/17010/state/apiserver/common/ens... File state/apiserver/common/ensuredead.go (left): https://codereview.appspot.com/12551043/diff/17010/state/apiserver/common/ens... state/apiserver/common/ensuredead.go:18: type DeadEnsurerer interface { I'm gonna miss that :) https://codereview.appspot.com/12551043/diff/17010/state/apiserver/common/ens... File state/apiserver/common/ensuredead.go (right): https://codereview.appspot.com/12551043/diff/17010/state/apiserver/common/ens... state/apiserver/common/ensuredead.go:35: return NotSupportedError(tag, "ensuring death") so grim.. :) but I have not better suggestion. https://codereview.appspot.com/12551043/diff/17010/state/state_test.go File state/state_test.go (left): https://codereview.appspot.com/12551043/diff/17010/state/state_test.go#oldcod... state/state_test.go:1538: // TODO(fwereade): when lp:1199352 (relation lacks Tag) is fixed, check Where is this TODO?
Please take a look.
Please take a look.
LGTM. Kills the -erers which is grand, and the extra cast/check error/NotImplementedError everywhere is offset by the test fakes becoming much simpler. https://codereview.appspot.com/12551043/diff/26001/state/apiserver/common/err... File state/apiserver/common/errors.go (right): https://codereview.appspot.com/12551043/diff/26001/state/apiserver/common/err... state/apiserver/common/errors.go:24: func NotSupportedError(entity, operation string) error { I wonder if this wants a less generic name? I'm not sure the package prefix is enough to say this is about entities not supporting operations.
https://codereview.appspot.com/12551043/diff/26001/state/apiserver/common/err... File state/apiserver/common/errors.go (right): https://codereview.appspot.com/12551043/diff/26001/state/apiserver/common/err... state/apiserver/common/errors.go:24: func NotSupportedError(entity, operation string) error { On 2013年08月07日 14:54:42, gz wrote: > I wonder if this wants a less generic name? I'm not sure the package prefix is > enough to say this is about entities not supporting operations. A reasonable point. I'll change it to use a better name, if I can think of one.