jujuc: HookContext now implements Context

jujuc: HookContext now implements Context

...and RelationContext implements ContextRelation. Name collisions have been
resolved with trailing _s, which will persist for a little while, but are not
intended to be long-term features.

Note that some logic in ConfigGetCommand moved behind the implementation of
Config: it needs to be implemented, but it's best that it's only done once ;).

https://code.launchpad.net/~fwereade/juju-core/juju-context-implement-interface/+merge/128443

Requires: https://code.launchpad.net/~fwereade/juju-core/jujuc-context-interface/+merge/128266

(do not edit description out of merge proposal)

[fwereade, 2012-10-08 08:58:55]

Please take a look.

[fwereade, 2012-10-08 13:21:46]

Please take a look.

[niemeyer, 2012-10-08 14:12:43]

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context.go
File worker/uniter/jujuc/context.go (right):

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:20: // HookContext allows clients to run hooks
against a specific environment.
// HookContext is the implementation of jujuc.Context.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:22: // order to avoid collisions.
Why are these fields being exposed? As I understand we're offering the same data
via the relation interface. Everything here should works towards privacy, and we
have a conflict it's a good hint its time to hide them and use the method.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:45: // UnitName is needed to implement Context.
d

We've already described that in the method. The repetition on every single
method doesn't help as it says nothing about what the method does.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:50: // PublicAddress is needed to implement
Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:55: // PrivateAddress is needed to implement
Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:60: // OpenPort is needed to implement Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:65: // ClosePort is needed to implement Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:70: // Config is needed to implement Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:97: // HasHookRelation is needed to implement
Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:102: // HookRelation is needed to implement
Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:107: // HasRemoteUnit is needed to implement
Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:112: // RemoteUnitName is needed to implement
Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:120: // HasRelation is needed to implement
Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:126: // Relation is needed to implement Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:135: // RelationIds is needed to implement
Context.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:312: // It implements ContextRelation.
// RelationContext is the implementation of jujuc.ContextRelation.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:375: // Name is needed to implement
ContextRelation.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:380: // FakeId is needed to implement
ContextRelation.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:385: // UnitNames is needed to implement
ContextRelation.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:394: // Settings is needed to implement
ContextRelation.
d

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:406: // ReadSettings is needed to implement
ContextRelation.
d

[fwereade, 2012-10-08 15:02:02]

Please take a look.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context.go
File worker/uniter/jujuc/context.go (right):

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:20: // HookContext allows clients to run hooks
against a specific environment.
On 2012/10/08 14:12:43, niemeyer wrote:
> // HookContext is the implementation of jujuc.Context.

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:22: // order to avoid collisions.
On 2012/10/08 14:12:43, niemeyer wrote:
> Why are these fields being exposed? As I understand we're offering the same
data
> via the relation interface. Everything here should works towards privacy, and
we
> have a conflict it's a good hint its time to hide them and use the method.

I am working towards privacy; but for the moment these stay public because the
tests depend on the concrete HookContext. As soon as HookContext has moved to
another package, and the tests have been rewritten to use their own
implementation of Context, all these will become hidden.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:45: // UnitName is needed to implement Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d
> 
> We've already described that in the method. The repetition on every single
> method doesn't help as it says nothing about what the method does.

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:50: // PublicAddress is needed to implement
Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:55: // PrivateAddress is needed to implement
Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:60: // OpenPort is needed to implement Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:65: // ClosePort is needed to implement Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:70: // Config is needed to implement Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:97: // HasHookRelation is needed to implement
Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:102: // HookRelation is needed to implement
Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:107: // HasRemoteUnit is needed to implement
Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:112: // RemoteUnitName is needed to implement
Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:120: // HasRelation is needed to implement
Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:126: // Relation is needed to implement Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/context....
worker/uniter/jujuc/context.go:135: // RelationIds is needed to implement
Context.
On 2012/10/08 14:12:43, niemeyer wrote:
> d

Done.

[niemeyer, 2012-10-08 16:40:48]

LGTM

[fwereade, 2012-10-09 06:28:02]

*** Submitted:

jujuc: HookContext now implements Context

...and RelationContext implements ContextRelation. Name collisions have been
resolved with trailing _s, which will persist for a little while, but are not
intended to be long-term features.

Note that some logic in ConfigGetCommand moved behind the implementation of
Config: it needs to be implemented, but it's best that it's only done once ;).

R=niemeyer
CC=
https://codereview.appspot.com/6624062