add-unit supports the force-machine argument

add-unit supports the force-machine argument

Semantics are the same as for deploy. Only one unit
can be added with force-machine, or else an error
occurs.

https://code.launchpad.net/~wallyworld/juju-core/add-unit-force-machine/+merge/174101

Requires: https://code.launchpad.net/~wallyworld/juju-core/force-machine-containers/+merge/173836

(do not edit description out of merge proposal)

[wallyworld, 2013-07-11 03:28:37]

Please take a look.

[wallyworld, 2013-07-11 10:13:54]

Please take a look.

[dimitern, 2013-07-11 21:38:48]

LGTM with a few suggestions.

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

https://codereview.appspot.com/10962046/diff/4001/cmd/juju/addunit.go#newcode26
cmd/juju/addunit.go:26: f.StringVar(&c.ForceMachineSpec, "force-machine", "",
"Machine/container to deploy unit, bypasses constraints")
s/Machine/machine/

https://codereview.appspot.com/10962046/diff/4001/cmd/juju/addunit.go#newcode35
cmd/juju/addunit.go:35: return errors.New("cannot use --num-units with
--force-machine")
why not make NumUnits default to 0 and change it to 1 later, thus this error
will be even technically correct (--num-units=1 and --force-machine is still
possible)?

https://codereview.appspot.com/10962046/diff/4001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/10962046/diff/4001/juju/conn.go#newcode205
juju/conn.go:205: if args.NumUnits > 1 && args.ForceMachineSpec != "" {
...here as well (see above)

https://codereview.appspot.com/10962046/diff/4001/state/statecmd/addunit.go
File state/statecmd/addunit.go (right):

https://codereview.appspot.com/10962046/diff/4001/state/statecmd/addunit.go#n...
state/statecmd/addunit.go:29: return nil, errors.New("cannot use --num-units
with --force-machine")
and here

[wallyworld, 2013-07-12 03:48:13]

Please take a look.

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

https://codereview.appspot.com/10962046/diff/4001/cmd/juju/addunit.go#newcode26
cmd/juju/addunit.go:26: f.StringVar(&c.ForceMachineSpec, "force-machine", "",
"Machine/container to deploy unit, bypasses constraints")
On 2013/07/11 21:38:48, dimitern wrote:
> s/Machine/machine/

Done.

https://codereview.appspot.com/10962046/diff/4001/cmd/juju/addunit.go#newcode35
cmd/juju/addunit.go:35: return errors.New("cannot use --num-units with
--force-machine")
On 2013/07/11 21:38:48, dimitern wrote:
> why not make NumUnits default to 0 and change it to 1 later, thus this error
> will be even technically correct (--num-units=1 and --force-machine is still
> possible)?

I used the same semantics here as was done for the deploy command. I didn't
write the original code but I think defaulting to 1 is sensible, since this is
the most common use case.

https://codereview.appspot.com/10962046/diff/4001/state/statecmd/addunit.go
File state/statecmd/addunit.go (right):

https://codereview.appspot.com/10962046/diff/4001/state/statecmd/addunit.go#n...
state/statecmd/addunit.go:29: return nil, errors.New("cannot use --num-units
with --force-machine")
On 2013/07/11 21:38:48, dimitern wrote:
> and here

error message fixed

[jtv.canonical, 2013-07-15 10:27:20]

I'm not questioning the design because one, that's what pre-implementation calls
are for and two, I can't say I understand much of it!

But the code mostly LGTM, with a few bad habits that need changing.

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

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit.go#newcode26
cmd/juju/addunit.go:26: f.StringVar(&c.ForceMachineSpec, "force-machine", "",
"machine/container to deploy unit, bypasses constraints")

The help string for this one is a bit cryptic.  Can you make it any friendlier?

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go
File cmd/juju/addunit_test.go (right):

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go#n...
cmd/juju/addunit_test.go:41: c.Assert(err, ErrorMatches, t.err)

Might as well use c.Check here, right?  That way you get all the failures.

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go#n...
cmd/juju/addunit_test.go:71: func (s *AddUnitSuite) assertForceMachine(c *C, svc
*state.Service, unitNum int, machineId string) {

Could you document what this method is for?  Sooner or later people are going to
want changes, and then they'll have to know whether the changes jive with the
function's original intent, as implicitly assumed at the call sites.

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go#n...
cmd/juju/addunit_test.go:74: c.Assert(units, HasLen, 3)

Why 3?  Father, Son, Holy Ghost?  Where does this come from?  Document!

And please don't tell me that you made a secret, undocumented deal between this
method and its caller to assume that 3 is the right number here, for I shall
have to kick you in the nuts.  That kind of under-the-table dealing is a recipe
for unmaintainable code.

Think kindly of the poor engineer who is unfamiliar with this code and under
pressure to fix it, because it may be you after others have messed with it!

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go#n...
cmd/juju/addunit_test.go:87: err = runAddUnit(c, "some-service-name",
"--force-machine", machine2.Id())

It helps to have blank lines separating the setup / exercise / verification
portions of the test.  In this case the two runAddUnit calls and their non-error
assertions would be the "exercise" portions.

And by the way, why those two calls?  I am completely in the dark about this
test's meaning.

https://codereview.appspot.com/10962046/diff/12001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/10962046/diff/12001/juju/conn_test.go#newcode514
juju/conn_test.go:514: func (s *DeployLocalSuite)
TestDeployForceMachineTooManyUnits(c *C) {

A testing practice I have learned to great benefit (from Aaron, if memory
serves) was to make the name of a test state the fact or behaviour that the test
proves: "deploy with --force-machine rejects too many units."  As if by magic I
started writing more focused tests.  I heartily recommend it!

[wallyworld, 2013-07-16 01:15:57]

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

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit.go#newcode26
cmd/juju/addunit.go:26: f.StringVar(&c.ForceMachineSpec, "force-machine", "",
"machine/container to deploy unit, bypasses constraints")
On 2013/07/15 10:27:20, jtv.canonical wrote:
> 
> The help string for this one is a bit cryptic.  Can you make it any
friendlier?

This was copied from elsewhere. I'm not sure how to expand on it without making
it way too verbose. I think for flags, the help text is meant to be a reminder
rather than a how to. The more extended doc belongs elsewhere.

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go
File cmd/juju/addunit_test.go (right):

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go#n...
cmd/juju/addunit_test.go:41: c.Assert(err, ErrorMatches, t.err)
On 2013/07/15 10:27:20, jtv.canonical wrote:
> 
> Might as well use c.Check here, right?  That way you get all the failures.

Done.

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go#n...
cmd/juju/addunit_test.go:71: func (s *AddUnitSuite) assertForceMachine(c *C, svc
*state.Service, unitNum int, machineId string) {
On 2013/07/15 10:27:20, jtv.canonical wrote:
> 
> Could you document what this method is for?  Sooner or later people are going
to
> want changes, and then they'll have to know whether the changes jive with the
> function's original intent, as implicitly assumed at the call sites.

There's tonnes of these already which are undocumented. I was just following
prior convention :-)
But I added a comment.

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go#n...
cmd/juju/addunit_test.go:74: c.Assert(units, HasLen, 3)
On 2013/07/15 10:27:20, jtv.canonical wrote:
> 
> Why 3?  Father, Son, Holy Ghost?  Where does this come from?  Document!
> 
> And please don't tell me that you made a secret, undocumented deal between
this
> method and its caller to assume that 3 is the right number here, for I shall
> have to kick you in the nuts.  That kind of under-the-table dealing is a
recipe
> for unmaintainable code.
> 
> Think kindly of the poor engineer who is unfamiliar with this code and under
> pressure to fix it, because it may be you after others have messed with it!

It's always 3 (bootstrap machine, host, container) due to how all the tests have
been set up. But I've made it a parameter.

https://codereview.appspot.com/10962046/diff/12001/cmd/juju/addunit_test.go#n...
cmd/juju/addunit_test.go:87: err = runAddUnit(c, "some-service-name",
"--force-machine", machine2.Id())
On 2013/07/15 10:27:20, jtv.canonical wrote:
> 
> It helps to have blank lines separating the setup / exercise / verification
> portions of the test.  In this case the two runAddUnit calls and their
non-error
> assertions would be the "exercise" portions.
> 
> And by the way, why those two calls?  I am completely in the dark about this
> test's meaning.

Which 2 calls? The AddMachine ones? Well --force-machine needs to have machines
in the environment on which to force units to be deployed. And so the machines
need to be created prior to running add-unit.

https://codereview.appspot.com/10962046/diff/12001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/10962046/diff/12001/juju/conn_test.go#newcode514
juju/conn_test.go:514: func (s *DeployLocalSuite)
TestDeployForceMachineTooManyUnits(c *C) {
On 2013/07/15 10:27:20, jtv.canonical wrote:
> 
> A testing practice I have learned to great benefit (from Aaron, if memory
> serves) was to make the name of a test state the fact or behaviour that the
test
> proves: "deploy with --force-machine rejects too many units."  As if by magic
I
> started writing more focused tests.  I heartily recommend it!

Done.

[jtv.canonical, 2013-07-16 04:00:46]

On 2013/07/16 01:15:57, wallyworld wrote:

> > Could you document what this method is for?  Sooner or later people are
going
> to
> > want changes, and then they'll have to know whether the changes jive with
the
> > function's original intent, as implicitly assumed at the call sites.
> 
> There's tonnes of these already which are undocumented. I was just following
> prior convention :-)
> But I added a comment.

Thanks.  Improvement has to start somewhere!