Actions: Work in Progress

Actions: Work in Progress

Adding action documents to the State

https://code.launchpad.net/~johnweldon4/juju-core/action/+merge/219225

(do not edit description out of merge proposal)

[johnweldon4, 2014-05-14 05:39:11]

Please take a look.

[fwereade, 2014-05-15 12:47:27]

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.

[johnweldon4, 2014-05-15 16:23:29]

A few comments and responses.  We can chat on irc or hangouts whenever.

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#newcode34
state/action.go:34: action := &Action{
On 2014/05/15 12:47:26, fwereade wrote:
> return &Action{
> 
> ?

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

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?

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

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 12:47:27, fwereade wrote:
> thanks for this cleanup btw

I love cleanup :)

https://codereview.appspot.com/98260043/diff/1/state/state.go#newcode1395
state/state.go:1395: return nil, onAbort(err, errDead)
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

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

[fwereade, 2014-05-15 18:02:35]

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

https://codereview.appspot.com/98260043/diff/1/state/action_test.go#newcode27
state/action_test.go:27: }
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).

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

[johnweldon4, 2014-05-16 05:37:30]

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#newcode1379
state/state.go:1379: func (st *State) AddAction(unit string, name string,
payload interface{}) (*Action, error) {
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?

[johnweldon4, 2014-05-20 06:20:30]

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.

[johnweldon4, 2014-05-20 06:29:01]

Please take a look.

[fwereade, 2014-05-20 07:12:17]

Looking good. Ping me when you're on, and we can go through the transaction loop
stuff.

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

https://codereview.appspot.com/98260043/diff/20001/state/action.go#newcode8
state/action.go:8: Unit    string
Drop the Unit field.

https://codereview.appspot.com/98260043/diff/20001/state/action.go#newcode31
state/action.go:31: // Id returns the mongo Id of the Action
even if it *is* the mongo id, we shouldn't *tell* people it is (if you see what
I mean).

I think that what we end up exposing to the user will want to be some long hex
string (identifiable by first few characters -- think git revisions)... but
let's not quibble about that now. This is the PK we want, and until we expose
the functionality we're free to fix it later.

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

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1314
state/unit.go:1314: func (u *Unit) AddAction(name string, payload
map[string]interface{}) (*Action, error) {
is it an interface{}, or a map[string]interface{}? I think it's the latter, but
then the doc needs to match as well.

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1332
state/unit.go:1332: // TODO(jcw4) transaction loop
also needs assert on unit life in the loop.

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

https://codereview.appspot.com/98260043/diff/20001/state/unit_test.go#newcode...
state/unit_test.go:1299: c.Assert(action.Id(), gc.Matches,
"^u#"+s.unit.Name()+"#\\d+")
I'd drop the empty TokenTest in the other file and just add a couple of bits
here: check that you can get the action by Id, and that the objects match.

[fwereade, 2014-05-20 14:32:09]

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

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1332
state/unit.go:1332: // TODO(jcw4) transaction loop
See https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode932
for comments on loops -- ping me when you've read it.

[johnweldon4, 2014-05-20 21:55:13]

Addressed all of these issues I believe.

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

https://codereview.appspot.com/98260043/diff/20001/state/action.go#newcode8
state/action.go:8: Unit    string
On 2014/05/20 07:12:17, fwereade wrote:
> Drop the Unit field.

Done.

https://codereview.appspot.com/98260043/diff/20001/state/action.go#newcode31
state/action.go:31: // Id returns the mongo Id of the Action
On 2014/05/20 07:12:17, fwereade wrote:
> even if it *is* the mongo id, we shouldn't *tell* people it is (if you see
what
> I mean).
> 
> I think that what we end up exposing to the user will want to be some long hex
> string (identifiable by first few characters -- think git revisions)... but
> let's not quibble about that now. This is the PK we want, and until we expose
> the functionality we're free to fix it later.

Done.

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

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1314
state/unit.go:1314: func (u *Unit) AddAction(name string, payload
map[string]interface{}) (*Action, error) {
On 2014/05/20 07:12:17, fwereade wrote:
> is it an interface{}, or a map[string]interface{}? I think it's the latter,
but
> then the doc needs to match as well.

Done.

https://codereview.appspot.com/98260043/diff/20001/state/unit.go#newcode1332
state/unit.go:1332: // TODO(jcw4) transaction loop
On 2014/05/20 14:32:08, fwereade wrote:
> See
https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode932
> for comments on loops -- ping me when you've read it.

Done.

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

https://codereview.appspot.com/98260043/diff/20001/state/unit_test.go#newcode...
state/unit_test.go:1299: c.Assert(action.Id(), gc.Matches,
"^u#"+s.unit.Name()+"#\\d+")
On 2014/05/20 07:12:17, fwereade wrote:
> I'd drop the empty TokenTest in the other file and just add a couple of bits
> here: check that you can get the action by Id, and that the objects match.

Done.

[johnweldon4, 2014-05-20 22:01:02]

Please take a look.

[fwereade, 2014-05-21 07:39:48]

Many comments, because I like the sound of my own typing, but very little
actionable. I'd appreciate your thoughts but this basically LGTM.

Fix any trivials and let me know when it's ready to merge, and I'll approve it.

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

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1325
state/unit.go:1325: if notDead, err := isNotDead(u.st.units, u.doc.Name); err !=
nil {
This one is interesting -- it's a not-strictly-necessary DB hit before running
the txn, and I'd ordinarily shy away from that in favour of checking u.doc.Life
-- but let's see how it goes. I suspect we'll need to go with the other approach
in the end, as we start to care about things like charm versions, but we can
wait and see if I'm right about that.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1331
state/unit.go:1331: sel := bson.D{{"_id", newActionID}}
This block is almost redundant, because we should be able to depend on the
uniqueness of sequence IDs (even in the case of a new unit being created with
the same name as an old one (which is itself a bug (although it only happens
when a service is destroyed and recreated))).

If it didn't involve an extra DB hit I'd have no problem with it; but as it is
I'm gently leaning towards dropping it. If there's a problem here we'll fall out
to ErrExcessiveContention anyway, and it should actually be pretty easy to
diagnose from there.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1340
state/unit.go:1340: ops := []txn.Op{{
As discussed yesterday, in this particular case the ops don't have to be rebuilt
each time, but it's probably good to keep the familiar structure.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1351
state/unit.go:1351: return newActionID, err
returning the id is not "normal" for this package, but IMO it's a good idea.
thanks.

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

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode...
state/unit_test.go:1304: // verify action Id() is of expected form (unit id
prefix, + sequence)
Add a note that this is temporary, and we shouldn't really leak the actual _id.

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode...
state/unit_test.go:1335: unitDead := state.TransactionHook{
s/unitDead/killUnit/ perhaps?

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode...
state/unit_test.go:1348: //TODO(jcw4): need to figure out how to inject
'contention' into the transaction
Haha. I've thought of a way, and I kinda endorse you doing it, so long as you
clearly comment that it'll stop working when lp:1174610 is fixed. In the Before
hook, you get st.Unit(name) and make it Dead; in the After hook, you remove the
unit and the service, and create a new service with the same name. From the POV
of that method the unit will appear to be resurrected.

But that's also dirty as sin, and will break when we fix other stuff, so I'm
reasonably comfortable skipping it. (The other possibility, which is *maybe*
cleaner, is to figure out the action doc id and create/remove a doc with that
name in the before/after hooks. But figuring out the id without dirt may be
tricky -- expose sequence() in export_test, and subtract 1? Could work, but
still depends on knowing _id structure; don't really love either.)

[johnweldon4, 2014-05-21 13:39:16]

Addressed the few changes needed.  Couple questions I'll ask about in #juju-dev

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

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1325
state/unit.go:1325: if notDead, err := isNotDead(u.st.units, u.doc.Name); err !=
nil {
On 2014/05/21 07:39:48, fwereade wrote:
> This one is interesting -- it's a not-strictly-necessary DB hit before running
> the txn, and I'd ordinarily shy away from that in favour of checking
u.doc.Life
> -- but let's see how it goes. I suspect we'll need to go with the other
approach
> in the end, as we start to care about things like charm versions, but we can
> wait and see if I'm right about that.

How is this different than doing a clone of the unit as you initially suggested.
 I went this route because it seemed more direct and I thought the db hit would
be comparable, if not slightly lighter than a full clone?

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1331
state/unit.go:1331: sel := bson.D{{"_id", newActionID}}
On 2014/05/21 07:39:48, fwereade wrote:
> This block is almost redundant, because we should be able to depend on the
> uniqueness of sequence IDs (even in the case of a new unit being created with
> the same name as an old one (which is itself a bug (although it only happens
> when a service is destroyed and recreated))).
> 
> If it didn't involve an extra DB hit I'd have no problem with it; but as it is
> I'm gently leaning towards dropping it. If there's a problem here we'll fall
out
> to ErrExcessiveContention anyway, and it should actually be pretty easy to
> diagnose from there.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1340
state/unit.go:1340: ops := []txn.Op{{
On 2014/05/21 07:39:48, fwereade wrote:
> As discussed yesterday, in this particular case the ops don't have to be
rebuilt
> each time, but it's probably good to keep the familiar structure.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit.go#newcode1351
state/unit.go:1351: return newActionID, err
On 2014/05/21 07:39:48, fwereade wrote:
> returning the id is not "normal" for this package, but IMO it's a good idea.
> thanks.

Done.

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

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode...
state/unit_test.go:1304: // verify action Id() is of expected form (unit id
prefix, + sequence)
On 2014/05/21 07:39:48, fwereade wrote:
> Add a note that this is temporary, and we shouldn't really leak the actual
_id.

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode...
state/unit_test.go:1335: unitDead := state.TransactionHook{
On 2014/05/21 07:39:48, fwereade wrote:
> s/unitDead/killUnit/ perhaps?

Done.

https://codereview.appspot.com/98260043/diff/40001/state/unit_test.go#newcode...
state/unit_test.go:1348: //TODO(jcw4): need to figure out how to inject
'contention' into the transaction
On 2014/05/21 07:39:48, fwereade wrote:
> Haha. I've thought of a way, and I kinda endorse you doing it, so long as you
> clearly comment that it'll stop working when lp:1174610 is fixed. In the
Before
> hook, you get st.Unit(name) and make it Dead; in the After hook, you remove
the
> unit and the service, and create a new service with the same name. From the
POV
> of that method the unit will appear to be resurrected.
> 
> But that's also dirty as sin, and will break when we fix other stuff, so I'm
> reasonably comfortable skipping it. (The other possibility, which is *maybe*
> cleaner, is to figure out the action doc id and create/remove a doc with that
> name in the before/after hooks. But figuring out the id without dirt may be
> tricky -- expose sequence() in export_test, and subtract 1? Could work, but
> still depends on knowing _id structure; don't really love either.)

1. 'dirty as sin', and
2. lots of work

Hmm, since you gave me an out I'd be happy to skip :)

[johnweldon4, 2014-05-21 13:52:56]

Please take a look.

[gz, 2014-05-21 20:35:25]

LGTM.

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

https://codereview.appspot.com/98260043/diff/60001/state/action.go#newcode8
state/action.go:8: Payload map[string]interface{}
I think I'd comment these members. Clarifies thinks like what defines the action
name, and what determines the payload type.

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

https://codereview.appspot.com/98260043/diff/60001/state/unit_test.go#newcode...
state/unit_test.go:1328: c.Assert(err, gc.NotNil)
Probably want to ErrorMatches this even though it's from below the AddAction
function.

https://codereview.appspot.com/98260043/diff/60001/state/unit_test.go#newcode...
state/unit_test.go:1349: //TODO(jcw4): need to figure out how to inject
'contention' into the transaction
This is where we really want a generic contention helper so each add doesn't
have to futz around with testing its loop. I think William has hopes for that at
some point later.

[johnweldon4, 2014-05-21 21:02:08]

Okay proposing again.

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

https://codereview.appspot.com/98260043/diff/60001/state/action.go#newcode8
state/action.go:8: Payload map[string]interface{}
On 2014/05/21 20:35:25, gz wrote:
> I think I'd comment these members. Clarifies thinks like what defines the
action
> name, and what determines the payload type.

Done.

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

https://codereview.appspot.com/98260043/diff/60001/state/unit_test.go#newcode...
state/unit_test.go:1328: c.Assert(err, gc.NotNil)
On 2014/05/21 20:35:25, gz wrote:
> Probably want to ErrorMatches this even though it's from below the AddAction
> function.

Done.

https://codereview.appspot.com/98260043/diff/60001/state/unit_test.go#newcode...
state/unit_test.go:1349: //TODO(jcw4): need to figure out how to inject
'contention' into the transaction
On 2014/05/21 20:35:25, gz wrote:
> This is where we really want a generic contention helper so each add doesn't
> have to futz around with testing its loop. I think William has hopes for that
at
> some point later.

Yep

[johnweldon4, 2014-05-21 21:04:24]

Please take a look.

[johnweldon4, 2014-05-22 06:14:06]

Please take a look.

[fwereade, 2014-05-22 08:02:14]

LGTM with final trivials. Sorry :(.

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

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode6
state/action.go:6: Id string `bson:"_id"`
blank lines before comments please

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode9
state/action.go:9: // action to actually perform
Name identifies the action; it should match an action defined by the unit's
charm.

?

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode12
state/action.go:12: // when this action is being performed.
Payload holds the action's parameters, if any; it should validate against the
schema defined by the named action in the unit's charm.

?

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

https://codereview.appspot.com/98260043/diff/100001/state/unit.go#newcode1344
state/unit.go:1344: return newActionID, err
ah, dammit. We shouldn't really return newActionID if err != nil. `switch err :=
...; err {`?

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

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcod...
state/unit_test.go:1295: func (s *UnitSuite) TestAddAction(c *gc.C) {
hey, can we move these to a new suite in action_test.go? unit_test is already
too damn big IMO.

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcod...
state/unit_test.go:1311: c.Assert(action.Id(), gc.Matches,
"^u#"+s.unit.Name()+"#\\d+")
how about an assertSaneActionId? You can use it below where you're currently
discarding the new ids.

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcod...
state/unit_test.go:1318: func (s *UnitSuite) TestAddActionFailsOnDeadUnit(c
*gc.C) {
TestAddActionLifecycle?

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcod...
state/unit_test.go:1357: func (s *UnitSuite)
TestAddActionFailsOnExcessiveContention(c *gc.C) {
I'd just drop this.

[johnweldon4, 2014-05-22 17:24:29]

Okay... addressed all
Running the tests and then I'll lbox propose

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

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode6
state/action.go:6: Id string `bson:"_id"`
On 2014/05/22 08:02:13, fwereade wrote:
> blank lines before comments please

Done.

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode9
state/action.go:9: // action to actually perform
On 2014/05/22 08:02:13, fwereade wrote:
> Name identifies the action; it should match an action defined by the unit's
> charm.
> 
> ?

Done.

https://codereview.appspot.com/98260043/diff/100001/state/action.go#newcode12
state/action.go:12: // when this action is being performed.
On 2014/05/22 08:02:13, fwereade wrote:
> Payload holds the action's parameters, if any; it should validate against the
> schema defined by the named action in the unit's charm.
> 
> ?

Done.

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

https://codereview.appspot.com/98260043/diff/100001/state/unit.go#newcode1344
state/unit.go:1344: return newActionID, err
On 2014/05/22 08:02:13, fwereade wrote:
> ah, dammit. We shouldn't really return newActionID if err != nil. `switch err
:=
> ...; err {`?

Done.

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

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcod...
state/unit_test.go:1295: func (s *UnitSuite) TestAddAction(c *gc.C) {
On 2014/05/22 08:02:14, fwereade wrote:
> hey, can we move these to a new suite in action_test.go? unit_test is already
> too damn big IMO.

Done.

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcod...
state/unit_test.go:1311: c.Assert(action.Id(), gc.Matches,
"^u#"+s.unit.Name()+"#\\d+")
On 2014/05/22 08:02:14, fwereade wrote:
> how about an assertSaneActionId? You can use it below where you're currently
> discarding the new ids.

Done. but only applies to the places where we don't expect an error...

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcod...
state/unit_test.go:1318: func (s *UnitSuite) TestAddActionFailsOnDeadUnit(c
*gc.C) {
On 2014/05/22 08:02:13, fwereade wrote:
> TestAddActionLifecycle?

Done.

https://codereview.appspot.com/98260043/diff/100001/state/unit_test.go#newcod...
state/unit_test.go:1357: func (s *UnitSuite)
TestAddActionFailsOnExcessiveContention(c *gc.C) {
On 2014/05/22 08:02:14, fwereade wrote:
> I'd just drop this.

Done.

[johnweldon4, 2014-05-22 17:27:30]

Please take a look.

[fwereade, 2014-05-23 13:50:33]

LGTM, approving