jujuc: switch commands to use Context

jujuc: switch commands to use Context



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

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

(do not edit description out of merge proposal)

[fwereade, 2012-10-08 15:31:56]

Please take a look.

[niemeyer, 2012-10-09 17:33:30]

I'm pretty unhappy with the error convention as it is now. The vast majority of
cases where the methods are used, we check for their correctness explicitly
where observing the error would make sense, and in the other cases I can't tell
whether it is safe or not without looking at the implementation, that doesn't
exist yet.

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

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/context.go#...
worker/uniter/jujuc/context.go:180: r := ctx.HookRelation()
For our own understanding, err would fit nicely here (1).

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/context.go#...
worker/uniter/jujuc/context.go:184: vars = append(vars,
"JUJU_REMOTE_UNIT="+ctx.RemoteUnitName())
(1)

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/context.go#...
worker/uniter/jujuc/context.go:401: r := ctx.HookRelation()
(1)

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/juju-log.go
File worker/uniter/jujuc/juju-log.go (right):

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/juju-log.go...
worker/uniter/jujuc/juju-log.go:44: badge = badge + " " +
c.ctx.HookRelation().FakeId()
(1)

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-ge...
File worker/uniter/jujuc/relation-get.go (right):

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-ge...
worker/uniter/jujuc/relation-get.go:30: doc += fmt.Sprintf("Current default unit
id is %q.", c.ctx.RemoteUnitName())
(1)

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-ge...
worker/uniter/jujuc/relation-get.go:56: c.UnitName = c.ctx.RemoteUnitName()
(1)

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-ge...
worker/uniter/jujuc/relation-get.go:69: r := c.ctx.Relation(c.RelationId)
What if the remote side lost this relation for whatever reason? Panic and blow
up the uniter? (1)

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-id...
File worker/uniter/jujuc/relation-ids.go (right):

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-id...
worker/uniter/jujuc/relation-ids.go:26: doc = fmt.Sprintf("Current default
relation name is %q.", c.ctx.HookRelation().Name())
(1)

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-id...
worker/uniter/jujuc/relation-ids.go:40: c.Name = c.ctx.HookRelation().Name()
(1)

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-id...
worker/uniter/jujuc/relation-ids.go:54: r := c.ctx.Relation(id)
This seems alright, assuming that all relations every listed are already in
memory by the time RelationIds is called. Otherwise we have the same issue as
mentioned before.

I'll tag that as (2) for now.

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-li...
File worker/uniter/jujuc/relation-list.go (right):

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-li...
worker/uniter/jujuc/relation-list.go:25: doc = fmt.Sprintf("Current default
relation id is %q.", c.ctx.HookRelation().FakeId())
(1)

https://codereview.appspot.com/6633043/diff/1/worker/uniter/jujuc/relation-li...
worker/uniter/jujuc/relation-list.go:52: return c.out.Write(ctx,
c.ctx.Relation(c.RelationId).UnitNames())
Same: do we guarantee the existence of Relation because HasRelation returned
true before?

[fwereade, 2012-10-09 20:04:20]

After live discussion, we (think ;) we) can compromise on (result, ok) returns.
Doing that now.

[fwereade, 2012-10-09 20:42:36]

Please take a look.

[niemeyer, 2012-10-09 20:50:25]

LGTM, thanks!

[fwereade, 2012-10-09 21:02:59]

*** Submitted:

jujuc: switch commands to use Context

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