Merge lp:~mattyw/juju-core/gocheck_included_in_build_fix into lp:~go-bot/juju-core/trunk

[Matthew Williams]

Bug fix for lp:1243942

Move test package into testing directory to prevent gocheck being included in build

https://codereview.appspot.com/15690053/

Bug fix for lp:1243942

Move test package into testing directory to prevent gocheck being included in build

https://codereview.appspot.com/15690053/

[Matthew Williams (mattyw)]

Reviewers: mp+192411_code.launchpad.net,

Message:
Please take a look.

Description:
Bug fix for lp:1243924

Bug fix for lp:1243924

https://code.launchpad.net/~mattyw/juju-core/gocheck_included_in_build_fix/+merge/192411

(do not edit description out of merge proposal)

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

[Andrew Wilkins (axwalk)]

On 2013/10/23 22:05:20, matthew.williams wrote:
> Please take a look.

The lp: points to "Open Rails Tracker". I don't think that's right? :)

Just as a general thing, it's useful to include a description of the
fix, as well as a pointer to the bug.

https://codereview.appspot.com/15690053/

[Matthew Williams (mattyw)]

On 2013/10/23 22:42:20, axw wrote:
> On 2013/10/23 22:05:20, matthew.williams wrote:
> > Please take a look.

> The lp: points to "Open Rails Tracker". I don't think that's right? :)

> Just as a general thing, it's useful to include a description of the
fix, as
> well as a pointer to the bug.

My bad, sorry.

Just fixing

https://codereview.appspot.com/15690053/

[Matthew Williams (mattyw)]

Please take a look.

https://codereview.appspot.com/15690053/

[Andrew Wilkins (axwalk)]

On 2013/10/23 22:58:47, mattyw wrote:
> On 2013/10/23 22:42:20, axw wrote:
> > On 2013/10/23 22:05:20, matthew.williams wrote:
> > > Please take a look.
> >
> > The lp: points to "Open Rails Tracker". I don't think that's right?
:)
> >
> > Just as a general thing, it's useful to include a description of the
fix, as
> > well as a pointer to the bug.

> My bad, sorry.

> Just fixing

No worries! Thanks for fixing the issue.

https://codereview.appspot.com/15690053/

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/15690053/diff/30001/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/15690053/diff/30001/cmd/jujud/machine_test.go#newcode38
cmd/jujud/machine_test.go:38:
Should that be s/test/testing/? IMHO, an alias of lxctesting would be
useful, like envtesting.

https://codereview.appspot.com/15690053/diff/30001/container/lxc/lxc.go
File container/lxc/lxc.go (right):

https://codereview.appspot.com/15690053/diff/30001/container/lxc/lxc.go#newcode34
container/lxc/lxc.go:34:
There's a convention for vars that are exported just for tests. Instead
of this, we do "var containerDir = ..." in this file, and then in a new
file called "export_test.go" have "var ContainerDir = containerDir".
That makes it a bit more obvious why it's exported. If you do that, all
the other case changes can (should) be reverted.

https://codereview.appspot.com/15690053/diff/30001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/15690053/diff/30001/container/lxc/lxc_test.go#newcode35
container/lxc/lxc_test.go:35:
testing/lxctesting?

https://codereview.appspot.com/15690053/diff/30001/container/lxc/testing/test.go
File container/lxc/testing/test.go (right):

https://codereview.appspot.com/15690053/diff/30001/container/lxc/testing/test.go#newcode4
container/lxc/testing/test.go:4: // Functions defined in this file
should *ONLY* be used for testing. These
I think this comment is kinda redundant now; this can be inferred from
its package.

https://codereview.appspot.com/15690053/diff/30001/provider/local/environprovider_test.go
File provider/local/environprovider_test.go (right):

https://codereview.appspot.com/15690053/diff/30001/provider/local/environprovider_test.go#newcode16
provider/local/environprovider_test.go:16:
testing/lxctesting?

https://codereview.appspot.com/15690053/diff/30001/worker/provisioner/lxc-broker_test.go
File worker/provisioner/lxc-broker_test.go (right):

https://codereview.appspot.com/15690053/diff/30001/worker/provisioner/lxc-broker_test.go#newcode33
worker/provisioner/lxc-broker_test.go:33:
testing/lxctesting?

https://codereview.appspot.com/15690053/

[Matthew Williams (mattyw)]

Please take a look.

https://codereview.appspot.com/15690053/

[Matthew Williams (mattyw)]

Please take a look.

https://codereview.appspot.com/15690053/

[Matthew Williams (mattyw)]

On 2013/10/28 13:26:40, mattyw wrote:
> Please take a look.

I'm not able to use the export_test model because the lxc testing
package is used by tests in other packages

https://codereview.appspot.com/15690053/

[Andrew Wilkins (axwalk)]

On 2013/10/28 13:58:40, mattyw wrote:
> On 2013/10/28 13:26:40, mattyw wrote:
> > Please take a look.

> I'm not able to use the export_test model because the lxc testing
package is
> used by tests in other packages

Ah, of course, sorry. LGTM. Thanks for fixing this.

https://codereview.appspot.com/15690053/

[John A Meinel (jameinel)]

axw, can you confirm that this is fully approved and then land it for Matthew? I don't think he has rights to set the MP to Approved.

[John A Meinel (jameinel)]

Just for my 2c, it LGTM, but I'd like the original conversation to finish it.

review: Approve

[Matthew Williams (mattyw)]

> Just for my 2c, it LGTM, but I'd like the original conversation to finish it.

Thanks John.

I'm afraid I don't know what you mean by "I'd like the original conversation to finish it."

[Andrew Wilkins (axwalk)]

@mattyw: I think he just meant he wants me to ack/nack.

Anyway, still LGTM - I didn't occur to me you didn't have access. Approving.

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in container/lxc/lxc_test.go

[John A Meinel (jameinel)]

The conflict means you need to merge trunk again and resolve it, and then
push back to Launchpad. Let us know when you finish and we'll submit it
again.

John
=:->
On Nov 11, 2013 5:29 AM, "Go Bot" <email address hidden> wrote:

> The proposal to merge lp:~mattyw/juju-core/gocheck_included_in_build_fix
> into lp:juju-core has been updated.
>
> Status: Approved => Needs review
>
> For more details, see:
>
> https://code.launchpad.net/~mattyw/juju-core/gocheck_included_in_build_fix/+merge/192411
> --
>
> https://code.launchpad.net/~mattyw/juju-core/gocheck_included_in_build_fix/+merge/192411
> You are reviewing the proposed merge of
> lp:~mattyw/juju-core/gocheck_included_in_build_fix into lp:juju-core.
>

[Matthew Williams (mattyw)]

Please take a look.

https://codereview.appspot.com/15690053/

[Matthew Williams (mattyw)]

Please take a look.

https://codereview.appspot.com/15690053/

=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2013-11-05 08:06:56 +0000
+++ cmd/jujud/machine_test.go	2013-11-11 10:57:52 +0000
@@ -13,7 +13,7 @@
 	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/cmd"
-	"launchpad.net/juju-core/container/lxc"
+	lxctesting "launchpad.net/juju-core/container/lxc/testing"
 	envtesting "launchpad.net/juju-core/environs/testing"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
@@ -35,7 +35,7 @@
 
 type MachineSuite struct {
 	agentSuite
-	lxc.TestSuite
+	lxctesting.TestSuite
 }
 
 var _ = gc.Suite(&MachineSuite{})

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2013-10-31 21:27:57 +0000
+++ container/lxc/lxc.go	2013-11-11 10:57:52 +0000
@@ -24,12 +24,12 @@
 
 var (
 	defaultTemplate     = "ubuntu-cloud"
-	containerDir        = "/var/lib/juju/containers"
-	removedContainerDir = "/var/lib/juju/removed-containers"
-	lxcContainerDir     = "/var/lib/lxc"
-	lxcRestartDir       = "/etc/lxc/auto"
-	lxcObjectFactory    = golxc.Factory()
 	aptHTTPProxyRE      = regexp.MustCompile(`(?i)^Acquire::HTTP::Proxy\s+"([^"]+)";$`)
+	ContainerDir        = "/var/lib/juju/containers"
+	RemovedContainerDir = "/var/lib/juju/removed-containers"
+	LxcContainerDir     = "/var/lib/lxc"
+	LxcRestartDir       = "/etc/lxc/auto"
+	LxcObjectFactory    = golxc.Factory()
 )
 
 const (
@@ -115,7 +115,7 @@
 	// Note here that the lxcObjectFacotry only returns a valid container
 	// object, and doesn't actually construct the underlying lxc container on
 	// disk.
-	container := lxcObjectFactory.New(name)
+	container := LxcObjectFactory.New(name)
 
 	// Create the cloud-init.
 	directory := jujuContainerDirectory(name)
@@ -156,7 +156,7 @@
 	}
 	logger.Tracef("lxc container created")
 	// Now symlink the config file into the restart directory.
-	containerConfigFile := filepath.Join(lxcContainerDir, name, "config")
+	containerConfigFile := filepath.Join(LxcContainerDir, name, "config")
 	if err := os.Symlink(containerConfigFile, restartSymlink(name)); err != nil {
 		return nil, err
 	}
@@ -181,7 +181,7 @@
 
 func (manager *containerManager) StopContainer(instance instance.Instance) error {
 	name := string(instance.Id())
-	container := lxcObjectFactory.New(name)
+	container := LxcObjectFactory.New(name)
 	// Remove the autostart link.
 	if err := os.Remove(restartSymlink(name)); err != nil {
 		logger.Errorf("failed to remove restart symlink: %v", err)
@@ -193,12 +193,12 @@
 	}
 
 	// Move the directory.
-	logger.Tracef("create old container dir: %s", removedContainerDir)
-	if err := os.MkdirAll(removedContainerDir, 0755); err != nil {
+	logger.Tracef("create old container dir: %s", RemovedContainerDir)
+	if err := os.MkdirAll(RemovedContainerDir, 0755); err != nil {
 		logger.Errorf("failed to create removed container directory: %v", err)
 		return err
 	}
-	removedDir, err := uniqueDirectory(removedContainerDir, name)
+	removedDir, err := uniqueDirectory(RemovedContainerDir, name)
 	if err != nil {
 		logger.Errorf("was not able to generate a unique directory: %v", err)
 		return err
@@ -211,7 +211,7 @@
 }
 
 func (manager *containerManager) ListContainers() (result []instance.Instance, err error) {
-	containers, err := lxcObjectFactory.List()
+	containers, err := LxcObjectFactory.List()
 	if err != nil {
 		logger.Errorf("failed getting all instances: %v", err)
 		return
@@ -235,17 +235,17 @@
 }
 
 func jujuContainerDirectory(containerName string) string {
-	return filepath.Join(containerDir, containerName)
+	return filepath.Join(ContainerDir, containerName)
 }
 
 const internalLogDirTemplate = "%s/%s/rootfs/var/log/juju"
 
 func internalLogDir(containerName string) string {
-	return fmt.Sprintf(internalLogDirTemplate, lxcContainerDir, containerName)
+	return fmt.Sprintf(internalLogDirTemplate, LxcContainerDir, containerName)
 }
 
 func restartSymlink(name string) string {
-	return filepath.Join(lxcRestartDir, name+".conf")
+	return filepath.Join(LxcRestartDir, name+".conf")
 }
 
 const localConfig = `%s

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2013-10-31 21:27:57 +0000
+++ container/lxc/lxc_test.go	2013-11-11 10:57:52 +0000
@@ -16,6 +16,7 @@
 	"launchpad.net/loggo"
 
 	"launchpad.net/juju-core/container/lxc"
+	lxctesting "launchpad.net/juju-core/container/lxc/testing"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/instance"
 	instancetest "launchpad.net/juju-core/instance/testing"
@@ -31,7 +32,7 @@
 }
 
 type LxcSuite struct {
-	lxc.TestSuite
+	lxctesting.TestSuite
 }
 
 var _ = gc.Suite(&LxcSuite{})

=== removed file 'container/lxc/test.go'
--- container/lxc/test.go	2013-09-22 23:29:33 +0000
+++ container/lxc/test.go	1970-01-01 00:00:00 +0000
@@ -1,40 +0,0 @@
-// Copyright 2013 Canonical Ltd.
-// Licensed under the AGPLv3, see LICENCE file for details.
-
-// Functions defined in this file should *ONLY* be used for testing.  These
-// functions are exported for testing purposes only, and shouldn't be called
-// from code that isn't in a test file.
-
-package lxc
-
-import (
-	gc "launchpad.net/gocheck"
-
-	"launchpad.net/juju-core/container/lxc/mock"
-	"launchpad.net/juju-core/testing/testbase"
-)
-
-// TestSuite replaces the lxc factory that the broker uses with a mock
-// implementation.
-type TestSuite struct {
-	testbase.LoggingSuite
-	Factory      mock.ContainerFactory
-	ContainerDir string
-	RemovedDir   string
-	LxcDir       string
-	RestartDir   string
-}
-
-func (s *TestSuite) SetUpTest(c *gc.C) {
-	s.LoggingSuite.SetUpTest(c)
-	s.ContainerDir = c.MkDir()
-	s.PatchValue(&containerDir, s.ContainerDir)
-	s.RemovedDir = c.MkDir()
-	s.PatchValue(&removedContainerDir, s.RemovedDir)
-	s.LxcDir = c.MkDir()
-	s.PatchValue(&lxcContainerDir, s.LxcDir)
-	s.RestartDir = c.MkDir()
-	s.PatchValue(&lxcRestartDir, s.RestartDir)
-	s.Factory = mock.MockFactory()
-	s.PatchValue(&lxcObjectFactory, s.Factory)
-}

=== added directory 'container/lxc/testing'
=== added file 'container/lxc/testing/test.go'
--- container/lxc/testing/test.go	1970-01-01 00:00:00 +0000
+++ container/lxc/testing/test.go	2013-11-11 10:57:52 +0000
@@ -0,0 +1,37 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package testing
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/container/lxc"
+	"launchpad.net/juju-core/container/lxc/mock"
+	"launchpad.net/juju-core/testing/testbase"
+)
+
+// TestSuite replaces the lxc factory that the broker uses with a mock
+// implementation.
+type TestSuite struct {
+	testbase.LoggingSuite
+	Factory      mock.ContainerFactory
+	ContainerDir string
+	RemovedDir   string
+	LxcDir       string
+	RestartDir   string
+}
+
+func (s *TestSuite) SetUpTest(c *gc.C) {
+	s.LoggingSuite.SetUpTest(c)
+	s.ContainerDir = c.MkDir()
+	s.PatchValue(&lxc.ContainerDir, s.ContainerDir)
+	s.RemovedDir = c.MkDir()
+	s.PatchValue(&lxc.RemovedContainerDir, s.RemovedDir)
+	s.LxcDir = c.MkDir()
+	s.PatchValue(&lxc.LxcContainerDir, s.LxcDir)
+	s.RestartDir = c.MkDir()
+	s.PatchValue(&lxc.LxcRestartDir, s.RestartDir)
+	s.Factory = mock.MockFactory()
+	s.PatchValue(&lxc.LxcObjectFactory, s.Factory)
+}

=== modified file 'provider/local/environprovider_test.go'
--- provider/local/environprovider_test.go	2013-10-02 01:27:45 +0000
+++ provider/local/environprovider_test.go	2013-11-11 10:57:52 +0000
@@ -7,13 +7,13 @@
 	gc "launchpad.net/gocheck"
 	"launchpad.net/loggo"
 
-	"launchpad.net/juju-core/container/lxc"
+	lxctesting "launchpad.net/juju-core/container/lxc/testing"
 	"launchpad.net/juju-core/provider/local"
 	"launchpad.net/juju-core/testing"
 )
 
 type baseProviderSuite struct {
-	lxc.TestSuite
+	lxctesting.TestSuite
 	home    *testing.FakeHome
 	restore func()
 }

=== modified file 'worker/provisioner/lxc-broker_test.go'
--- worker/provisioner/lxc-broker_test.go	2013-10-31 21:36:40 +0000
+++ worker/provisioner/lxc-broker_test.go	2013-11-11 10:57:52 +0000
@@ -13,8 +13,8 @@
 
 	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/constraints"
-	"launchpad.net/juju-core/container/lxc"
 	"launchpad.net/juju-core/container/lxc/mock"
+	lxctesting "launchpad.net/juju-core/container/lxc/testing"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
@@ -31,7 +31,7 @@
 )
 
 type lxcSuite struct {
-	lxc.TestSuite
+	lxctesting.TestSuite
 	events chan mock.Event
 }