cmd/juju: add add-unit command

cmd/juju: add add-unit command

add-unit adds units.

https://code.launchpad.net/~dave-cheney/juju-core/go-cmd-juju-add-unit/+merge/118280

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-08-06 04:46:44]

Please take a look.

[fwereade, 2012-08-06 06:46:37]

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.

[dave_cheney.net, 2012-08-06 07:04:16]

Hyphenated file names are very un Go like. 

On 06/08/2012, at 16:46, fwereade@gmail.com wrote:

> 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/

[dave_cheney.net, 2012-08-06 09:16:36]

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 ?

[dave_cheney.net, 2012-08-07 00:47:21]

https://codereview.appspot.com/6453090/

[rog, 2012-08-07 08:36:17]

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.

[rog, 2012-08-07 08:36:32]

LGTM BTW.

[niemeyer, 2012-08-07 10:48:31]

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.

[dave_cheney.net, 2012-08-08 01:51:29]

*** Submitted:

cmd/juju: add add-unit command

add-unit adds units.

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

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
I respectfully disagree. There is already a precedent for not doing this,
destroy.go.

If you would like to propose a followup branch that renames this file please do
so.

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.")
On 2012/08/07 10:48:31, niemeyer wrote:
> s/Number/number/
> s/\.//
> 
> Also, I believe we have a -n shorthand for this.

Done.

https://codereview.appspot.com/6449093/diff/4001/cmd/juju/addunit.go#newcode51
cmd/juju/addunit.go:51: st, err := conn.State()
On 2012/08/07 10:48:31, niemeyer wrote:
> 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.

Done.