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

Revision history for this message
William Reade (fwereade) wrote :

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#newcode1295
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#newcode1311
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#newcode1318
state/unit_test.go:1318: func (s *UnitSuite)
TestAddActionFailsOnDeadUnit(c *gc.C) {
TestAddActionLifecycle?

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

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

« Back to merge proposal