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

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.

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