cmd/juju: add remove-unit subcommand

cmd/juju: add remove-unit subcommand



https://code.launchpad.net/~dave-cheney/juju-core/018-cmd-juju-remove-units/+merge/129341

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-10-12 04:05:58]

Please take a look.

[fwereade, 2012-10-15 05:51:35]

Little bit more work to do here, I think:

(Also looking forward to the followup with --force :))

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit.go
File cmd/juju/removeunit.go (right):

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:15: ServiceName string
Unused?

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:20: return &cmd.Info{"remove-unit", "", "remvoes service
units", ""}
* s/remvoes/removes/
* please mention the args

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit_test.go
File cmd/juju/removeunit_test.go (right):

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit_test.go#...
cmd/juju/removeunit_test.go:24: func (s *RemoveUnitSuite) TestRemoveUnit(c *C) {
A bit more detail here would be good... at least check the units are Dying, or
something; and check what happens when you try to remove a unit which is already
dying, or dead, or even removed. Is that an error? Should it be? etc etc :).

[dave_cheney.net, 2012-10-15 06:09:24]

Please take a look.

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit.go
File cmd/juju/removeunit.go (right):

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:15: ServiceName string
On 2012/10/15 05:51:35, fwereade wrote:
> Unused?

Done.

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:20: return &cmd.Info{"remove-unit", "", "remvoes service
units", ""}
On 2012/10/15 05:51:35, fwereade wrote:
> * s/remvoes/removes/
> * please mention the args

Done.

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit_test.go
File cmd/juju/removeunit_test.go (right):

https://codereview.appspot.com/6651060/diff/2001/cmd/juju/removeunit_test.go#...
cmd/juju/removeunit_test.go:24: func (s *RemoveUnitSuite) TestRemoveUnit(c *C) {
On 2012/10/15 05:51:35, fwereade wrote:
> A bit more detail here would be good... at least check the units are Dying, or
> something; and check what happens when you try to remove a unit which is
already
> dying, or dead, or even removed. Is that an error? Should it be? etc etc :).

Done.

[niemeyer, 2012-10-15 12:59:06]

LGTM given the following, but please wait for William.

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go
File cmd/juju/removeunit.go (right):

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:12: // RemoveUnitCommand is responsible adding additional
units to a service.
for removing units from

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:19: return &cmd.Info{"remove-unit", "", "removes service
units (service/0, service/1, etc)", ""}
"removes the provided service units"

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:27: args = f.Args()
c.UnitNames = f.Args()
if len(c.UnitNames) == 0 {

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:31: c.UnitNames = f.Args()
d

plus:

for _, name := range c.UnitNames {
        if !state.IsUnitName(name) {
                return fmt.Errorf("invalid service unit name: %q", name)
        }
}

Please include a simple test using an invalid unit name to trigger it.

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit_test.go
File cmd/juju/removeunit_test.go (right):

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit_test.go#...
cmd/juju/removeunit_test.go:38: // TODO(dfc) removing a removed unit
(EnsureDying) is not an error.
// Removing a unit that is dying is not an error.

[rog, 2012-10-15 13:01:49]

LGTM

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go
File cmd/juju/removeunit.go (right):

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:12: // RemoveUnitCommand is responsible adding additional
units to a service.
erm, removing units from a service?

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:51: return conn.RemoveUnits(units...)
Just a thought: i'm not sure that Conn.RemoveUnits has the right name - it
doesn't remove units, it just marks them as dying. This may be confusing, as we
do actually also have a RemoveUnit entry point in state. I think I'd tend
towards deleting the RemoveUnits method from Conn and using EnsureDying
directly, but YMMV.

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit_test.go
File cmd/juju/removeunit_test.go (right):

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit_test.go#...
cmd/juju/removeunit_test.go:34: // can't remove a non existant unit.
s/existant/existent/

[fwereade, 2012-10-15 13:37:49]

LGTM assuming niemeyer's suggestions, with the exception of:

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go
File cmd/juju/removeunit.go (right):

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:19: return &cmd.Info{"remove-unit", "", "removes service
units (service/0, service/1, etc)", ""}
On 2012/10/15 12:59:06, niemeyer wrote:
> "removes the provided service units"

I was thinking something like: 

&cmd.Info{"remove-unit", "<unit> [...]", "remove service units", ""}

("remove service units" seems better to me because that's the short description
of the command -- it will appear by the command name sans args.)

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:51: return conn.RemoveUnits(units...)
On 2012/10/15 13:01:49, rog wrote:
> Just a thought: i'm not sure that Conn.RemoveUnits has the right name - it
> doesn't remove units, it just marks them as dying. This may be confusing, as
we
> do actually also have a RemoveUnit entry point in state. I think I'd tend
> towards deleting the RemoveUnits method from Conn and using EnsureDying
> directly, but YMMV.

+1 to reworking this in a separate CL.

[niemeyer, 2012-10-15 13:42:06]

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go
File cmd/juju/removeunit.go (right):

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:19: return &cmd.Info{"remove-unit", "", "removes service
units (service/0, service/1, etc)", ""}
On 2012/10/15 13:37:49, fwereade wrote:
> On 2012/10/15 12:59:06, niemeyer wrote:
> > "removes the provided service units"
> 
> I was thinking something like: 
> 
> &cmd.Info{"remove-unit", "<unit> [...]", "remove service units", ""}
> 
> ("remove service units" seems better to me because that's the short
description
> of the command -- it will appear by the command name sans args.)

SGTM

[dave_cheney.net, 2012-10-15 23:11:41]

*** Submitted:

cmd/juju: add remove-unit subcommand

R=fwereade, niemeyer, rog
CC=
https://codereview.appspot.com/6651060

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go
File cmd/juju/removeunit.go (right):

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:12: // RemoveUnitCommand is responsible adding additional
units to a service.
On 2012/10/15 12:59:06, niemeyer wrote:
> for removing units from

Done.

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:12: // RemoveUnitCommand is responsible adding additional
units to a service.
On 2012/10/15 13:01:49, rog wrote:
> erm, removing units from a service?

Done.

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:19: return &cmd.Info{"remove-unit", "", "removes service
units (service/0, service/1, etc)", ""}
On 2012/10/15 13:37:49, fwereade wrote:
> On 2012/10/15 12:59:06, niemeyer wrote:
> > "removes the provided service units"
> 
> I was thinking something like: 
> 
> &cmd.Info{"remove-unit", "<unit> [...]", "remove service units", ""}
> 
> ("remove service units" seems better to me because that's the short
description
> of the command -- it will appear by the command name sans args.)

Done.

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:27: args = f.Args()
On 2012/10/15 12:59:06, niemeyer wrote:
> c.UnitNames = f.Args()
> if len(c.UnitNames) == 0 {

Done.

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:51: return conn.RemoveUnits(units...)
On 2012/10/15 13:01:49, rog wrote:
> Just a thought: i'm not sure that Conn.RemoveUnits has the right name - it
> doesn't remove units, it just marks them as dying. This may be confusing, as
we
> do actually also have a RemoveUnit entry point in state. I think I'd tend
> towards deleting the RemoveUnits method from Conn and using EnsureDying
> directly, but YMMV.

Done.

https://codereview.appspot.com/6651060/diff/8001/cmd/juju/removeunit.go#newco...
cmd/juju/removeunit.go:51: return conn.RemoveUnits(units...)
On 2012/10/15 13:37:49, fwereade wrote:
> On 2012/10/15 13:01:49, rog wrote:
> > Just a thought: i'm not sure that Conn.RemoveUnits has the right name - it
> > doesn't remove units, it just marks them as dying. This may be confusing, as
> we
> > do actually also have a RemoveUnit entry point in state. I think I'd tend
> > towards deleting the RemoveUnits method from Conn and using EnsureDying
> > directly, but YMMV.
> 
> +1 to reworking this in a separate CL.

Good point, will do in followup.