Merge lp:~dave-cheney/juju-core/go-cmd-juju-add-unit into lp:~juju/juju-core/trunk

[Dave Cheney]

cmd/juju: add add-unit command

add-unit adds units.

https://codereview.appspot.com/6449093/

[William Reade (fwereade)]

Code looks fine, but:

https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go
File cmd/juju/addunit.go (right):

https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go#newcode1
cmd/juju/addunit.go:1: package main
Can we call this add-unit please? Non-matching names just bug me :).

https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go#newcode51
cmd/juju/addunit.go:51: st, err := conn.State()
I *think* that this is Doing It Wrong, and that Conn clients are not
meant to interact with state directly, but I may have the wrong end of
the stick.

https://codereview.appspot.com/6449093/

[Roger Peppe (rogpeppe)]

On 2012/08/06 09:16:36, dfc wrote:
> Thank you for your review.

> https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go
> File cmd/juju/addunit.go (right):

https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go#newcode51
> cmd/juju/addunit.go:51: st, err := conn.State()
> On 2012/08/06 06:46:37, fwereade wrote:
> > I *think* that this is Doing It Wrong, and that Conn clients are not
meant to
> > interact with state directly, but I may have the wrong end of the
stick.

> if conn.AddUnits took the service name as a string, we can avoid going
via the
> state. Should I raise a bug to address this ?

That could work. I'm still not sure where the correct line is between
juju.Conn and state.State. We're definitely not *hiding* the State - I'm
inclined to add operations to juju.Conn that make it trivial to
implement the juju command-line operations. AddUnits on the boundary
here, as it's trivial to implement on the State too.

https://codereview.appspot.com/6449093/

[Roger Peppe (rogpeppe)]

LGTM BTW.

https://codereview.appspot.com/6449093/

[Gustavo Niemeyer (niemeyer)]

LGTM with trivials:

https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go
File cmd/juju/addunit.go (right):

https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go#newcode1
cmd/juju/addunit.go:1: package main
On 2012/08/06 06:46:37, fwereade wrote:
> Can we call this add-unit please? Non-matching names just bug me :).

+1

https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go#newcode24
cmd/juju/addunit.go:24: f.IntVar(&c.NumUnits, "num-units", 1, "Number of
service units to add.")
s/Number/number/
s/\.//

Also, I believe we have a -n shorthand for this.

https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go#newcode51
cmd/juju/addunit.go:51: st, err := conn.State()
On 2012/08/06 09:16:38, dfc wrote:
> On 2012/08/06 06:46:37, fwereade wrote:
> > I *think* that this is Doing It Wrong, and that Conn clients are not
meant to
> > interact with state directly, but I may have the wrong end of the
stick.

> if conn.AddUnits took the service name as a string, we can avoid going
via the
> state. Should I raise a bug to address this ?

I think this is good as it is for now. Conn may be seen as a set of
high-level logic used by command lines, but IMO we don't need one-liner
wrappers there for functionality like the one seen here.

https://codereview.appspot.com/6449093/

=== added file 'cmd/juju/addunit.go'
--- cmd/juju/addunit.go	1970-01-01 00:00:00 +0000
+++ cmd/juju/addunit.go	2012-08-06 04:48:27 +0000
@@ -0,0 +1,62 @@
+package main
+
+import (
+	"errors"
+
+	"launchpad.net/gnuflag"
+	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/juju"
+)
+
+// AddUnitCommand is responsible adding additional units to a service.
+type AddUnitCommand struct {
+	EnvName     string
+	ServiceName string
+	NumUnits    int
+}
+
+func (c *AddUnitCommand) Info() *cmd.Info {
+	return &cmd.Info{"add-unit", "", "add a service unit", ""}
+}
+
+func (c *AddUnitCommand) Init(f *gnuflag.FlagSet, args []string) error {
+	addEnvironFlags(&c.EnvName, f)
+	f.IntVar(&c.NumUnits, "num-units", 1, "Number of service units to add.")
+	if err := f.Parse(true, args); err != nil {
+		return err
+	}
+	args = f.Args()
+	switch len(args) {
+	case 1:
+		c.ServiceName = args[0]
+	case 0:
+		return errors.New("no service specified")
+	default:
+		return cmd.CheckEmpty(args[1:])
+	}
+	if c.NumUnits < 1 {
+		return errors.New("must add at least one unit")
+	}
+	return nil
+}
+
+// Run connects to the environment specified on the command line 
+// and calls conn.AddUnits.
+func (c *AddUnitCommand) Run(_ *cmd.Context) error {
+	conn, err := juju.NewConn(c.EnvName)
+	if err != nil {
+		return err
+	}
+	defer conn.Close()
+	st, err := conn.State()
+	if err != nil {
+		return err
+	}
+	service, err := st.Service(c.ServiceName)
+	if err != nil {
+		return err
+	}
+	_, err = conn.AddUnits(service, c.NumUnits)
+	return err
+
+}

=== added file 'cmd/juju/addunit_test.go'
--- cmd/juju/addunit_test.go	1970-01-01 00:00:00 +0000
+++ cmd/juju/addunit_test.go	2012-08-06 04:48:27 +0000
@@ -0,0 +1,46 @@
+package main
+
+import (
+	"bytes"
+	. "launchpad.net/gocheck"
+	"launchpad.net/juju-core/charm"
+	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/testing"
+)
+
+type AddUnitSuite struct {
+	DeploySuite
+}
+
+var _ = Suite(&AddUnitSuite{})
+
+func (s *AddUnitSuite) SetUpTest(c *C) {
+	s.DeploySuite.SetUpTest(c)
+}
+
+func (s *AddUnitSuite) TearDownTest(c *C) {
+	s.DeploySuite.TearDownTest(c)
+}
+
+func runAddUnit(c *C, args ...string) error {
+	com := &AddUnitCommand{}
+	err := com.Init(newFlagSet(), args)
+	c.Assert(err, IsNil)
+	return com.Run(&cmd.Context{c.MkDir(), &bytes.Buffer{}, &bytes.Buffer{}})
+}
+
+func (s *AddUnitSuite) TestAddUnit(c *C) {
+	testing.Charms.BundlePath(s.seriesPath, "dummy")
+	err := runDeploy(c, "local:dummy", "some-service-name")
+	c.Assert(err, IsNil)
+	curl := charm.MustParseURL("local:precise/dummy-1")
+	s.assertService(c, "some-service-name", curl, 1, 0)
+
+	err = runAddUnit(c, "some-service-name")
+	c.Assert(err, IsNil)
+	s.assertService(c, "some-service-name", curl, 2, 0)
+
+	err = runAddUnit(c, "--num-units", "2", "some-service-name")
+	c.Assert(err, IsNil)
+	s.assertService(c, "some-service-name", curl, 4, 0)
+}

=== modified file 'cmd/juju/cmd_test.go'
--- cmd/juju/cmd_test.go	2012-07-30 16:51:00 +0000
+++ cmd/juju/cmd_test.go	2012-08-06 04:48:27 +0000
@@ -264,3 +264,20 @@
 
 	// environment tested elsewhere
 }
+
+func initAddUnitCommand(args ...string) (*AddUnitCommand, error) {
+	com := &AddUnitCommand{}
+	return com, com.Init(newFlagSet(), args)
+}
+
+func (*cmdSuite) TestAddUnitCommandInit(c *C) {
+	// missing args
+	_, err := initAddUnitCommand()
+	c.Assert(err, ErrorMatches, "no service specified")
+
+	// bad unit count
+	_, err = initAddUnitCommand("service-name", "--num-units", "0")
+	c.Assert(err, ErrorMatches, "must add at least one unit")
+
+	// environment tested elsewhere
+}

=== modified file 'cmd/juju/main.go'
--- cmd/juju/main.go	2012-07-18 00:33:07 +0000
+++ cmd/juju/main.go	2012-08-06 04:48:27 +0000
@@ -25,6 +25,7 @@
 // provides an entry point for testing with arbitrary command line arguments.
 func Main(args []string) {
 	juju := &cmd.SuperCommand{Name: "juju", Doc: jujuDoc, Log: &cmd.Log{}}
+	juju.Register(&AddUnitCommand{})
 	juju.Register(&BootstrapCommand{})
 	juju.Register(&DeployCommand{})
 	juju.Register(&DestroyCommand{})

=== modified file 'cmd/juju/main_test.go'
--- cmd/juju/main_test.go	2012-07-25 15:46:01 +0000
+++ cmd/juju/main_test.go	2012-08-06 04:48:27 +0000
@@ -138,6 +138,7 @@
 }
 
 var commandNames = []string{
+	"add-unit",
 	"bootstrap",
 	"deploy",
 	"destroy-environment",