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/