cmd/juju: destroy-machine

cmd/juju: destroy-machine

This CL includes some rationalizations of existing code: machine lifecycle
advancement has been simplified, and its error messages have been improved,
to allow destroy-machine and destroy-unit to act consistently with one
another and in a manner helpful to the user.

https://code.launchpad.net/~fwereade/juju-core/cli-destroy-machine/+merge/145787

(do not edit description out of merge proposal)

[fwereade, 2013-01-31 13:11:17]

Please take a look.

[TheMue, 2013-01-31 17:49:57]

LGTM, only two small remarks.

https://codereview.appspot.com/7227063/diff/2001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/7227063/diff/2001/juju/conn_test.go#newcode398
juju/conn_test.go:398: // Create 4 principal units.
5 units.

https://codereview.appspot.com/7227063/diff/2001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/7227063/diff/2001/state/machine.go#newcode174
state/machine.go:174: func (m0 *Machine) advanceLifecycle(life Life) (err error)
{
Would like a more speaking identifier than m0 here, even if it is unusual long
then.

[jameinel, 2013-02-04 06:54:07]

Did this get landed, or are you still seeking a second review? The patch is big
enough I was going to punt if it already landed.

[fwereade, 2013-02-04 15:07:49]

On 2013/02/04 06:54:07, jameinel wrote:
> Did this get landed, or are you still seeking a second review? The patch is
big
> enough I was going to punt if it already landed.

Not yet, I'm still waiting. I think/hope it's less nasty than it looks...

[dimitern, 2013-02-04 16:41:53]

LGTM, a few comments only.

https://codereview.appspot.com/7227063/diff/2001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7227063/diff/2001/juju/conn.go#newcode226
juju/conn.go:226: // TODO lp:1101139
Maybe a bit more info:
// TODO launchpad bug 1101139: "units are not assigned transactionally"

https://codereview.appspot.com/7227063/diff/2001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/7227063/diff/2001/juju/conn_test.go#newcode398
juju/conn_test.go:398: // Create 4 principal units.
On 2013/01/31 17:49:57, TheMue wrote:
> 5 units.

+1

https://codereview.appspot.com/7227063/diff/2001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/7227063/diff/2001/state/machine.go#newcode174
state/machine.go:174: func (m0 *Machine) advanceLifecycle(life Life) (err error)
{
On 2013/01/31 17:49:57, TheMue wrote:
> Would like a more speaking identifier than m0 here, even if it is unusual long
> then.

...or at least a short comment why the need to copy m?

[fwereade, 2013-02-07 15:00:29]

*** Submitted:

cmd/juju: destroy-machine

This CL includes some rationalizations of existing code: machine lifecycle
advancement has been simplified, and its error messages have been improved,
to allow destroy-machine and destroy-unit to act consistently with one
another and in a manner helpful to the user.

R=
CC=
https://codereview.appspot.com/7227063

https://codereview.appspot.com/7227063/diff/2001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7227063/diff/2001/juju/conn.go#newcode226
juju/conn.go:226: // TODO lp:1101139
On 2013/02/04 16:41:54, dimitern wrote:
> Maybe a bit more info:
> // TODO launchpad bug 1101139: "units are not assigned transactionally"

Done.

https://codereview.appspot.com/7227063/diff/2001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/7227063/diff/2001/juju/conn_test.go#newcode398
juju/conn_test.go:398: // Create 4 principal units.
On 2013/02/04 16:41:54, dimitern wrote:
> On 2013/01/31 17:49:57, TheMue wrote:
> > 5 units.
> 
> +1

Done.

https://codereview.appspot.com/7227063/diff/2001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/7227063/diff/2001/state/machine.go#newcode174
state/machine.go:174: func (m0 *Machine) advanceLifecycle(life Life) (err error)
{
On 2013/02/04 16:41:54, dimitern wrote:
> On 2013/01/31 17:49:57, TheMue wrote:
> > Would like a more speaking identifier than m0 here, even if it is unusual
long
> > then.
> 
> ...or at least a short comment why the need to copy m?

Renamed to "original"; explained at Refresh() time, below.