Issue 6620054: jujuc: add Context interface
|
Created:
11 years, 7 months ago by fwereade Modified:
11 years, 7 months ago Reviewers:
mp+128266 Visibility:
Public. |
Descriptionjujuc: add Context interface
https://code.launchpad.net/~fwereade/juju-core/jujuc-context-interface/+merge/128266
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : jujuc: add Context interface #
Total comments: 25
Patch Set 3 : jujuc: add Context interface #Patch Set 4 : jujuc: add Context interface #
Total comments: 1
Patch Set 5 : jujuc: add Context interface #MessagesTotal messages: 10
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please let me know what you think of the following suggestions: https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... File worker/uniter/jujuc/interface.go (right): https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:5: // Context expresses the capabilities of a hook. // Context is the interface that all hook helper commands // depend on to interact with the rest of the system. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:9: Unit() string UnitName https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:31: RelationId() int // ... // It returns ErrNoRelation when not running a relation hook. RelationId() (int, error) Avoids mistakes. It's easy and convenient to run RelationId() and forget it can be invalid. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:36: CounterpartUnit() string // RemoteUnitName returns the name of the remote unit the hook // execution is associated with. // ErrNoRemote is returned in case no remote unit is associated // with the hook execution. RemoteUnitName() (string, error) In the context of hook execution we can make (and have been making) good use of the term "remote". https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:43: // not known, it returns ErrRelationNotFound. // It returns ErrNoRelation when the relation is not known. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:48: var ErrRelationNotFound = errors.New("relation not found") ErrNoRelation https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:51: type Relation interface { // RelationContext is the interface that all hook helper commands that handle // relation concerns depend on to interact with the rest of the system. (we have a Relation, and that's not its interface; RelationContext may not be 100% right, but the analogy with Context is useful) https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:54: // the kind of relation only. // Name returns the name the locally executing charm assigned to this relation. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:58: // identifies the relation to the hook. // ... // identifies the relation to the hook. In reality, the identification // of the relation is the integer following the colon, but the composed // name is useful to humans observing it. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:64: // Settings allows read/write access to the unit's settings in this relation. s/unit/local unit/ https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:67: // ReadSettings returns the settings of any counterpart unit in the s/counterpart/remote/ https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:73: type MapChanger interface { // Settings is implemented by types that manipulate unit settings.
Sign in to reply to this message.
https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... File worker/uniter/jujuc/interface.go (right): https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:51: type Relation interface { On 2012/10/05 17:00:33, niemeyer wrote: > // RelationContext is the interface that all hook helper commands that handle > // relation concerns depend on to interact with the rest of the system. > > (we have a Relation, and that's not its interface; RelationContext may not be > 100% right, but the analogy with Context is useful) Actually, *ContextRelation* might be 100% right.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... File worker/uniter/jujuc/interface.go (right): https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:5: // Context expresses the capabilities of a hook. On 2012/10/05 17:00:33, niemeyer wrote: > // Context is the interface that all hook helper commands > // depend on to interact with the rest of the system. Done. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:9: Unit() string On 2012/10/05 17:00:33, niemeyer wrote: > UnitName Done. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:31: RelationId() int On 2012/10/05 17:00:33, niemeyer wrote: > // ... > // It returns ErrNoRelation when not running a relation hook. > RelationId() (int, error) > > Avoids mistakes. It's easy and convenient to run RelationId() and forget it can > be invalid. Done. Not 100% sure, but happy to see how it works out in practice :). https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:36: CounterpartUnit() string On 2012/10/05 17:00:33, niemeyer wrote: > // RemoteUnitName returns the name of the remote unit the hook > // execution is associated with. > // ErrNoRemote is returned in case no remote unit is associated > // with the hook execution. > RemoteUnitName() (string, error) > > In the context of hook execution we can make (and have been making) good use of > the term "remote". Yeah -- I thought there were some issues with "remote", but perhaps this is not the place to mess with terminology too much. Not really beeling the benefits of ErrNoRemote, but done regardless. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:43: // not known, it returns ErrRelationNotFound. On 2012/10/05 17:00:33, niemeyer wrote: > // It returns ErrNoRelation when the relation is not known. Done. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:48: var ErrRelationNotFound = errors.New("relation not found") On 2012/10/05 17:00:33, niemeyer wrote: > ErrNoRelation Done. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:51: type Relation interface { On 2012/10/05 17:02:57, niemeyer wrote: > On 2012/10/05 17:00:33, niemeyer wrote: > > // RelationContext is the interface that all hook helper commands that handle > > // relation concerns depend on to interact with the rest of the system. > > > > (we have a Relation, and that's not its interface; RelationContext may not be > > 100% right, but the analogy with Context is useful) > > Actually, *ContextRelation* might be 100% right. Done; but I think that really we have a *state*.Relation; and that that is not in itself enough reason to eschew a jujuc.Relation. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:54: // the kind of relation only. On 2012/10/05 17:00:33, niemeyer wrote: > // Name returns the name the locally executing charm assigned to this relation. Much better, ty. Done. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:58: // identifies the relation to the hook. On 2012/10/05 17:00:33, niemeyer wrote: > // ... > // identifies the relation to the hook. In reality, the identification > // of the relation is the integer following the colon, but the composed > // name is useful to humans observing it. Done. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:62: Units() []string I'm suddenly thinking that , for consistency's sake, this shoudl really be UnitNames. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:64: // Settings allows read/write access to the unit's settings in this relation. On 2012/10/05 17:00:33, niemeyer wrote: > s/unit/local unit/ Done. https://codereview.appspot.com/6620054/diff/3001/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:67: // ReadSettings returns the settings of any counterpart unit in the On 2012/10/05 17:00:33, niemeyer wrote: > s/counterpart/remote/ Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/6620054/diff/5002/worker/uniter/jujuc/interfac... File worker/uniter/jujuc/interface.go (right): https://codereview.appspot.com/6620054/diff/5002/worker/uniter/jujuc/interfac... worker/uniter/jujuc/interface.go:51: Relation(id int) ContextRelation I think this and HookRelation should both have an error result. You'll have to do this: if HasRelation(id); Relation(id) instead of this: if r, err := Relation(id); err != nil which is conventional. I'm willing to wait and see, though.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
On 2012/10/08 13:29:55, niemeyer wrote: > https://codereview.appspot.com/6620054/diff/5002/worker/uniter/jujuc/interfac... > File worker/uniter/jujuc/interface.go (right): > > https://codereview.appspot.com/6620054/diff/5002/worker/uniter/jujuc/interfac... > worker/uniter/jujuc/interface.go:51: Relation(id int) ContextRelation > I think this and HookRelation should both have an error result. You'll have to > do this: > > if HasRelation(id); Relation(id) > > instead of this: > > if r, err := Relation(id); err != nil > > which is conventional. > > I'm willing to wait and see, though. Cheers -- I think it really comes out quite nicely in the end. But we shall see :).
Sign in to reply to this message.
*** Submitted: jujuc: add Context interface R=niemeyer CC= https://codereview.appspot.com/6620054
Sign in to reply to this message.
|
