cmd/juju: destroy-unit is more restrictive

cmd/juju: destroy-unit is more restrictive

It now only accepts Alive (because allowing only a single destroy per entity
is already conventional) principal (because subordinate lifecycle is
controlled by the principal) units.

https://code.launchpad.net/~fwereade/juju-core/destroy-unit-handle-subs/+merge/142478

(do not edit description out of merge proposal)

[fwereade, 2013-01-09 11:05:13]

Please take a look.

[TheMue, 2013-01-09 11:18:08]

LGTM

[dimitern, 2013-01-09 14:54:02]

LGTM

[rog, 2013-01-09 14:59:23]

the behaviour seems good, but one thought below.

https://codereview.appspot.com/7058061/diff/1/cmd/juju/destroyunit.go
File cmd/juju/destroyunit.go (right):

https://codereview.appspot.com/7058061/diff/1/cmd/juju/destroyunit.go#newcode52
cmd/juju/destroyunit.go:52: if unit, err := conn.State.Unit(name);
state.IsNotFound(err) {
i wonder if this logic should be in conn.DestroyUnits.

https://codereview.appspot.com/7058061/diff/1/cmd/juju/destroyunit_test.go
File cmd/juju/destroyunit_test.go (right):

https://codereview.appspot.com/7058061/diff/1/cmd/juju/destroyunit_test.go#ne...
cmd/juju/destroyunit_test.go:31: func (s *DestroyUnitSuite)
TestDestroyPrincipalUnits(c *C) {
these tests feel like they belong in the juju package and the original tests
should be sufficient here.

[fwereade, 2013-01-09 15:02:30]

Thanks guys; rog, I think you're right. Assuming a clean and simple move of the
tests, I'll be submitting directly when it's done, unless you object.

[fwereade, 2013-01-10 10:50:06]

Please take a look.

[rog, 2013-01-10 10:56:05]

LGTM with a couple of trivial suggestions.

https://codereview.appspot.com/7058061/diff/6001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7058061/diff/6001/juju/conn.go#newcode264
juju/conn.go:264: if unit, err := conn.State.Unit(name); state.IsNotFound(err) {
this may well look better with a switch.

unit, err := conn.State.Unit(name)
switch {
case state.IsNotFound(err):
      return fmt.Errorf("unit %q is not alive", name)
case err != nil:
   etc
}

https://codereview.appspot.com/7058061/diff/6001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/7058061/diff/6001/juju/conn_test.go#newcode512
juju/conn_test.go:512: }
add newline

[fwereade, 2013-01-10 16:06:48]

*** Submitted:

cmd/juju: destroy-unit is more restrictive

It now only accepts Alive (because allowing only a single destroy per entity
is already conventional) principal (because subordinate lifecycle is
controlled by the principal) units.

R=TheMue, dimitern, rog
CC=
https://codereview.appspot.com/7058061

https://codereview.appspot.com/7058061/diff/6001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7058061/diff/6001/juju/conn.go#newcode264
juju/conn.go:264: if unit, err := conn.State.Unit(name); state.IsNotFound(err) {
On 2013/01/10 10:56:05, rog wrote:
> this may well look better with a switch.
> 
> unit, err := conn.State.Unit(name)
> switch {
> case state.IsNotFound(err):
>       return fmt.Errorf("unit %q is not alive", name)
> case err != nil:
>    etc
> }

Done.

https://codereview.appspot.com/7058061/diff/6001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/7058061/diff/6001/juju/conn_test.go#newcode512
juju/conn_test.go:512: }
On 2013/01/10 10:56:05, rog wrote:
> add newline

Done.