state: set departed on dying units' relations

state: set departed on dying units' relations

This ended up including:

  * stopping using magic strings in cleanup
  * drawing a clear distinction between "joined" and "in scope" (which
    accounts for most of the lines of changes)

...but did *not* include:

  * fixing the name of the Uniter.JoinedRelations API, because I want to
    wait for API versioning

https://code.launchpad.net/~fwereade/juju-core/use-prepare-leave-scope/+merge/219758

Requires: https://code.launchpad.net/~fwereade/juju-core/prepare-leave-scope/+merge/181065

(do not edit description out of merge proposal)

[johnweldon4, 2014-05-16 03:59:26]

https://codereview.appspot.com/94540043/diff/1/state/cleanup.go
File state/cleanup.go (right):

https://codereview.appspot.com/94540043/diff/1/state/cleanup.go#newcode59
state/cleanup.go:59: err = st.cleanupDyingUnit(doc.Prefix)
should the parameter to cleanupDyingUnit be doc.Prefix or a new field doc.Name? 
Is it sane to assume that doc.Prefix will work in st.Unit(name) as the name?

[fwereade, 2014-05-19 14:32:35]

On 2014/05/16 03:59:26, johnweldon4 wrote:
> https://codereview.appspot.com/94540043/diff/1/state/cleanup.go
> File state/cleanup.go (right):
> 
> https://codereview.appspot.com/94540043/diff/1/state/cleanup.go#newcode59
> state/cleanup.go:59: err = st.cleanupDyingUnit(doc.Prefix)
> should the parameter to cleanupDyingUnit be doc.Prefix or a new field
doc.Name? 
> Is it sane to assume that doc.Prefix will work in st.Unit(name) as the name?

Heh. It's called prefix because that was what we originall called it; it's
really just "the data that makes sense in the form of a single string". Yuck,
indeed.

[fwereade, 2014-05-19 14:39:02]

Please take a look.

[fwereade, 2014-05-19 15:01:46]

Please take a look.

[fwereade, 2014-05-19 16:14:42]

This needs better tests, FWIW: I should explicitly exercise the cases where (eg)
the unit leaves scope before this gets run, the unit gets removed before this
gets run, the relation gets removed, etc.

But I'd still like reviews of what's there.

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

https://codereview.appspot.com/94540043/diff/40001/state/cleanup.go#newcode171
state/cleanup.go:171: return nil
oops, that should be a `continue`

[fwereade, 2014-05-20 22:02:40]

Please take a look.

[fwereade, 2014-05-21 08:58:11]

Please take a look.

[gz, 2014-05-21 20:18:45]

LGTM. Does this still want more tests past those added in the prereq?

https://codereview.appspot.com/94540043/diff/80001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/94540043/diff/80001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:711: // on making that change until we have API
versioning.
Nit, I'd generally avoid first person in doc comment things, versus random TODO
type comments where it's fine. Could be "Rename to RelationsInScope when API
versioning is introduced and makes it safe to do so." or similar.

https://codereview.appspot.com/94540043/diff/80001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/94540043/diff/80001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter_test.go:1058: check := func() {
I'd put the args and expected result outside the closure.

[fwereade, 2014-05-22 09:16:46]

I think I actually have sufficient coverage with the tests that are there; the
cleanup tests hit both the happiest and the saddest path, and the pre-existing
tests cover PrepareLeaveScope itself.

https://codereview.appspot.com/94540043/diff/80001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/94540043/diff/80001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter.go:711: // on making that change until we have API
versioning.
On 2014/05/21 20:18:45, gz wrote:
> Nit, I'd generally avoid first person in doc comment things, versus random
TODO
> type comments where it's fine. Could be "Rename to RelationsInScope when API
> versioning is introduced and makes it safe to do so." or similar.

Done.

https://codereview.appspot.com/94540043/diff/80001/state/apiserver/uniter/uni...
File state/apiserver/uniter/uniter_test.go (right):

https://codereview.appspot.com/94540043/diff/80001/state/apiserver/uniter/uni...
state/apiserver/uniter/uniter_test.go:1058: check := func() {
On 2014/05/21 20:18:45, gz wrote:
> I'd put the args and expected result outside the closure.

Done.