Merge lp://staging/~fwereade/juju-core/jujuc-server-get-context into lp://staging/~juju/juju-core/trunk

Reviewers: mp+128876_code.launchpad.net,

Message:
Please take a look.

Description:
jujuc: command registration

Commands are registered at package level; servers are created with
context-
getting functions instead of command-getting ones.

https://code.launchpad.net/~fwereade/juju-core/jujuc-server-get-context/+merge/128876

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6632059/

Affected files:
   A [revision details]
   M cmd/jujuc/main_test.go
   M worker/uniter/jujuc/server.go
   M worker/uniter/jujuc/server_test.go
   M worker/uniter/uniter.go

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go
File cmd/jujuc/main_test.go (right):

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go#newcode67
cmd/jujuc/main_test.go:67: func init() {
Doesn't seem right. I believe this should be done within the test
itself, and undone once the test is finished, to avoid affecting
everything else as if this was part of the implementation.

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go#newcode98
cmd/jujuc/main_test.go:98: getCtx := func(contextId string)
(jujuc.Context, error) {
If we're touching this, is there any benefit in continuing to pretend we
have a contextId, when in fact we know we only have one context?

https://codereview.appspot.com/6632059/

Please take a look.

https://codereview.appspot.com/6632059/

On 2012/10/11 07:01:48, fwereade wrote:
> Please take a look.

er, sorry, ignore that, wipping

https://codereview.appspot.com/6632059/

Please take a look.

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go
File cmd/jujuc/main_test.go (right):

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go#newcode67
cmd/jujuc/main_test.go:67: func init() {
On 2012/10/10 12:52:02, niemeyer wrote:
> Doesn't seem right. I believe this should be done within the test
itself, and
> undone once the test is finished, to avoid affecting everything else
as if this
> was part of the implementation.

Yes, I would appear to have been on crack there. Sorry.

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go#newcode98
cmd/jujuc/main_test.go:98: getCtx := func(contextId string)
(jujuc.Context, error) {
On 2012/10/10 12:52:02, niemeyer wrote:
> If we're touching this, is there any benefit in continuing to pretend
we have a
> contextId, when in fact we know we only have one context?

Well, it's still required in one way or another for sanity-checking; and
while we *are* still using a context id I'd prefer to keep that name.
Sane?

(BTW, while we're on this subject -- should I be planning to keep
JUJU_CONTEXT_ID, for stability's sake, or to rename it at some point?)

https://codereview.appspot.com/6632059/

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go
File cmd/jujuc/main_test.go (right):

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go#newcode98
cmd/jujuc/main_test.go:98: getCtx := func(contextId string)
(jujuc.Context, error) {
On 2012/10/11 07:18:17, fwereade wrote:
> On 2012/10/10 12:52:02, niemeyer wrote:
> > If we're touching this, is there any benefit in continuing to
pretend we have
> a
> > contextId, when in fact we know we only have one context?

> Well, it's still required in one way or another for sanity-checking;
and while
> we *are* still using a context id I'd prefer to keep that name. Sane?

I think it's guiding the interface in a misleading direction.
getContext(id) is perhaps entirely unnecessary if there's only ever a
single context. Why are we providing a *get context* function when we
can provide the *context*?

What used to be an id is now (as I understand our agreement) simply a
verification token. That may be done via the context's interface itself,
by asking it what's the current token and comparing locally at an
appropriate location.

> (BTW, while we're on this subject -- should I be planning to keep
> JUJU_CONTEXT_ID, for stability's sake, or to rename it at some point?)

I'd be happy to see it renamed to be more realistic at some point.
There's very little reason I can imagine for someone to be using this
variable, as it never changes.

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go#newcode107
cmd/jujuc/main_test.go:107: srv, err := jujuc.NewServer(getCtx,
s.sockPath)
NewServer(context, s.sockPath) where necessary?

https://codereview.appspot.com/6632059/

On 2012/10/12 03:19:41, niemeyer wrote:
> https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go
> File cmd/jujuc/main_test.go (right):

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go#newcode98
> cmd/jujuc/main_test.go:98: getCtx := func(contextId string)
(jujuc.Context,
> error) {
> On 2012/10/11 07:18:17, fwereade wrote:
> > On 2012/10/10 12:52:02, niemeyer wrote:
> > > If we're touching this, is there any benefit in continuing to
pretend we
> have
> > a
> > > contextId, when in fact we know we only have one context?
> >
> > Well, it's still required in one way or another for sanity-checking;
and while
> > we *are* still using a context id I'd prefer to keep that name.
Sane?

> I think it's guiding the interface in a misleading direction.
getContext(id) is
> perhaps entirely unnecessary if there's only ever a single context.
Why are we
> providing a *get context* function when we can provide the *context*?

> What used to be an id is now (as I understand our agreement) simply a
> verification token. That may be done via the context's interface
itself, by
> asking it what's the current token and comparing locally at an
appropriate
> location.

This begs the question a bit -- we're assuming, IMO without enough
justification, that it's a good idea to have a long-lived Context; I'm
starting to think that actually it's not such a great idea. Not
necessarily *bad*, mind you, just not an assumption I want to act on at
this point. The intent of this CL was just to rationalise the command
selection; context "selection" is a different issue that I'll address
when it seems sensible.

That said, if you really feel this change takes us in a *bad* direction,
I'm happy to drop it; but I'm a little surprised, because I had seen it
as neutral from that perspective.

> > (BTW, while we're on this subject -- should I be planning to keep
> > JUJU_CONTEXT_ID, for stability's sake, or to rename it at some
point?)

> I'd be happy to see it renamed to be more realistic at some point.
There's very
> little reason I can imagine for someone to be using this variable, as
it never
> changes.

https://codereview.appspot.com/6632059/diff/1/cmd/jujuc/main_test.go#newcode107
> cmd/jujuc/main_test.go:107: srv, err := jujuc.NewServer(getCtx,
s.sockPath)
> NewServer(context, s.sockPath) where necessary?

https://codereview.appspot.com/6632059/