cleanup code to remove pending Actions

cleanup code to remove pending Actions

remove pending Actions from the queue when the units to which they
are assigned are removed.

https://code.launchpad.net/~johnweldon4/juju-core/unit-action-cleanup/+merge/221007

(do not edit description out of merge proposal)

[johnweldon4, 2014-05-26 22:26:03]

Please take a look.

[fwereade, 2014-05-26 23:44:49]

This is great, just a bit light on the testing.

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

https://codereview.appspot.com/92630043/diff/20001/state/action.go#newcode76
state/action.go:76: // TODO(jcw4) add logging
It's not so much about the logging as it is about the creation of a result in
the DB explaining why it fails. I'd add a log message *now* and a TODO to drop
it when we can record what happened in a more useful way.

https://codereview.appspot.com/92630043/diff/20001/state/action_test.go
File state/action_test.go (right):

https://codereview.appspot.com/92630043/diff/20001/state/action_test.go#newco...
state/action_test.go:84: c.Assert(len(actions), gc.Equals, 0)
I think this test would work better standing alone. Add a couple of actions;
destroy the unit, run cleanups, check they're still there; make dead, check no
cleanups; remove, check they're still there; run cleanups, check they're gone.
And that's plenty for a single test method :).

(many would say that even what I suggest there is testing too much at a time,
and they have a point; I will certainly not complain if you write more, and
smaller, tests :))

You can put it here, or in cleanup_test.go; I have a slight preference for the
latter, but nothing overwhelming. Picking the right place to test cross-cutting
issues is hard.

https://codereview.appspot.com/92630043/diff/20001/state/action_test.go#newco...
state/action_test.go:109: c.Assert(actionId, gc.Matches,
"^u#"+unitName+"#a#\\d+")
Much nicer than before. Thanks.

https://codereview.appspot.com/92630043/diff/20001/state/cleanup.go
File state/cleanup.go (right):

https://codereview.appspot.com/92630043/diff/20001/state/cleanup.go#newcode192
state/cleanup.go:192: // the unit Name
If we drop Unit.Actions, we can drop this comment too, because UnitActions
becomes the One True Way :).

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

https://codereview.appspot.com/92630043/diff/20001/state/state.go#newcode1412
state/state.go:1412: func (st *State) UnitActions(name string) ([]*Action,
error) {
missing tests

https://codereview.appspot.com/92630043/diff/20001/state/state.go#newcode1414
state/state.go:1414: sel := bson.D{{"_id", bson.D{{"$regex", "^" +
actionPrefix(unitGlobalKey(name))}}}}
fwiw, I'd pull the actionPrefix(...) stuff out into a var. ymmv

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

https://codereview.appspot.com/92630043/diff/20001/state/unit.go#newcode1371
state/unit.go:1371: func (u *Unit) Actions() ([]*Action, error) {
missing tests (but tbh I'd drop this one, it's not very orthogonal)

[johnweldon4, 2014-05-27 06:15:18]

Updated to address comments, added a test for State.UnitActions(), etc.

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

https://codereview.appspot.com/92630043/diff/20001/state/action.go#newcode76
state/action.go:76: // TODO(jcw4) add logging
On 2014/05/26 23:44:48, fwereade wrote:
> It's not so much about the logging as it is about the creation of a result in
> the DB explaining why it fails. I'd add a log message *now* and a TODO to drop
> it when we can record what happened in a more useful way.

Done.

https://codereview.appspot.com/92630043/diff/20001/state/action_test.go
File state/action_test.go (right):

https://codereview.appspot.com/92630043/diff/20001/state/action_test.go#newco...
state/action_test.go:84: c.Assert(len(actions), gc.Equals, 0)
On 2014/05/26 23:44:48, fwereade wrote:
> I think this test would work better standing alone. Add a couple of actions;
> destroy the unit, run cleanups, check they're still there; make dead, check no
> cleanups; remove, check they're still there; run cleanups, check they're gone.
> And that's plenty for a single test method :).
> 
> (many would say that even what I suggest there is testing too much at a time,
> and they have a point; I will certainly not complain if you write more, and
> smaller, tests :))
> 
> You can put it here, or in cleanup_test.go; I have a slight preference for the
> latter, but nothing overwhelming. Picking the right place to test
cross-cutting
> issues is hard.

Done.

https://codereview.appspot.com/92630043/diff/20001/state/action_test.go#newco...
state/action_test.go:109: c.Assert(actionId, gc.Matches,
"^u#"+unitName+"#a#\\d+")
On 2014/05/26 23:44:48, fwereade wrote:
> Much nicer than before. Thanks.

Done.

https://codereview.appspot.com/92630043/diff/20001/state/cleanup.go
File state/cleanup.go (right):

https://codereview.appspot.com/92630043/diff/20001/state/cleanup.go#newcode192
state/cleanup.go:192: // the unit Name
On 2014/05/26 23:44:48, fwereade wrote:
> If we drop Unit.Actions, we can drop this comment too, because UnitActions
> becomes the One True Way :).

Done.

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

https://codereview.appspot.com/92630043/diff/20001/state/state.go#newcode1412
state/state.go:1412: func (st *State) UnitActions(name string) ([]*Action,
error) {
On 2014/05/26 23:44:48, fwereade wrote:
> missing tests

Done.

https://codereview.appspot.com/92630043/diff/20001/state/state.go#newcode1414
state/state.go:1414: sel := bson.D{{"_id", bson.D{{"$regex", "^" +
actionPrefix(unitGlobalKey(name))}}}}
On 2014/05/26 23:44:48, fwereade wrote:
> fwiw, I'd pull the actionPrefix(...) stuff out into a var. ymmv

Done.

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

https://codereview.appspot.com/92630043/diff/20001/state/unit.go#newcode1371
state/unit.go:1371: func (u *Unit) Actions() ([]*Action, error) {
On 2014/05/26 23:44:49, fwereade wrote:
> missing tests (but tbh I'd drop this one, it's not very orthogonal)

Done.

[johnweldon4, 2014-05-27 06:22:33]

Please take a look.

[fwereade, 2014-05-27 06:59:55]

Great, really just trivials to do... and I just realised we want a test for
Fail, too. Checking the logs would be ideal (with a comment that it's temporary
and should be replaced with a check for a fail result); checking that
UnitActions no longer returns failed actions is definitely necessary.

https://codereview.appspot.com/92630043/diff/40001/state/cleanup_test.go
File state/cleanup_test.go (right):

https://codereview.appspot.com/92630043/diff/40001/state/cleanup_test.go#newc...
state/cleanup_test.go:320: _, err = unit.AddAction("action1", nil)
Hey, this makes me think of another test we should have: that we can add two
actions with the same name, and they don't get confused.

Or *possibly* that we can queue only one of a given action at a time. Maybe
that's more sensible? No action needed right now, but please track it as a
consideration and do a followup testing whichever you think is right. We can
chat about it next meeting if you like?

https://codereview.appspot.com/92630043/diff/40001/state/cleanup_test.go#newc...
state/cleanup_test.go:346: c.Assert(len(actions), gc.Equals, 0)
final check that no cleanups remain?

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

https://codereview.appspot.com/92630043/diff/40001/state/state.go#newcode1414
state/state.go:1414: actionId := actionPrefix(unitGlobalKey(name))
`prefix :=` perhaps?

[johnweldon4, 2014-05-27 17:53:18]

Please take a look.

https://codereview.appspot.com/92630043/diff/40001/state/cleanup_test.go
File state/cleanup_test.go (right):

https://codereview.appspot.com/92630043/diff/40001/state/cleanup_test.go#newc...
state/cleanup_test.go:320: _, err = unit.AddAction("action1", nil)
On 2014/05/27 06:59:54, fwereade wrote:
> Hey, this makes me think of another test we should have: that we can add two
> actions with the same name, and they don't get confused.
> 
> Or *possibly* that we can queue only one of a given action at a time. Maybe
> that's more sensible? No action needed right now, but please track it as a
> consideration and do a followup testing whichever you think is right. We can
> chat about it next meeting if you like?

Done.

https://codereview.appspot.com/92630043/diff/40001/state/cleanup_test.go#newc...
state/cleanup_test.go:346: c.Assert(len(actions), gc.Equals, 0)
On 2014/05/27 06:59:54, fwereade wrote:
> final check that no cleanups remain?

Done.

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

https://codereview.appspot.com/92630043/diff/40001/state/state.go#newcode1414
state/state.go:1414: actionId := actionPrefix(unitGlobalKey(name))
On 2014/05/27 06:59:54, fwereade wrote:
> `prefix :=` perhaps?

Done.

[fwereade, 2014-05-28 16:03:00]

LGTM, thanks