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

Should just be another round, I think. Ping me when you're online and we
can chat.

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
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?

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode34
state/action.go:34: action := &Action{
return &Action{

?

https://codereview.appspot.com/98260043/diff/1/state/action.go#newcode70
state/action.go:70: err := a.st.runTransaction(ops)
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?

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#newcode27
state/action_test.go:27: }
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.

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)
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.)

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() {
thanks for this cleanup btw

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) {
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).

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1395
state/state.go:1395: return nil, onAbort(err, errDead)
I don't think errDead is a sensible error message for "doc already
exists".

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1396
state/state.go:1396: }
Given that we'll want two asserts here, we'll have two possible failure
modes (even if the one you're currently handling should be a cant-happen
one). Let's talk today about how to write the code in (what I think is)
the cleanest way, because it's not actually very obvious, I'm afraid.

Yeah, this is another thing I should be documenting.

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())
It might be nice to have some sort of check on the form of the Id.

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