Change add-machine tests to include output tests.

Change add-machine tests to include output tests.



https://code.launchpad.net/~jimmiebtlr/juju-core/add_machines_tag/+merge/221013

(do not edit description out of merge proposal)

[jimmiebtlr, 2014-05-27 05:53:34]

Please take a look.

[jimmiebtlr, 2014-05-28 04:45:54]

Please take a look.

[dave_cheney.net, 2014-05-28 05:22:38]

https://codereview.appspot.com/98610044/diff/60001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (right):

https://codereview.appspot.com/98610044/diff/60001/cmd/juju/addmachine.go#new...
cmd/juju/addmachine.go:161: ctx.Infof("created container %v",
names.MachineTag(machineId))
isn't this now going to say "created container machine-0/lxc:1" or "created
machine machine-0".

To reduce the stuttering how about replacing line 160:165 with

ctx.Infof("created %v", names.MachineTag(machineId))

[jimmiebtlr, 2014-05-28 05:43:32]

Please take a look.

[jameinel, 2014-05-28 09:29:55]

On 2014/05/28 05:43:32, jimmiebtlr wrote:
> Please take a look.

So I confirmed it with William, that we don't actually want the CLI to be
exposing tags to the user. The actual user facing things is intended to be
things like "0" for machine-0 and mysql/2 for unit-mysql-2.

I'm quite happy to see the tests get updated, but I think we actually want the
original syntax.

[jameinel, 2014-05-28 09:29:56]

On 2014/05/28 05:43:32, jimmiebtlr wrote:
> Please take a look.

So I confirmed it with William, that we don't actually want the CLI to be
exposing tags to the user. The actual user facing things is intended to be
things like "0" for machine-0 and mysql/2 for unit-mysql-2.

I'm quite happy to see the tests get updated, but I think we actually want the
original syntax.

[jimmiebtlr, 2014-05-29 00:44:36]

Please take a look.

[axw, 2014-05-29 03:31:55]

A few minors, nearly there

https://codereview.appspot.com/98610044/diff/90001/cmd/juju/addmachine.go
File cmd/juju/addmachine.go (left):

https://codereview.appspot.com/98610044/diff/90001/cmd/juju/addmachine.go#old...
cmd/juju/addmachine.go:159: 
Seeing as there were no changes to this file, may as well put the line back in.

https://codereview.appspot.com/98610044/diff/90001/cmd/juju/addmachine_test.go
File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/98610044/diff/90001/cmd/juju/addmachine_test.g...
cmd/juju/addmachine_test.go:84: c.Assert(testing.Stderr(cxt), gc.Equals,
"created container "+strconv.Itoa(i)+"/"+string(ctype)+"/0\n")
Extract the fmt.Sprintf from the line below, and reuse here.

https://codereview.appspot.com/98610044/diff/90001/cmd/juju/addmachine_test.g...
cmd/juju/addmachine_test.go:100: if string(container) == "machine" {
Why is this here? There's no such container-type as "machine" in
instance.ContainerTypes.

https://codereview.appspot.com/98610044/diff/90001/cmd/juju/addmachine_test.g...
cmd/juju/addmachine_test.go:103: c.Assert(testing.Stderr(cxt), gc.Equals,
"created container "+machineNum+"/"+string(container)+"/0\n")
Again, just extract the fmt.Sprintf and reuse please.

[jimmiebtlr, 2014-05-29 06:16:18]

Please take a look.

[axw, 2014-05-29 06:30:03]

On 2014/05/29 06:16:18, jimmiebtlr wrote:
> Please take a look.

LGTM, thanks

[fwereade, 2014-05-29 07:36:25]

LGTM with typo fix. Bonus points for idying up the other var/method names, but
not mandatory -- just sort out "cxt" and I'll approve.

(fwiw you can fix the description with `lbox propose -edit`)

https://codereview.appspot.com/98610044/diff/110001/cmd/juju/addmachine_test.go
File cmd/juju/addmachine_test.go (right):

https://codereview.appspot.com/98610044/diff/110001/cmd/juju/addmachine_test....
cmd/juju/addmachine_test.go:33: cxt, err := runAddMachine(c)
s/cxt/ctx/ throughout.

Or, better, s/cxt/context/ -- those few saved bytes/keystrokes really aren't
worth the mental load, especially for someone who's not already familiar with
that abbreviation.

(it took me a while to understand this myself, because ctx is such a "natural"
-- ie familiar -- contraction for me, but really: good long(ish) variable names
help everybody)

https://codereview.appspot.com/98610044/diff/110001/cmd/juju/addmachine_test....
cmd/juju/addmachine_test.go:87: s._assertAddContainer(c, strconv.Itoa(i),
machine, ctype)
this "_assert" is weird. Don't suppose anyone knows why we did that?

[jimmiebtlr, 2014-05-29 14:40:06]

Please take a look.

[fwereade, 2014-06-02 09:13:35]

LGTM, thanks.