Merge into trunk : add_machines_tag : Code : juju-core

Reviewer	Review Type	Date Requested	Status
Juju Engineering		2014-05-27	Pending
Review via email: mp+221013@code.staging.launchpad.net

Revision history for this message

Jimmie Butler (jimmiebtlr) wrote on 2014-05-27:

#

Download full text (6.9 KiB)

Reviewers: mp+221013_code.launchpad.net,

Message:
Please take a look.

Description:
juju add-machine now displays tag instead of id.

Also add tests to check output.

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/98610044/

Affected files (+45, -23 lines):
   A [revision details]
   M cmd/juju/addmachine.go
   M cmd/juju/addmachine_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140527050326-y76dbybaykpv5lc8
+New revision: <email address hidden>

Index: cmd/juju/addmachine.go
=== modified file 'cmd/juju/addmachine.go'
--- cmd/juju/addmachine.go 2014-05-12 05:23:59 +0000
+++ cmd/juju/addmachine.go 2014-05-27 04:20:30 +0000
@@ -158,9 +158,9 @@
machineId := machineInfo.Machine

   if names.IsContainerMachine(machineId) {
- ctx.Infof("created container %v", machineId)
+ ctx.Infof("created container %v", names.MachineTag(machineId))
   } else {
- ctx.Infof("created machine %v", machineId)
+ ctx.Infof("created machine %v", names.MachineTag(machineId))
   }
   return nil
  }

Index: cmd/juju/addmachine_test.go
=== modified file 'cmd/juju/addmachine_test.go'
--- cmd/juju/addmachine_test.go 2014-05-22 06:03:06 +0000
+++ cmd/juju/addmachine_test.go 2014-05-27 05:25:52 +0000
@@ -10,6 +10,7 @@
jc "github.com/juju/testing/checkers"
gc "launchpad.net/gocheck"

+ "launchpad.net/juju-core/cmd"
   "launchpad.net/juju-core/cmd/envcmd"
   "launchpad.net/juju-core/constraints"
   "launchpad.net/juju-core/instance"
@@ -24,25 +25,27 @@

var _ = gc.Suite(&AddMachineSuite{})

-func runAddMachine(c *gc.C, args ...string) error {
- _, err := testing.RunCommand(c, envcmd.Wrap(&AddMachineCommand{}),
args...)
- return err
+func runAddMachine(c *gc.C, args ...string) (*cmd.Context, error) {
+ cxt, err := testing.RunCommand(c, envcmd.Wrap(&AddMachineCommand{}),
args...)
+ return cxt, err
}

  func (s *AddMachineSuite) TestAddMachine(c *gc.C) {
- err := runAddMachine(c)
+ cxt, err := runAddMachine(c)
+ c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
   c.Assert(err, gc.IsNil)
   m, err := s.State.Machine("0")
   c.Assert(err, gc.IsNil)
   c.Assert(m.Life(), gc.Equals, state.Alive)
- c.Assert(m.Series(), gc.DeepEquals, "precise")
+ c.Assert(m.Series(), gc.Equals, "precise")
   mcons, err := m.Constraints()
   c.Assert(err, gc.IsNil)
   c.Assert(&mcons, jc.Satisfies, constraints.IsEmpty)
  }

  func (s *AddMachineSuite) TestAddMachineWithSeries(c *gc.C) {
- err := runAddMachine(c, "--series", "series")
+ cxt, err := runAddMachine(c, "--series", "series")
+ c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
   c.Assert(err, gc.IsNil)
   m, err := s.State.Machine("0")
   c.Assert(err, gc.IsNil)
@@ -50,7 +53,8 @@
  }

func (s *AddMachineSuite) TestAddMachineWithConstraints(c *gc.C) {
- err := runAddMachine(c, "--constraints", "mem=4G")
+ cxt, err := runAddMachine(c, "--constraints", "me...

Reviewers: mp+221013_code.launchpad.net,

Message:
Please take a look.

Description:
juju add-machine now displays tag instead of id.

Also add tests to check output.

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

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/98610044/

Affected files (+45, -23 lines):
   A [revision details]
   M cmd/juju/addmachine.go
   M cmd/juju/addmachine_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details]	2012-01-01 00:00:00 +0000
+++ [revision details]	2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140527050326-y76dbybaykpv5lc8
+New revision: jimmiebtlr@gmail.com-20140527052552-2fasrl9wy34ecb0x

Index: cmd/juju/addmachine.go
=== modified file 'cmd/juju/addmachine.go'
--- cmd/juju/addmachine.go	2014-05-12 05:23:59 +0000
+++ cmd/juju/addmachine.go	2014-05-27 04:20:30 +0000
@@ -158,9 +158,9 @@
  	machineId := machineInfo.Machine

if names.IsContainerMachine(machineId) {
-		ctx.Infof("created container %v", machineId)
+		ctx.Infof("created container %v", names.MachineTag(machineId))
  	} else {
-		ctx.Infof("created machine %v", machineId)
+		ctx.Infof("created machine %v", names.MachineTag(machineId))
  	}
  	return nil
  }

Index: cmd/juju/addmachine_test.go
=== modified file 'cmd/juju/addmachine_test.go'
--- cmd/juju/addmachine_test.go	2014-05-22 06:03:06 +0000
+++ cmd/juju/addmachine_test.go	2014-05-27 05:25:52 +0000
@@ -10,6 +10,7 @@
  	jc "github.com/juju/testing/checkers"
  	gc "launchpad.net/gocheck"

+	"launchpad.net/juju-core/cmd"
  	"launchpad.net/juju-core/cmd/envcmd"
  	"launchpad.net/juju-core/constraints"
  	"launchpad.net/juju-core/instance"
@@ -24,25 +25,27 @@

var _ = gc.Suite(&AddMachineSuite{})

-func runAddMachine(c *gc.C, args ...string) error {
-	_, err := testing.RunCommand(c, envcmd.Wrap(&AddMachineCommand{}),  
args...)
-	return err
+func runAddMachine(c *gc.C, args ...string) (*cmd.Context, error) {
+	cxt, err := testing.RunCommand(c, envcmd.Wrap(&AddMachineCommand{}),  
args...)
+	return cxt, err
  }

func (s *AddMachineSuite) TestAddMachine(c *gc.C) {
-	err := runAddMachine(c)
+	cxt, err := runAddMachine(c)
+	c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
  	c.Assert(err, gc.IsNil)
  	m, err := s.State.Machine("0")
  	c.Assert(err, gc.IsNil)
  	c.Assert(m.Life(), gc.Equals, state.Alive)
-	c.Assert(m.Series(), gc.DeepEquals, "precise")
+	c.Assert(m.Series(), gc.Equals, "precise")
  	mcons, err := m.Constraints()
  	c.Assert(err, gc.IsNil)
  	c.Assert(&mcons, jc.Satisfies, constraints.IsEmpty)
  }

func (s *AddMachineSuite) TestAddMachineWithSeries(c *gc.C) {
-	err := runAddMachine(c, "--series", "series")
+	cxt, err := runAddMachine(c, "--series", "series")
+	c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
  	c.Assert(err, gc.IsNil)
  	m, err := s.State.Machine("0")
  	c.Assert(err, gc.IsNil)
@@ -50,7 +53,8 @@
  }

func (s *AddMachineSuite) TestAddMachineWithConstraints(c *gc.C) {
-	err := runAddMachine(c, "--constraints", "mem=4G")
+	cxt, err := runAddMachine(c, "--constraints", "mem=4G")
+	c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
  	c.Assert(err, gc.IsNil)
  	m, err := s.State.Machine("0")
  	c.Assert(err, gc.IsNil)
@@ -76,49 +80,65 @@

func (s *AddMachineSuite) TestAddContainerToNewMachine(c *gc.C) {
  	for i, ctype := range instance.ContainerTypes {
-		c.Logf("test %d: %s", i, ctype)
-		err := runAddMachine(c, string(ctype))
+		cxt, err := runAddMachine(c, string(ctype))
+		c.Assert(testing.Stderr(cxt), gc.Equals, "created container  
machine-"+strconv.Itoa(i)+"-"+string(ctype)+"-0\n")
  		c.Assert(err, gc.IsNil)
  		s._assertAddContainer(c, strconv.Itoa(i), fmt.Sprintf("%d/%s/0", i,  
ctype), ctype)
  	}
  }

func (s *AddMachineSuite) TestAddContainerToExistingMachine(c *gc.C) {
-	err := runAddMachine(c)
+	cxt, err := runAddMachine(c)
+	c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
  	c.Assert(err, gc.IsNil)
  	for i, container := range instance.ContainerTypes {
  		machineNum := strconv.Itoa(i + 1)
-		err = runAddMachine(c)
+		cxt, err = runAddMachine(c)
+		c.Assert(testing.Stderr(cxt), gc.Equals, "created machine  
machine-"+machineNum+"\n")
  		c.Assert(err, gc.IsNil)
-		err := runAddMachine(c, fmt.Sprintf("%s:%s", container, machineNum))
+		cxt, err := runAddMachine(c, fmt.Sprintf("%s:%s", container, machineNum))
+		if string(container) == "machine" {
+			c.Assert(testing.Stderr(cxt), gc.Equals, "created container  
machine-"+machineNum+"\n")
+		} else {
+			c.Assert(testing.Stderr(cxt), gc.Equals, "created container  
machine-"+machineNum+"-"+string(container)+"-0\n")
+		}
  		c.Assert(err, gc.IsNil)
  		s._assertAddContainer(c, machineNum, fmt.Sprintf("%s/%s/0", machineNum,  
container), container)
  	}
  }

func (s *AddMachineSuite) TestAddUnsupportedContainerToMachine(c *gc.C) {
-	err := runAddMachine(c)
+	cxt, err := runAddMachine(c)
+	c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
  	c.Assert(err, gc.IsNil)
  	m, err := s.State.Machine("0")
  	c.Assert(err, gc.IsNil)
  	m.SetSupportedContainers([]instance.ContainerType{instance.KVM})
-	err = runAddMachine(c, "lxc:0")
+	cxt, err = runAddMachine(c, "lxc:0")
+	c.Assert(testing.Stderr(cxt), gc.Equals, "")
  	c.Assert(err, gc.ErrorMatches, "cannot add a new machine: machine 0  
cannot host lxc containers")
  }

func (s *AddMachineSuite) TestAddMachineErrors(c *gc.C) {
-	err := runAddMachine(c, ":lxc")
+	cxt, err := runAddMachine(c, ":lxc")
+	c.Assert(testing.Stderr(cxt), gc.Equals, "")
  	c.Check(err, gc.ErrorMatches, `cannot add a new machine: :lxc placement  
is invalid`)
-	err = runAddMachine(c, "lxc:")
+	cxt, err = runAddMachine(c, "lxc:")
+	c.Assert(cxt, gc.IsNil)
  	c.Check(err, gc.ErrorMatches, `invalid value "" for "lxc" scope: expected  
machine-id`)
-	err = runAddMachine(c, "2")
+	cxt, err = runAddMachine(c, "2")
+	c.Assert(testing.Stderr(cxt), gc.Equals, "")
  	c.Check(err, gc.ErrorMatches, `machine-id cannot be specified when adding  
machines`)
-	err = runAddMachine(c, "foo")
+	cxt, err = runAddMachine(c, "foo")
+	c.Assert(testing.Stderr(cxt), gc.Equals, "")
  	c.Check(err, gc.ErrorMatches, `cannot add a new machine: foo placement is  
invalid`)
-	err = runAddMachine(c, "foo:bar")
+	cxt, err = runAddMachine(c, "foo:bar")
+	c.Assert(testing.Stderr(cxt), gc.Equals, "")
  	c.Check(err, gc.ErrorMatches, `invalid environment name "foo"`)
-	err = runAddMachine(c, "dummyenv:invalid")
+	cxt, err = runAddMachine(c, "dummyenv:invalid")
+	c.Assert(testing.Stderr(cxt), gc.Equals, "")
  	c.Check(err, gc.ErrorMatches, `cannot add a new machine: invalid  
placement is invalid`)
-	err = runAddMachine(c, "lxc", "--constraints", "container=lxc")
+	cxt, err = runAddMachine(c, "lxc", "--constraints", "container=lxc")
+	c.Assert(cxt, gc.IsNil)
  	c.Check(err, gc.ErrorMatches, `container constraint "lxc" not allowed  
when adding a machine`)
  }

Revision history for this message

Dave Cheney (dave-cheney) wrote on 2014-05-27:

#

Download full text (6.0 KiB)

> +func runAddMachine(c *gc.C, args ...string) (*cmd.Context, error) {
> + cxt, err := testing.RunCommand(c, envcmd.Wrap(&AddMachineCommand{}), args...)
> + return cxt, err
> }

This can be collapsed onto one line

> func (s *AddMachineSuite) TestAddMachine(c *gc.C) {
> - err := runAddMachine(c)
> + cxt, err := runAddMachine(c)
> + c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")

Why is this going to stderr, it's not an error.

> c.Assert(err, gc.IsNil)

Test the error first

> m, err := s.State.Machine("0")
> c.Assert(err, gc.IsNil)
> c.Assert(m.Life(), gc.Equals, state.Alive)
> - c.Assert(m.Series(), gc.DeepEquals, "precise")
> + c.Assert(m.Series(), gc.Equals, "precise")
> mcons, err := m.Constraints()
> c.Assert(err, gc.IsNil)
> c.Assert(&mcons, jc.Satisfies, constraints.IsEmpty)
> }
>
> func (s *AddMachineSuite) TestAddMachineWithSeries(c *gc.C) {
> - err := runAddMachine(c, "--series", "series")
> + cxt, err := runAddMachine(c, "--series", "series")
> + c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")

same

> c.Assert(err, gc.IsNil)
> m, err := s.State.Machine("0")
> c.Assert(err, gc.IsNil)
> @@ -50,7 +53,8 @@
> }
>
> func (s *AddMachineSuite) TestAddMachineWithConstraints(c *gc.C) {
> - err := runAddMachine(c, "--constraints", "mem=4G")
> + cxt, err := runAddMachine(c, "--constraints", "mem=4G")
> + c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")

same

> c.Assert(err, gc.IsNil)
> m, err := s.State.Machine("0")
> c.Assert(err, gc.IsNil)
> @@ -76,49 +80,65 @@
>
> func (s *AddMachineSuite) TestAddContainerToNewMachine(c *gc.C) {
> for i, ctype := range instance.ContainerTypes {
> - c.Logf("test %d: %s", i, ctype)
> - err := runAddMachine(c, string(ctype))
> + cxt, err := runAddMachine(c, string(ctype))
> + c.Assert(testing.Stderr(cxt), gc.Equals, "created container machine-"+strconv.Itoa(i)+"-"+string(ctype)+"-0\n")

same

> c.Assert(err, gc.IsNil)
> s._assertAddContainer(c, strconv.Itoa(i), fmt.Sprintf("%d/%s/0", i, ctype), ctype)
> }
> }
>
> func (s *AddMachineSuite) TestAddContainerToExistingMachine(c *gc.C) {
> - err := runAddMachine(c)
> + cxt, err := runAddMachine(c)
> + c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
> c.Assert(err, gc.IsNil)
> for i, container := range instance.ContainerTypes {
> machineNum := strconv.Itoa(i + 1)
> - err = runAddMachine(c)
> + cxt, err = runAddMachine(c)
> + c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-"+machineNum+"\n")
> c.Assert(err, gc.IsNil)
> - err := runAddMachine(c, fmt.Sprintf("%s:%s", container, machineNum))
> + cxt, err := runAddMachine(c, fmt.Sprintf("%s:%s", container, machineNum))
> + if string(container) == "machine" {
> + ...

> +func runAddMachine(c *gc.C, args ...string) (*cmd.Context, error) {
> +       cxt, err := testing.RunCommand(c, envcmd.Wrap(&AddMachineCommand{}), args...)
> +       return cxt, err
>  }

This can be collapsed onto one line

>  func (s *AddMachineSuite) TestAddMachine(c *gc.C) {
> -       err := runAddMachine(c)
> +       cxt, err := runAddMachine(c)
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")

Why is this going to stderr, it's not an error.

>         c.Assert(err, gc.IsNil)

Test the error first

>         m, err := s.State.Machine("0")
>         c.Assert(err, gc.IsNil)
>         c.Assert(m.Life(), gc.Equals, state.Alive)
> -       c.Assert(m.Series(), gc.DeepEquals, "precise")
> +       c.Assert(m.Series(), gc.Equals, "precise")
>         mcons, err := m.Constraints()
>         c.Assert(err, gc.IsNil)
>         c.Assert(&mcons, jc.Satisfies, constraints.IsEmpty)
>  }
>
>  func (s *AddMachineSuite) TestAddMachineWithSeries(c *gc.C) {
> -       err := runAddMachine(c, "--series", "series")
> +       cxt, err := runAddMachine(c, "--series", "series")
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")

same

>         c.Assert(err, gc.IsNil)
>         m, err := s.State.Machine("0")
>         c.Assert(err, gc.IsNil)
> @@ -50,7 +53,8 @@
>  }
>
>  func (s *AddMachineSuite) TestAddMachineWithConstraints(c *gc.C) {
> -       err := runAddMachine(c, "--constraints", "mem=4G")
> +       cxt, err := runAddMachine(c, "--constraints", "mem=4G")
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")

same

>         c.Assert(err, gc.IsNil)
>         m, err := s.State.Machine("0")
>         c.Assert(err, gc.IsNil)
> @@ -76,49 +80,65 @@
>
>  func (s *AddMachineSuite) TestAddContainerToNewMachine(c *gc.C) {
>         for i, ctype := range instance.ContainerTypes {
> -               c.Logf("test %d: %s", i, ctype)
> -               err := runAddMachine(c, string(ctype))
> +               cxt, err := runAddMachine(c, string(ctype))
> +               c.Assert(testing.Stderr(cxt), gc.Equals, "created container machine-"+strconv.Itoa(i)+"-"+string(ctype)+"-0\n")

same

>                 c.Assert(err, gc.IsNil)
>                 s._assertAddContainer(c, strconv.Itoa(i), fmt.Sprintf("%d/%s/0", i, ctype), ctype)
>         }
>  }
>
>  func (s *AddMachineSuite) TestAddContainerToExistingMachine(c *gc.C) {
> -       err := runAddMachine(c)
> +       cxt, err := runAddMachine(c)
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
>         c.Assert(err, gc.IsNil)
>         for i, container := range instance.ContainerTypes {
>                 machineNum := strconv.Itoa(i + 1)
> -               err = runAddMachine(c)
> +               cxt, err = runAddMachine(c)
> +               c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-"+machineNum+"\n")
>                 c.Assert(err, gc.IsNil)
> -               err := runAddMachine(c, fmt.Sprintf("%s:%s", container, machineNum))
> +               cxt, err := runAddMachine(c, fmt.Sprintf("%s:%s", container, machineNum))
> +               if string(container) == "machine" {
> +                       c.Assert(testing.Stderr(cxt), gc.Equals, "created container machine-"+machineNum+"\n")
> +               } else {
> +                       c.Assert(testing.Stderr(cxt), gc.Equals, "created container machine-"+machineNum+"-"+string(container)+"-0\n")
> +               }
>                 c.Assert(err, gc.IsNil)
>                 s._assertAddContainer(c, machineNum, fmt.Sprintf("%s/%s/0", machineNum, container), container)
>         }
>  }
>
>  func (s *AddMachineSuite) TestAddUnsupportedContainerToMachine(c *gc.C) {
> -       err := runAddMachine(c)
> +       cxt, err := runAddMachine(c)
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "created machine machine-0\n")
>         c.Assert(err, gc.IsNil)
>         m, err := s.State.Machine("0")
>         c.Assert(err, gc.IsNil)
>         m.SetSupportedContainers([]instance.ContainerType{instance.KVM})
> -       err = runAddMachine(c, "lxc:0")
> +       cxt, err = runAddMachine(c, "lxc:0")
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "")
>         c.Assert(err, gc.ErrorMatches, "cannot add a new machine: machine 0 cannot host lxc containers")
>  }
>
>  func (s *AddMachineSuite) TestAddMachineErrors(c *gc.C) {
> -       err := runAddMachine(c, ":lxc")
> +       cxt, err := runAddMachine(c, ":lxc")
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "")
>         c.Check(err, gc.ErrorMatches, `cannot add a new machine: :lxc placement is invalid`)
> -       err = runAddMachine(c, "lxc:")
> +       cxt, err = runAddMachine(c, "lxc:")
> +       c.Assert(cxt, gc.IsNil)
>         c.Check(err, gc.ErrorMatches, `invalid value "" for "lxc" scope: expected machine-id`)
> -       err = runAddMachine(c, "2")
> +       cxt, err = runAddMachine(c, "2")
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "")
>         c.Check(err, gc.ErrorMatches, `machine-id cannot be specified when adding machines`)
> -       err = runAddMachine(c, "foo")
> +       cxt, err = runAddMachine(c, "foo")
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "")
>         c.Check(err, gc.ErrorMatches, `cannot add a new machine: foo placement is invalid`)
> -       err = runAddMachine(c, "foo:bar")
> +       cxt, err = runAddMachine(c, "foo:bar")
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "")
>         c.Check(err, gc.ErrorMatches, `invalid environment name "foo"`)
> -       err = runAddMachine(c, "dummyenv:invalid")
> +       cxt, err = runAddMachine(c, "dummyenv:invalid")
> +       c.Assert(testing.Stderr(cxt), gc.Equals, "")
>         c.Check(err, gc.ErrorMatches, `cannot add a new machine: invalid placement is invalid`)
> -       err = runAddMachine(c, "lxc", "--constraints", "container=lxc")
> +       cxt, err = runAddMachine(c, "lxc", "--constraints", "container=lxc")
> +       c.Assert(cxt, gc.IsNil)
>         c.Check(err, gc.ErrorMatches, `container constraint "lxc" not allowed when adding a machine`)
>  }
>
>

Revision history for this message

Jimmie Butler (jimmiebtlr) wrote on 2014-05-28:

#

Please take a look.

https://codereview.appspot.com/98610044/

Revision history for this message

Dave Cheney (dave-cheney) wrote on 2014-05-28:

#

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#newcode161
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))

https://codereview.appspot.com/98610044/

Revision history for this message

Jimmie Butler (jimmiebtlr) wrote on 2014-05-28:

#

Please take a look.

https://codereview.appspot.com/98610044/

Revision history for this message

John A Meinel (jameinel) wrote on 2014-05-28:

#

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.

https://codereview.appspot.com/98610044/

Revision history for this message

John A Meinel (jameinel) wrote on 2014-05-28:

#

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.

https://codereview.appspot.com/98610044/

Revision history for this message

Jimmie Butler (jimmiebtlr) wrote on 2014-05-29:

#

Please take a look.

https://codereview.appspot.com/98610044/

Revision history for this message

Andrew Wilkins (axwalk) wrote on 2014-05-29:

#

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#oldcode159
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.go#newcode84
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.go#newcode100
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.go#newcode103
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.

https://codereview.appspot.com/98610044/

Revision history for this message

Jimmie Butler (jimmiebtlr) wrote on 2014-05-29:

#

Please take a look.

https://codereview.appspot.com/98610044/

Revision history for this message

Andrew Wilkins (axwalk) wrote on 2014-05-29:

#

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

LGTM, thanks

https://codereview.appspot.com/98610044/

Revision history for this message

William Reade (fwereade) wrote on 2014-05-29:

#

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.go#newcode33
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.go#newcode87
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?

https://codereview.appspot.com/98610044/

Revision history for this message

Jimmie Butler (jimmiebtlr) wrote on 2014-05-29:

#

Please take a look.

https://codereview.appspot.com/98610044/

Revision history for this message

William Reade (fwereade) wrote on 2014-06-02:

#

LGTM, thanks.

https://codereview.appspot.com/98610044/

juju-core

Merge lp://staging/~jimmiebtlr/juju-core/add_machines_tag into lp://staging/~go-bot/juju-core/trunk

Commit message

Description of the change

Preview Diff

Subscribers