Code review comment for lp:~johnweldon4/juju-core/action

Updated code to address items in this review.

Still need to do:

1) Transaction loop
2) Cleanup (maybe wait til fwereade refactor makes it to juju-core?)

https://codereview.appspot.com/98260043/diff/1/state/action.go
File state/action.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode23
state/action.go:23: Status ActionStatus
On 2014/05/15 12:47:26, fwereade wrote:
> Hmm, wondering now about the consequences on watches if we have the
status
> direct in the action doc. Would it make sense to leave status out for
now? The
> unit's going to be watching this collection, and doesn't need to
respond to
> status changes; but the client will be watching the *results*
collection, and
> might well want to respond to changes there. Sane?

Done.

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode70
state/action.go:70: err := a.st.runTransaction(ops)
On 2014/05/15 12:47:26, fwereade wrote:
> we should handle txn.ErrAborted and give a real error back -- in this
case it
> indicates (I think) that the action has already completed. Right?

Done.

https://codereview.appspot.com/98260043/diff/1/state/action_test.go
File state/action_test.go (right):

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode8
state/action_test.go:8: SettingsSuite
On 2014/05/15 18:02:34, fwereade wrote:
> Ooh, didn't spot that: this will include all the Test* methods from
> SettingsSuite as well, and we don't want that. I'd recommend ConnSuite
(the one
> right here in state_test, not the one in... er juju/testing?).

Done.

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode27
state/action_test.go:27: }
On 2014/05/15 18:02:34, fwereade wrote:
> On 2014/05/15 16:23:28, johnweldon4 wrote:
> >
> > On 2014/05/15 12:47:26, fwereade wrote:
> > > FWIW we usually leave these out, but I think the explicitness of
this
> approach
> > > may actually be better. Don't feel obliged to include them if you
don't
> think
> > > it's necessary.
> >
> >
> > Do you mean the (SetUp|TearDown)(Suite|Test) funcs?
> > Because they're just calling the inner SettingsSuite funcs does that
mean the
> > test harness will do it automatically?

> Yeah, if you embed a type you get all its methods (so long as there's
no
> ambiguity, which there isn't in this case).

Done.

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode30
state/action_test.go:30: action, err :=
s.SettingsSuite.state.AddAction("fakeunit/0", "fakeaction", nil)
On 2014/05/15 18:02:34, fwereade wrote:
> On 2014/05/15 16:23:28, johnweldon4 wrote:
> > On 2014/05/15 12:47:27, fwereade wrote:
> > > Hmm, I think we *should* make sure that the unit exists and isn't
Dead.
> > >
> > > (I've been thinking, and have tentatively concluded that actions
on dying
> > units
> > > are fine, they just might not run in time; but actions on dead
units are
> > > definitely insane.)
> >
> >
> > In this case I'm actually not even testing any "real" unit. Of
course when we
> > do the refactor to make func (u* Unit) AddAction(...) then we'll
need a real
> > Unit for this test.
> > But this test is kind of superfluous any way. state_test has a very
similar
> > test already.

> I think this suite's existence is good, it's nice to cluster the tests
for
> related functionality, and there's an awful lot on the direct State
tests
> already.

Done.

https://codereview.appspot.com/98260043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode320
state/state.go:320: for k := range oldConfig.AllAttrs() {
On 2014/05/15 16:23:29, johnweldon4 wrote:
> On 2014/05/15 12:47:27, fwereade wrote:
> > thanks for this cleanup btw

> I love cleanup :)

Done.

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1379
state/state.go:1379: func (st *State) AddAction(unit string, name
string, payload interface{}) (*Action, error) {
On 2014/05/16 05:37:30, johnweldon4 wrote:
> On 2014/05/15 12:47:27, fwereade wrote:
> > Thinking about this: I think it should be (u *Unit) AddAction(name
string,
> > payload interface{}) (...).
> >
> > Should be a simple move (and also make it easier to assert that the
unit isn't
> > dead).

> I'm assuming we put the Action doc in the same actions collection on
state?
> We're just doing the add on the Unit?

Done.

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1395
state/state.go:1395: return nil, onAbort(err, errDead)
On 2014/05/15 16:23:28, johnweldon4 wrote:
> On 2014/05/15 12:47:27, fwereade wrote:
> > I don't think errDead is a sensible error message for "doc already
exists".

> Er. Yes. Copy / Paste dumbness

Done.

https://codereview.appspot.com/98260043/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/98260043/diff/1/state/state_test.go#newcode182
state/state_test.go:182: answerAction, err :=
s.State.Action(testAction.Id())
On 2014/05/15 16:23:29, johnweldon4 wrote:
> On 2014/05/15 12:47:27, fwereade wrote:
> > It might be nice to have some sort of check on the form of the Id.

> Makes sense... I'll do something like "u#" == answerAction.Id()[:2],
maybe min
> length check and substring match on "#" in id[2:]. If you have any
suggestions
> let me know.

Done.

https://codereview.appspot.com/98260043/