launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/juju-core/uploadtools into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

Commit message:
Always upload the tools.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~rvb/juju-core/uploadtools/+merge/157610

In the MAAS provider, we should always upload the tools as part of the Bootstrap process because they cannot be shared between environment.  This branch changes the Bootstrap() so that the tools will always be uploaded for the series defined in the environment.  I think this is the right thing to do (this is also confirmed by testing) but I added notes to remind us to check that this is sane with the juju-core team when this will be landed in juju-core trunk.

Note that 'uploadTools' was a parameter passed down to Bootstrap() a few weeks ago, now it's up to the provider to decide what it should do.
-- 
https://code.launchpad.net/~rvb/juju-core/uploadtools/+merge/157610
Your team MAAS Maintainers is requested to review the proposed merge of lp:~rvb/juju-core/uploadtools into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-04-05 10:38:15 +0000
+++ environs/maas/environ.go	2013-04-08 09:33:24 +0000
@@ -98,7 +98,9 @@
 // uploadTools builds the current version of the juju tools and uploads them
 // to the environment's Storage.
 func (env *maasEnviron) uploadTools() (*state.Tools, error) {
-	tools, err := environs.PutTools(env.Storage(), nil)
+	// Also upload the tools for the default series taken from the environment.
+	// TODO: Check that this is sane with the juju-core team.
+	tools, err := environs.PutTools(env.Storage(), nil, env.Config().DefaultSeries())
 	if err != nil {
 		return nil, fmt.Errorf("cannot upload tools: %v", err)
 	}
@@ -184,8 +186,6 @@
 
 // Bootstrap is specified in the Environ interface.
 func (env *maasEnviron) Bootstrap(cons constraints.Value, stateServerCert, stateServerKey []byte) error {
-	// TODO: Fix this quick hack.  uploadTools is a now-obsolete parameter.
- 	uploadTools := false
 
 	// This was all cargo-culted from the EC2 provider.
 	password := env.Config().AdminSecret()
@@ -197,12 +197,10 @@
 	if err != nil {
 		return err
 	}
+	// MAAS does not support public storage: always upload the tools.
+	// TODO: Check that this is sane with the juju-core team.
 	var tools *state.Tools
-	if uploadTools {
-		tools, err = env.uploadTools()
-	} else {
-		tools, err = env.findTools()
-	}
+	tools, err = env.uploadTools()
 	if err != nil {
 		return err
 	}

=== modified file 'environs/maas/environ_test.go'
--- environs/maas/environ_test.go	2013-04-08 01:50:24 +0000
+++ environs/maas/environ_test.go	2013-04-08 09:33:24 +0000
@@ -8,7 +8,6 @@
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
-	envtesting "launchpad.net/juju-core/environs/testing"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/version"
@@ -62,11 +61,6 @@
 	suite.testMAASObject.TestServer.NewFile("provider-state", []byte("test file content"))
 }
 
-func (suite *EnvironSuite) setupFakeTools(c *C) {
-	storage := NewStorage(suite.environ)
-	envtesting.PutFakeTools(c, storage)
-}
-
 func (EnvironSuite) TestSetConfigUpdatesConfig(c *C) {
 	cfg := getTestConfig("test env", "http://maas2.example.com";, "a:b:c", "secret")
 	env, err := NewEnviron(cfg)
@@ -200,7 +194,6 @@
 // TODO: this test fails from time to time: we need to investigate what's
 // going on (also see the additional remarks below).
 func (suite *EnvironSuite) DisableTestStartInstanceStartsInstance(c *C) {
-	suite.setupFakeTools(c)
 	env := suite.makeEnviron()
 	suite.setupFakeProviderStateFile(c)
 	suite.testMAASObject.TestServer.NewNode(`{"system_id": "node1", "hostname": "host1"}`)
@@ -336,7 +329,6 @@
 // at the time of writing that would require more support from gomaasapi's
 // testing service than we have.
 func (suite *EnvironSuite) TestBootstrapSucceeds(c *C) {
-	suite.setupFakeTools(c)
 	env := suite.makeEnviron()
 	suite.testMAASObject.TestServer.NewNode(`{"system_id": "thenode"}`)
 	cert := []byte{1, 2, 3}
@@ -347,7 +339,6 @@
 }
 
 func (suite *EnvironSuite) TestBootstrapFailsIfNoNodes(c *C) {
-	suite.setupFakeTools(c)
 	env := suite.makeEnviron()
 	cert := []byte{1, 2, 3}
 	key := []byte{4, 5, 6}
@@ -357,8 +348,19 @@
 	c.Check(err, ErrorMatches, ".*409.*")
 }
 
+func (suite *EnvironSuite) TestBootstrapUploadsTools(c *C) {
+	env := suite.makeEnviron()
+	suite.testMAASObject.TestServer.NewNode(`{"system_id": "thenode"}`)
+	cert := []byte{1, 2, 3}
+	key := []byte{4, 5, 6}
+	err := env.Bootstrap(constraints.Value{}, cert, key)
+	c.Assert(err, IsNil)
+
+	_, err = env.findTools()
+	c.Assert(err, IsNil)
+}
+
 func (suite *EnvironSuite) TestBootstrapIntegratesWithEnvirons(c *C) {
-	suite.setupFakeTools(c)
 	env := suite.makeEnviron()
 	suite.testMAASObject.TestServer.NewNode(`{"system_id": "bootstrapnode"}`)