Code review comment for lp:~themue/juju-core/go-state-lifecycle-relation

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go
File state/lifecycle.go (right):

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go#newcode45
state/lifecycle.go:45: // init initializes the lifecycle with the state
Alive .
Why do we need this? We should be creating the actual nodes with life
set to Alive in the first place, rather than changing them to Alive
after the fact via this method.

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go#newcode88
state/lifecycle.go:88: // Attention! The config map is defined by the
ConfigNode.
As we debated ad infinitum, there are other configuration nodes that we
manipulate without ConfigNode, so this isn't really a relevant note.

What might be more useful is:

// Note that there are unknown keys in the node.

https://codereview.appspot.com/6455065/diff/17001/state/lifecycle.go#newcode98
state/lifecycle.go:98: if !validTransitions[current][next] {
Please add a test verifying that SetLife(Dying) when the current state
is Dead works and does nothing. This was mentioned in the very first
review I made and still seems unhandled.

https://codereview.appspot.com/6455065/diff/17001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6455065/diff/17001/state/service.go#newcode212
state/service.go:212: if err := r.Refresh(); err != nil {
This workflow doesn't make sense:

1) r := newRelation
2) r.Refresh
3) r.endpoint = ...

If we're going to be adding Refresh now, the sites that call it should
be sane, otherwise we're corrupting the whole logic in a way that we
won't be able to get out of in the near future. Please ping me online
for us to sort this out.

https://codereview.appspot.com/6455065/diff/17001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6455065/diff/17001/state/state.go#newcode348
state/state.go:348: err = newRelation(s, relationKey).lifecycle.init()
Why writing a second time to the node just for setting life=alive?

https://codereview.appspot.com/6455065/diff/17001/state/state.go#newcode401
state/state.go:401: if err = r.Refresh(); err != nil {
Same case as before.

https://codereview.appspot.com/6455065/