Code review comment for lp:~wallyworld/juju-core/fix-tools-sources

Reviewers: mp+187959_code.launchpad.net,

Message:
Please take a look.

Description:
Fix tools sources for local and azure

The local provider did not implement CustomToolsSources,
so when it tried to get tools via simplestreams, it
failed and then used the legacy fallback. But this meant
that checksum and size info was missing.

The azure provider did implement CustomToolsSources, but
until the official tools repository comes online, the
tools sources do need to include the public container
from which tools can be fetched, or else simplestreams
will fail and revert to legacy tools code.

https://code.launchpad.net/~wallyworld/juju-core/fix-tools-sources/+merge/187959

(do not edit description out of merge proposal)

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

Affected files (+46, -2 lines):
   A [revision details]
   M provider/azure/environ.go
   M provider/azure/environ_test.go
   M provider/local/environ.go
   M provider/local/local_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-20130926184157-5d305cr4rb895lnt
+New revision: <email address hidden>

Index: provider/azure/environ.go
=== modified file 'provider/azure/environ.go'
--- provider/azure/environ.go 2013-09-26 09:18:36 +0000
+++ provider/azure/environ.go 2013-09-27 00:23:48 +0000
@@ -918,7 +918,11 @@
  // GetToolsSources returns a list of sources which are used to search for
simplestreams tools metadata.
  func (env *azureEnviron) GetToolsSources() ([]simplestreams.DataSource,
error) {
   // Add the simplestreams source off the control bucket.
- return
[]simplestreams.DataSource{storage.NewStorageSimpleStreamsDataSource(env.Storage(),
storage.BaseToolsPath)}, nil
+ sources := []simplestreams.DataSource{
+ storage.NewStorageSimpleStreamsDataSource(env.Storage(),
storage.BaseToolsPath),
+ simplestreams.NewURLDataSource(
+ "https://jujutools.blob.core.windows.net/juju-tools/tools",
simplestreams.VerifySSLHostnames)}
+ return sources, nil
  }

  // getImageStream returns the name of the simplestreams stream from which

Index: provider/azure/environ_test.go
=== modified file 'provider/azure/environ_test.go'
--- provider/azure/environ_test.go 2013-09-26 12:28:28 +0000
+++ provider/azure/environ_test.go 2013-09-27 00:23:48 +0000
@@ -1313,6 +1313,9 @@

   sources, err := tools.GetMetadataSources(env)
   c.Assert(err, gc.IsNil)
- c.Assert(len(sources), gc.Equals, 1)
+ c.Assert(len(sources), gc.Equals, 2)
   assertSourceContents(c, sources[0], "filename", data)
+ url, err := sources[1].URL("")
+ c.Assert(err, gc.IsNil)
+ c.Assert(url,
gc.Equals, "https://jujutools.blob.core.windows.net/juju-tools/tools/")
  }

Index: provider/local/environ.go
=== modified file 'provider/local/environ.go'
--- provider/local/environ.go 2013-09-26 09:18:36 +0000
+++ provider/local/environ.go 2013-09-27 00:23:48 +0000
@@ -23,7 +23,9 @@
   "launchpad.net/juju-core/environs/config"
   "launchpad.net/juju-core/environs/filestorage"
   "launchpad.net/juju-core/environs/httpstorage"
+ "launchpad.net/juju-core/environs/simplestreams"
   "launchpad.net/juju-core/environs/storage"
+ envtools "launchpad.net/juju-core/environs/tools"
   coreerrors "launchpad.net/juju-core/errors"
   "launchpad.net/juju-core/instance"
   "launchpad.net/juju-core/juju/osenv"
@@ -54,6 +56,9 @@
  // localEnviron implements Environ.
  var _ environs.Environ = (*localEnviron)(nil)

+// localEnviron implements SupportsCustomSources.
+var _ envtools.SupportsCustomSources = (*localEnviron)(nil)
+
  type localEnviron struct {
   localMutex sync.Mutex
   config *environConfig
@@ -63,6 +68,13 @@
   containerManager lxc.ContainerManager
  }

+// GetToolsSources returns a list of sources which are used to search for
simplestreams tools metadata.
+func (e *localEnviron) GetToolsSources() ([]simplestreams.DataSource,
error) {
+ // Add the simplestreams source off the control bucket.
+ return []simplestreams.DataSource{
+ storage.NewStorageSimpleStreamsDataSource(e.Storage(),
storage.BaseToolsPath)}, nil
+}
+
  // Name is specified in the Environ interface.
  func (env *localEnviron) Name() string {
   return env.name

Index: provider/local/local_test.go
=== modified file 'provider/local/local_test.go'
--- provider/local/local_test.go 2013-09-20 02:53:59 +0000
+++ provider/local/local_test.go 2013-09-27 00:23:48 +0000
@@ -4,13 +4,17 @@
  package local_test

  import (
+ "strings"
   stdtesting "testing"

   gc "launchpad.net/gocheck"

   "launchpad.net/juju-core/environs"
+ envtesting "launchpad.net/juju-core/environs/testing"
+ "launchpad.net/juju-core/environs/tools"
   "launchpad.net/juju-core/provider"
   "launchpad.net/juju-core/provider/local"
+ jc "launchpad.net/juju-core/testing/checkers"
   "launchpad.net/juju-core/testing/testbase"
  )

@@ -20,12 +24,31 @@

  type localSuite struct {
   testbase.LoggingSuite
+ envtesting.ToolsFixture
  }

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

+func (s *localSuite) SetUpTest(c *gc.C) {
+ s.LoggingSuite.SetUpTest(c)
+ s.ToolsFixture.SetUpTest(c)
+}
+
  func (*localSuite) TestProviderRegistered(c *gc.C) {
   provider, error := environs.Provider(provider.Local)
   c.Assert(error, gc.IsNil)
   c.Assert(provider, gc.DeepEquals, local.Provider)
  }
+
+func (s *localSuite) TestGetToolsMetadataSources(c *gc.C) {
+ testConfig := minimalConfig(c)
+ environ, err := local.Provider.Open(testConfig)
+ c.Assert(err, gc.IsNil)
+
+ sources, err := tools.GetMetadataSources(environ)
+ c.Assert(err, gc.IsNil)
+ c.Assert(len(sources), gc.Equals, 1)
+ url, err := sources[0].URL("")
+ c.Assert(err, gc.IsNil)
+ c.Assert(strings.Contains(url, "/tools"), jc.IsTrue)
+}