Merge lp:~wallyworld/juju-core/add-machine-panic into lp:~go-bot/juju-core/trunk

[Ian Booth]

Remove panic for bad arg to add-machine

If add-machine is invoked with a machine id as the
argument, it no longer panics.

The recently refactored IsMachineOrNewContainer
method was moved from the names package to cmd
since it's only used there. Plus there were left
over tests in state which were duplicated in names
and some of the tests were missing so I fix that
as well.

https://codereview.appspot.com/12309043/

Remove panic for bad arg to add-machine

If add-machine is invoked with a machine id as the
argument, it no longer panics.

The recently refactored IsMachineOrNewContainer
method was moved from the names package to cmd
since it's only used there. Plus there were left
over tests in state which were duplicated in names
and some of the tests were missing so I fix that
as well.

https://codereview.appspot.com/12309043/

[Ian Booth (wallyworld)]

Reviewers: mp+178185_code.launchpad.net,

Message:
Please take a look.

Description:
Remove panic for bad arg to add-machine

If add-machine is invoked with a machine id as the
argument, it no longer panics.

The recently refactored IsMachineOrNewContainer
method was moved from the names package to cmd
since it's only used there. Plus there were left
over tests in state which were duplicated in names
and some of the tests were missing so I fix that
as well.

https://code.launchpad.net/~wallyworld/juju-core/add-machine-panic/+merge/178185

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/juju/addmachine.go
   M cmd/juju/addmachine_test.go
   M cmd/juju/addunit.go
   A cmd/names.go
   A cmd/names_test.go
   M names/machine.go
   M names/machine_test.go
   M names/service_test.go
   M state/state_test.go

[Tim Penhey (thumper)]

LGTM

https://codereview.appspot.com/12309043/

[Dimiter Naydenov (dimitern)]

LGTM minus some suggestions below

https://codereview.appspot.com/12309043/diff/1/cmd/names.go
File cmd/names.go (right):

https://codereview.appspot.com/12309043/diff/1/cmd/names.go#newcode8
cmd/names.go:8: "regexp"
"regexp"

"...names"

https://codereview.appspot.com/12309043/diff/1/cmd/names_test.go
File cmd/names_test.go (right):

https://codereview.appspot.com/12309043/diff/1/cmd/names_test.go#newcode12
cmd/names_test.go:12: type NamesSuite struct {
s/NamesSuite/namesSuite/ ?

https://codereview.appspot.com/12309043/diff/1/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12309043/diff/1/names/machine.go#newcode14
names/machine.go:14: var (
var validMachine = ...

https://codereview.appspot.com/12309043/diff/1/names/service_test.go
File names/service_test.go (right):

https://codereview.appspot.com/12309043/diff/1/names/service_test.go#newcode36
names/service_test.go:36:
d

https://codereview.appspot.com/12309043/

[Ian Booth (wallyworld)]

https://codereview.appspot.com/12309043/diff/1/cmd/names.go
File cmd/names.go (right):

https://codereview.appspot.com/12309043/diff/1/cmd/names.go#newcode8
cmd/names.go:8: "regexp"
On 2013/08/02 08:51:03, dimitern wrote:
> "regexp"

> "...names"

Done.

https://codereview.appspot.com/12309043/diff/1/cmd/names_test.go
File cmd/names_test.go (right):

https://codereview.appspot.com/12309043/diff/1/cmd/names_test.go#newcode12
cmd/names_test.go:12: type NamesSuite struct {
On 2013/08/02 08:51:03, dimitern wrote:
> s/NamesSuite/namesSuite/ ?

Done.

https://codereview.appspot.com/12309043/diff/1/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12309043/diff/1/names/machine.go#newcode14
names/machine.go:14: var (
On 2013/08/02 08:51:03, dimitern wrote:
> var validMachine = ...

Done.

https://codereview.appspot.com/12309043/diff/1/names/service_test.go
File names/service_test.go (right):

https://codereview.appspot.com/12309043/diff/1/names/service_test.go#newcode36
names/service_test.go:36:
On 2013/08/02 08:51:03, dimitern wrote:
> d

Done.

https://codereview.appspot.com/12309043/

[Go Bot (go-bot)]

The attempt to merge lp:~wallyworld/juju-core/add-machine-panic into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.834s
ok launchpad.net/juju-core/agent/tools 25.645s
ok launchpad.net/juju-core/bzr 6.633s
ok launchpad.net/juju-core/cert 2.235s
ok launchpad.net/juju-core/charm 0.552s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.024s
ok launchpad.net/juju-core/cmd 0.306s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 99.876s
ok launchpad.net/juju-core/cmd/jujud 48.141s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 0.885s
ok launchpad.net/juju-core/constraints 0.039s
ok launchpad.net/juju-core/container/lxc 0.331s
? launchpad.net/juju-core/container/lxc/mock [no test files]
ok launchpad.net/juju-core/downloader 5.282s
ok launchpad.net/juju-core/environs 1.236s
? launchpad.net/juju-core/environs/all [no test files]
ok launchpad.net/juju-core/environs/azure 3.034s
ok launchpad.net/juju-core/environs/cloudinit 0.482s
ok launchpad.net/juju-core/environs/config 0.696s
ok launchpad.net/juju-core/environs/dummy 15.957s
ok launchpad.net/juju-core/environs/ec2 181.906s
ok launchpad.net/juju-core/environs/imagemetadata 0.305s
ok launchpad.net/juju-core/environs/instances 0.238s
ok launchpad.net/juju-core/environs/jujutest 0.320s
ok launchpad.net/juju-core/environs/local 1.204s
? launchpad.net/juju-core/environs/local/storage [no test files]
ok launchpad.net/juju-core/environs/localstorage 0.203s
ok launchpad.net/juju-core/environs/maas 2.047s
ok launchpad.net/juju-core/environs/openstack 3.827s
? launchpad.net/juju-core/environs/provider [no test files]
ok launchpad.net/juju-core/environs/sync 0.273s
ok launchpad.net/juju-core/environs/testing 0.022s
? launchpad.net/juju-core/errors [no test files]
ok launchpad.net/juju-core/instance 0.032s
ok launchpad.net/juju-core/juju 11.628s
ok launchpad.net/juju-core/juju/osenv 0.237s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.018s
ok launchpad.net/juju-core/log/syslog 0.027s
ok launchpad.net/juju-core/names 0.029s
ok launchpad.net/juju-core/rpc 0.259s
ok launchpad.net/juju-core/rpc/jsoncodec 0.222s
ok launchpad.net/juju-core/schema 0.020s
FAIL launchpad.net/juju-core/state [build failed]
? launchpad.net/juju-core/state/api [no test files]
ok launchpad.net/juju-core/state/api/agent 1.784s
? launchpad.net/juju-core/state/api/common [no test files]
ok launchpad.net/juju-core/state/api/deployer 5.224s
ok launchpad.net/juju-core/state/api/machiner 2.556s
ok launchpad.net/juju-core/state/api/params 0.032s
ok launchpad.net/juju-core/state/api/upgrader 2.869s
ok launchpad.net/juju-core/state/api/watcher 2.955s
ok launchpad.net/juju-core/state/apiserver 2.710s
ok launchpad.net/juju-core/state/apiserver/agent 2.529s
ok launchpad.net/juju-core/state/apiserver/client 13.238s
ok launchpad.net/juju-co...

=== modified file 'cmd/juju/addmachine.go'
--- cmd/juju/addmachine.go	2013-07-30 23:46:30 +0000
+++ cmd/juju/addmachine.go	2013-08-02 22:26:24 +0000
@@ -57,7 +57,7 @@
 	}
 	// container arg can either be 'type:machine' or 'type'
 	if c.ContainerType, err = instance.ParseSupportedContainerType(containerSpec); err != nil {
-		if !names.IsMachineOrNewContainer(containerSpec) {
+		if names.IsMachine(containerSpec) || !cmd.IsMachineOrNewContainer(containerSpec) {
 			return fmt.Errorf("malformed container argument %q", containerSpec)
 		}
 		sep := strings.Index(containerSpec, ":")

=== modified file 'cmd/juju/addmachine_test.go'
--- cmd/juju/addmachine_test.go	2013-07-16 00:47:26 +0000
+++ cmd/juju/addmachine_test.go	2013-08-02 22:26:24 +0000
@@ -92,10 +92,14 @@
 }
 
 func (s *AddMachineSuite) TestAddMachineErrors(c *C) {
-	err := runAddMachine(c, ":foo")
-	c.Assert(err, ErrorMatches, `malformed container argument ":foo"`)
-	err = runAddMachine(c, "foo:")
-	c.Assert(err, ErrorMatches, `malformed container argument "foo:"`)
+	err := runAddMachine(c, ":lxc")
+	c.Assert(err, ErrorMatches, `malformed container argument ":lxc"`)
+	err = runAddMachine(c, "lxc:")
+	c.Assert(err, ErrorMatches, `malformed container argument "lxc:"`)
+	err = runAddMachine(c, "2")
+	c.Assert(err, ErrorMatches, `malformed container argument "2"`)
+	err = runAddMachine(c, "foo")
+	c.Assert(err, ErrorMatches, `malformed container argument "foo"`)
 	err = runAddMachine(c, "lxc", "--constraints", "container=lxc")
 	c.Assert(err, ErrorMatches, `container constraint "lxc" not allowed when adding a machine`)
 }

=== modified file 'cmd/juju/addunit.go'
--- cmd/juju/addunit.go	2013-07-30 23:46:30 +0000
+++ cmd/juju/addunit.go	2013-08-02 22:26:24 +0000
@@ -11,7 +11,6 @@
 
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/juju"
-	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/statecmd"
 )
@@ -36,7 +35,7 @@
 		if c.NumUnits > 1 {
 			return errors.New("cannot use --num-units > 1 with --to")
 		}
-		if !names.IsMachineOrNewContainer(c.ToMachineSpec) {
+		if !cmd.IsMachineOrNewContainer(c.ToMachineSpec) {
 			return fmt.Errorf("invalid --to parameter %q", c.ToMachineSpec)
 		}
 	}

=== added file 'cmd/names.go'
--- cmd/names.go	1970-01-01 00:00:00 +0000
+++ cmd/names.go	2013-08-02 22:26:24 +0000
@@ -0,0 +1,25 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package cmd
+
+import (
+	"regexp"
+
+	"launchpad.net/juju-core/names"
+)
+
+const (
+	ContainerSnippet     = "(/[a-z]+/" + names.NumberSnippet + ")"
+	ContainerSpecSnippet = "([a-z]+:)?"
+)
+
+var (
+	validMachineOrNewContainer = regexp.MustCompile("^" + ContainerSpecSnippet + names.MachineSnippet + "$")
+)
+
+// IsMachineOrNewContainer returns whether spec is a valid machine id
+// or new container definition.
+func IsMachineOrNewContainer(spec string) bool {
+	return validMachineOrNewContainer.MatchString(spec)
+}

=== added file 'cmd/names_test.go'
--- cmd/names_test.go	1970-01-01 00:00:00 +0000
+++ cmd/names_test.go	2013-08-02 22:26:24 +0000
@@ -0,0 +1,36 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package cmd_test
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/cmd"
+)
+
+type namesSuite struct {
+}
+
+var _ = gc.Suite(&namesSuite{})
+
+func (*namesSuite) TestNameChecks(c *gc.C) {
+	assertMachineOrNewContainer := func(s string, expect bool) {
+		c.Assert(cmd.IsMachineOrNewContainer(s), gc.Equals, expect)
+	}
+	assertMachineOrNewContainer("0", true)
+	assertMachineOrNewContainer("00", false)
+	assertMachineOrNewContainer("1", true)
+	assertMachineOrNewContainer("0/lxc/0", true)
+	assertMachineOrNewContainer("lxc:0", true)
+	assertMachineOrNewContainer("lxc:lxc:0", false)
+	assertMachineOrNewContainer("kvm:0/lxc/1", true)
+	assertMachineOrNewContainer("lxc:", false)
+	assertMachineOrNewContainer(":lxc", false)
+	assertMachineOrNewContainer("0/lxc/", false)
+	assertMachineOrNewContainer("0/lxc", false)
+	assertMachineOrNewContainer("kvm:0/lxc", false)
+	assertMachineOrNewContainer("0/lxc/01", false)
+	assertMachineOrNewContainer("0/lxc/10", true)
+	assertMachineOrNewContainer("0/kvm/4", true)
+}

=== modified file 'names/machine.go'
--- names/machine.go	2013-07-30 18:27:04 +0000
+++ names/machine.go	2013-08-02 22:26:24 +0000
@@ -7,27 +7,17 @@
 )
 
 const (
-	ContainerSnippet     = "(/[a-z]+/" + NumberSnippet + ")"
-	MachineSnippet       = NumberSnippet + ContainerSnippet + "*"
-	ContainerSpecSnippet = "([a-z]+:)?"
+	ContainerSnippet = "(/[a-z]+/" + NumberSnippet + ")"
+	MachineSnippet   = NumberSnippet + ContainerSnippet + "*"
 )
 
-var (
-	validMachine               = regexp.MustCompile("^" + MachineSnippet + "$")
-	validMachineOrNewContainer = regexp.MustCompile("^" + ContainerSpecSnippet + MachineSnippet + "$")
-)
+var validMachine = regexp.MustCompile("^" + MachineSnippet + "$")
 
 // IsMachine returns whether id is a valid machine id.
 func IsMachine(id string) bool {
 	return validMachine.MatchString(id)
 }
 
-// IsMachineOrNewContainer returns whether spec is a valid machine id
-// or new container definition.
-func IsMachineOrNewContainer(spec string) bool {
-	return validMachineOrNewContainer.MatchString(spec)
-}
-
 // MachineTag returns the tag for the machine with the given id.
 func MachineTag(id string) string {
 	tag := makeTag(MachineTagKind, id)

=== modified file 'names/machine_test.go'
--- names/machine_test.go	2013-07-31 14:21:38 +0000
+++ names/machine_test.go	2013-08-02 22:26:24 +0000
@@ -64,6 +64,9 @@
 	{pattern: "1/foo", valid: false},
 	{pattern: "2/foo/", valid: false},
 	{pattern: "3/lxc/42", valid: true},
+	{pattern: "3/lxc-nodash/42", valid: false},
+	{pattern: "0/lxc/00", valid: false},
+	{pattern: "0/lxc/0/", valid: false},
 	{pattern: "03/lxc/42", valid: false},
 	{pattern: "3/lxc/042", valid: false},
 	{pattern: "4/foo/bar", valid: false},
@@ -81,35 +84,3 @@
 		c.Assert(names.IsMachine(test.pattern), gc.Equals, test.valid)
 	}
 }
-
-var machineOrNewContainerTests = []struct {
-	pattern string
-	valid   bool
-}{
-	{pattern: "42", valid: true},
-	{pattern: "0", valid: true},
-	{pattern: "042", valid: false},
-	{pattern: ":42", valid: false},
-	{pattern: "lxc:42", valid: true},
-	{pattern: "lxc:042", valid: false},
-	{pattern: "lxc:0", valid: true},
-	{pattern: "foo42", valid: false},
-	{pattern: "foo", valid: false},
-	{pattern: "foo:3/", valid: false},
-	{pattern: "kvm:3/foo", valid: false},
-	{pattern: "kvm:3/foo/", valid: false},
-	{pattern: "lxc:42/kvm/0", valid: true},
-	{pattern: "lxc:042/kvm/0", valid: false},
-	{pattern: "lxc:42/kvm/00", valid: false},
-	{pattern: "lxc:42/kvm/56/lxc/0", valid: true},
-	{pattern: "lxc:042/kvm/56/lxc/0", valid: false},
-	{pattern: "lxc:42/kvm/056/lxc/0", valid: false},
-	{pattern: "lxc:42/kvm/56/lxc/00", valid: false},
-}
-
-func (s *machineSuite) TestMachineOrNewContainerFormats(c *gc.C) {
-	for i, test := range machineOrNewContainerTests {
-		c.Logf("test %d: %q", i, test.pattern)
-		c.Assert(names.IsMachineOrNewContainer(test.pattern), gc.Equals, test.valid)
-	}
-}

=== modified file 'names/service_test.go'
--- names/service_test.go	2013-07-31 14:21:38 +0000
+++ names/service_test.go	2013-08-02 22:26:24 +0000
@@ -17,6 +17,7 @@
 	pattern string
 	valid   bool
 }{
+	{pattern: "", valid: false},
 	{pattern: "wordpress", valid: true},
 	{pattern: "foo42", valid: true},
 	{pattern: "doing55in54", valid: true},
@@ -27,11 +28,25 @@
 	{pattern: "foo/42", valid: false},
 	{pattern: "is-it-", valid: false},
 	{pattern: "broken2-", valid: false},
+	{pattern: "foo2", valid: true},
+	{pattern: "foo-2", valid: false},
 }
 
 func (s *serviceSuite) TestServiceNameFormats(c *gc.C) {
+	assertService := func(s string, expect bool) {
+		c.Assert(names.IsService(s), gc.Equals, expect)
+		// Check that anything that is considered a valid service name
+		// is also (in)valid if a(n) (in)valid unit designator is added
+		// to it.
+		c.Assert(names.IsUnit(s+"/0"), gc.Equals, expect)
+		c.Assert(names.IsUnit(s+"/99"), gc.Equals, expect)
+		c.Assert(names.IsUnit(s+"/-1"), gc.Equals, false)
+		c.Assert(names.IsUnit(s+"/blah"), gc.Equals, false)
+		c.Assert(names.IsUnit(s+"/"), gc.Equals, false)
+	}
+
 	for i, test := range serviceNameTests {
 		c.Logf("test %d: %q", i, test.pattern)
-		c.Assert(names.IsService(test.pattern), gc.Equals, test.valid)
+		assertService(test.pattern, test.valid)
 	}
 }

=== modified file 'state/state_test.go'
--- state/state_test.go	2013-08-01 15:41:09 +0000
+++ state/state_test.go	2013-08-02 22:26:24 +0000
@@ -19,7 +19,6 @@
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
-	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api/params"
 	statetesting "launchpad.net/juju-core/state/testing"
@@ -1046,87 +1045,6 @@
 	}
 }
 
-func (*StateSuite) TestNameChecks(c *gc.C) {
-	assertService := func(s string, expect bool) {
-		c.Assert(names.IsService(s), gc.Equals, expect)
-		// Check that anything that is considered a valid service name
-		// is also (in)valid if a(n) (in)valid unit designator is added
-		// to it.
-		c.Assert(names.IsUnit(s+"/0"), gc.Equals, expect)
-		c.Assert(names.IsUnit(s+"/99"), gc.Equals, expect)
-		c.Assert(names.IsUnit(s+"/-1"), gc.Equals, false)
-		c.Assert(names.IsUnit(s+"/blah"), gc.Equals, false)
-		c.Assert(names.IsUnit(s+"/"), gc.Equals, false)
-	}
-	// Service names must be non-empty...
-	assertService("", false)
-	// must not consist entirely of numbers
-	assertService("33", false)
-	// may consist of a single word
-	assertService("wordpress", true)
-	// may contain hyphen-seperated words...
-	assertService("super-duper-wordpress", true)
-	// ...but those words must have at least one letter in them
-	assertService("super-1234-wordpress", false)
-	// may contain internal numbers.
-	assertService("w0rd-pre55", true)
-	// must not begin with a number
-	assertService("3wordpress", false)
-	// but internal, hyphen-sperated words can begin with numbers
-	assertService("foo-2foo", true)
-	// and may end with a number...
-	assertService("foo2", true)
-	// ...unless that number is all by itself
-	assertService("foo-2", false)
-
-	assertMachine := func(s string, expect bool) {
-		c.Assert(names.IsMachine(s), gc.Equals, expect)
-	}
-	assertMachine("0", true)
-	assertMachine("00", false)
-	assertMachine("1", true)
-	assertMachine("1000001", true)
-	assertMachine("01", false)
-	assertMachine("-1", false)
-	assertMachine("", false)
-	assertMachine("cantankerous", false)
-	// And container specs
-	assertMachine("0/", false)
-	assertMachine("0/0", false)
-	assertMachine("0/lxc", false)
-	assertMachine("0/lxc/", false)
-	assertMachine("0/lxc/0", true)
-	assertMachine("0/lxc/0/", false)
-	assertMachine("0/lxc/00", false)
-	assertMachine("0/lxc/01", false)
-	assertMachine("0/lxc/10", true)
-	assertMachine("0/kvm/4", true)
-	assertMachine("0/no-dash/0", false)
-	assertMachine("0/lxc/1/embedded/2", true)
-
-	assertMachineOrNewContainer := func(s string, expect bool) {
-		c.Assert(names.IsMachineOrNewContainer(s), gc.Equals, expect)
-	}
-	assertMachineOrNewContainer("0", true)
-	assertMachineOrNewContainer("00", false)
-	assertMachineOrNewContainer("1", true)
-	assertMachineOrNewContainer("0/lxc/0", true)
-	assertMachineOrNewContainer("lxc:0", true)
-	assertMachineOrNewContainer("lxc:lxc:0", false)
-	assertMachineOrNewContainer("kvm:0/lxc/1", true)
-	assertMachineOrNewContainer("lxc:", false)
-	assertMachineOrNewContainer(":lxc", false)
-	assertMachineOrNewContainer("0/lxc/", false)
-	assertMachineOrNewContainer("0/lxc", false)
-	assertMachineOrNewContainer("kvm:0/lxc", false)
-	assertMachine("0/lxc/00", false)
-	assertMachine("0/lxc/01", false)
-	assertMachineOrNewContainer("0/lxc/01", false)
-	assertMachineOrNewContainer("0/lxc/10", true)
-	assertMachineOrNewContainer("0/kvm/4", true)
-	assertMachine("0/lxc/1/embedded/2", true)
-}
-
 type attrs map[string]interface{}
 
 func (s *StateSuite) TestWatchEnvironConfig(c *gc.C) {