Merge lp:~themue/juju-core/033-sync-tools-source into lp:~go-bot/juju-core/trunk

[Frank Mueller]

cmd/juju: added source flag to sync-tools

The sync-tools command now allows to specify
a local directory as the source for the tools
to synchronize. If not specified otherwise
the standard EC2 HTTP source is taken.

https://codereview.appspot.com/11103043/

cmd/juju: added source flag to sync-tools

The sync-tools command now allows to specify
a local directory as the source for the tools
to synchronize. If not specified otherwise
the standard EC2 HTTP source is taken.

https://codereview.appspot.com/11103043/

[Frank Mueller (themue)]

Reviewers: mp+173884_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: added source flag to sync-tools

The sync-tools command now allows to specify
a local directory as the source for the tools
to synchronize. If not specified otherwise
the standard EC2 HTTP source is taken.

https://code.launchpad.net/~themue/juju-core/033-sync-tools-source/+merge/173884

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/juju/synctools.go
   M cmd/juju/synctools_test.go

[John A Meinel (jameinel)]

we probably need to think carefully about whether we need 'path' instead
of 'filepath' (if something is called URL I would be surprised to see
'\' in the string.)

otherwise LGTM
just minor stuff.

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools.go#newcode185
cmd/juju/synctools.go:185: if sourceFlagValue == "default" {
I would have actually expected "" to mean default, but I can see people
doing: --source=default as a way to be explicit.

Even so, I would probably like to have:

if sourceFlagValue == "" || sourceFlagValue == "default" {
}

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools.go#newcode191
cmd/juju/synctools.go:191: // fileStorageReader implements the
StorageReader accessing the local
fileStorageReader implements StorageReader backed by the local
filesystem

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools.go#newcode250
cmd/juju/synctools.go:250: }
I'm a little concerned about path.Join vs filepath.Join

On Windows, filepath.Join is going to return "\" paths, which is
inappropriate for something called a URL.

Since you can still access files via '/' paths, I'd rather see us just
use "path.Join" in here.

though I'm not sure what filepath.Clean does to know whether it munges
all of our '/' characters.

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools_test.go
File cmd/juju/synctools_test.go (right):

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools_test.go#newcode209
cmd/juju/synctools_test.go:209: func putBinary(c *C, s string, v
version.Binary) {
can you s/s/storagePath/ to be clear what this string is?

https://codereview.appspot.com/11103043/

[Frank Mueller (themue)]

Please take a look.

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools.go#newcode185
cmd/juju/synctools.go:185: if sourceFlagValue == "default" {
On 2013/07/10 10:59:09, jameinel wrote:
> I would have actually expected "" to mean default, but I can see
people doing:
> --source=default as a way to be explicit.

> Even so, I would probably like to have:

> if sourceFlagValue == "" || sourceFlagValue == "default" {
> }

Done.

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools.go#newcode191
cmd/juju/synctools.go:191: // fileStorageReader implements the
StorageReader accessing the local
On 2013/07/10 10:59:09, jameinel wrote:
> fileStorageReader implements StorageReader backed by the local
filesystem

Done.

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools.go#newcode250
cmd/juju/synctools.go:250: }
On 2013/07/10 10:59:09, jameinel wrote:
> I'm a little concerned about path.Join vs filepath.Join

> On Windows, filepath.Join is going to return "\" paths, which is
inappropriate
> for something called a URL.

> Since you can still access files via '/' paths, I'd rather see us just
use
> "path.Join" in here.

> though I'm not sure what filepath.Clean does to know whether it munges
all of
> our '/' characters.

Done. In our case it's relative unimportant as the method is only to
implement the interface and used internally in fileStorageReader.Get().

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools_test.go
File cmd/juju/synctools_test.go (right):

https://codereview.appspot.com/11103043/diff/1/cmd/juju/synctools_test.go#newcode209
cmd/juju/synctools_test.go:209: func putBinary(c *C, s string, v
version.Binary) {
On 2013/07/10 10:59:09, jameinel wrote:
> can you s/s/storagePath/ to be clear what this string is?

Done.

https://codereview.appspot.com/11103043/

[Dimiter Naydenov (dimitern)]

A few thoughts.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode50
cmd/juju/synctools.go:50: to avoid having to access data outside of the
local cloud.
please add an example of the two kinds of --source options: file path
and url.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode61
cmd/juju/synctools.go:61: f.StringVar(&c.source, "source", "default",
"chose a location on the file system or via http as source")
s/"default"/""/ (see below)

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode186
cmd/juju/synctools.go:186: if sourceFlagValue == "" || sourceFlagValue
== "default" {
"default" seems icky - how's this different from "" ? why do we need it?

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode206
cmd/juju/synctools.go:206: return nil, fmt.Errorf("specified source path
is no directory: %s", p)
s/no/not a/, also if you're saying "specified", please pass `path`
instead of `p`, since that what was passed before Clean (less confusing
for the user)

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode226
cmd/juju/synctools.go:226: pathlen := len(f.path) + 1
maybe len(f.path) + len("/") is less ambiguous.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode243
cmd/juju/synctools.go:243: sort.Strings(list)
why are you sorting it?

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode249
cmd/juju/synctools.go:249: return path.Join(f.path, name), nil
in this case it should be filepath.Join, I think

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go
File cmd/juju/synctools_test.go (right):

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode53
cmd/juju/synctools_test.go:53: // Create a local tool directory.
s/tool/tools/

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode106
cmd/juju/synctools_test.go:106: c.Assert(ctx, NotNil)
shouldn't you be testing for specific contents of ctx instead?

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode213
cmd/juju/synctools_test.go:213: dir, _ := filepath.Split(path)
isn't dir the same as storagePath after this, use that instead? why the
split? why not Base or Dir?

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode216
cmd/juju/synctools_test.go:216: file, err := os.Create(path)
why not ioutil.WriteFile instead of most of the block below?

https://codereview.appspot.com/11103043/

[Frank Mueller (themue)]

Please take a look.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode50
cmd/juju/synctools.go:50: to avoid having to access data outside of the
local cloud.
On 2013/07/11 20:40:40, dimitern wrote:
> please add an example of the two kinds of --source options: file path
and url.

Done, but only added a hint to the option to specify a source directory.
The idea of alternatively pass a HTTP location has been postponed.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode61
cmd/juju/synctools.go:61: f.StringVar(&c.source, "source", "default",
"chose a location on the file system or via http as source")
On 2013/07/11 20:40:40, dimitern wrote:
> s/"default"/""/ (see below)

Done, also changed wrong help text.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode186
cmd/juju/synctools.go:186: if sourceFlagValue == "" || sourceFlagValue
== "default" {
On 2013/07/11 20:40:40, dimitern wrote:
> "default" seems icky - how's this different from "" ? why do we need
it?

Idea has been to give better chance to make it explicit. But you're
right, it's not needed, so removed it.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode206
cmd/juju/synctools.go:206: return nil, fmt.Errorf("specified source path
is no directory: %s", p)
On 2013/07/11 20:40:40, dimitern wrote:
> s/no/not a/, also if you're saying "specified", please pass `path`
instead of
> `p`, since that what was passed before Clean (less confusing for the
user)

Done.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode226
cmd/juju/synctools.go:226: pathlen := len(f.path) + 1
On 2013/07/11 20:40:40, dimitern wrote:
> maybe len(f.path) + len("/") is less ambiguous.

Simply added a comment. ;)

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode243
cmd/juju/synctools.go:243: sort.Strings(list)
On 2013/07/11 20:40:40, dimitern wrote:
> why are you sorting it?

See interface specification: "... in alphabetical order ...".

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode249
cmd/juju/synctools.go:249: return path.Join(f.path, name), nil
On 2013/07/11 20:40:40, dimitern wrote:
> in this case it should be filepath.Join, I think

Had it before, but see discussion with John.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go
File cmd/juju/synctools_test.go (right):

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode53
cmd/juju/synctools_test.go:53: // Create a local tool directory.
On 2013/07/11 20:40:40, dimitern wrote:
> s/tool/tools/

Done.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode106
cmd/juju/synctools_test.go:106: c.Assert(ctx, NotNil)
On 2013/07/11 20:40:40, dimitern wrote:
> shouldn't you be testing for specific contents of ctx instead?

See tests below, simply done it the same way. Don't know the original
intention, but this way nobody should ask why the test differs from
those below.

https...

[Dimiter Naydenov (dimitern)]

Thanks for the changes, LGTM.

https://codereview.appspot.com/11103043/

=== modified file 'cmd/juju/synctools.go'
--- cmd/juju/synctools.go	2013-06-20 06:42:25 +0000
+++ cmd/juju/synctools.go	2013-07-12 08:35:41 +0000
@@ -7,6 +7,10 @@
 	"bytes"
 	"fmt"
 	"io"
+	"os"
+	"path"
+	"path/filepath"
+	"sort"
 
 	"launchpad.net/gnuflag"
 	"launchpad.net/juju-core/cmd"
@@ -29,6 +33,7 @@
 	dryRun       bool
 	publicBucket bool
 	dev          bool
+	source       string
 }
 
 var _ cmd.Command = (*SyncToolsCommand)(nil)
@@ -40,9 +45,12 @@
 		Doc: `
 This copies the Juju tools tarball from the official bucket into
 your environment. This is generally done when you want Juju to be able
-to run without having to access Amazon. Sometimes this is because the
-environment does not have public access, and sometimes you just want
-to avoid having to access data outside of the local cloud.
+to run without having to access Amazon. Alternatively you can specify
+a local directory as source.
+
+Sometimes this is because the environment does not have public access, 
+and sometimes you just want to avoid having to access data outside of 
+the local cloud.
 `,
 	}
 }
@@ -53,6 +61,7 @@
 	f.BoolVar(&c.dryRun, "dry-run", false, "don't copy, just print what would be copied")
 	f.BoolVar(&c.dev, "dev", false, "consider development versions as well as released ones")
 	f.BoolVar(&c.publicBucket, "public", false, "write to the public-bucket of the account, instead of the bucket private to the environment.")
+	f.StringVar(&c.source, "source", "", "chose a location on the file system as source")
 
 	// BUG(lp:1163164)  jam 2013-04-2 we would like to add a "source"
 	// location, rather than only copying from us-east-1
@@ -106,7 +115,11 @@
 }
 
 func (c *SyncToolsCommand) Run(ctx *cmd.Context) error {
-	sourceStorage := ec2.NewHTTPStorageReader(defaultToolsLocation)
+	sourceStorage, err := selectSourceStorage(c.source)
+	if err != nil {
+		log.Errorf("unable to select source: %v", err)
+		return err
+	}
 	targetEnv, err := environs.NewFromName(c.EnvName)
 	if err != nil {
 		log.Errorf("unable to read %q from environment", c.EnvName)
@@ -170,3 +183,72 @@
 	fmt.Fprintf(ctx.Stderr, "copied %d tools\n", len(missing))
 	return nil
 }
+
+// selectSourceStorage returns a storage reader based on the passed source flag.
+func selectSourceStorage(sourceFlagValue string) (environs.StorageReader, error) {
+	if sourceFlagValue == "" {
+		return ec2.NewHTTPStorageReader(defaultToolsLocation), nil
+	}
+	return newFileStorageReader(sourceFlagValue)
+}
+
+// fileStorageReader implements StorageReader backed by the local filesystem.
+type fileStorageReader struct {
+	path string
+}
+
+// newFileStorageReader return a new storage reader for
+// a directory inside the local file system.
+func newFileStorageReader(path string) (environs.StorageReader, error) {
+	p := filepath.Clean(path)
+	fi, err := os.Stat(p)
+	if err != nil {
+		return nil, err
+	}
+	if !fi.Mode().IsDir() {
+		return nil, fmt.Errorf("specified source path is not a directory: %s", path)
+	}
+	return &fileStorageReader{p}, nil
+}
+
+// Get implements environs.StorageReader.Get.
+func (f *fileStorageReader) Get(name string) (io.ReadCloser, error) {
+	filename, err := f.URL(name)
+	if err != nil {
+		return nil, err
+	}
+	file, err := os.Open(filename)
+	if err != nil {
+		return nil, err
+	}
+	return file, nil
+}
+
+// List implements environs.StorageReader.List.
+func (f *fileStorageReader) List(prefix string) ([]string, error) {
+	// Add one for the missing path separator.
+	pathlen := len(f.path) + 1
+	pattern := filepath.Join(f.path, prefix+"*")
+	matches, err := filepath.Glob(pattern)
+	if err != nil {
+		return nil, err
+	}
+	list := []string{}
+	for _, match := range matches {
+		fi, err := os.Stat(match)
+		if err != nil {
+			return nil, err
+		}
+		if !fi.Mode().IsDir() {
+			filename := match[pathlen:]
+			list = append(list, filename)
+		}
+	}
+	sort.Strings(list)
+	return list, nil
+}
+
+// URL implements environs.StorageReader.URL.
+func (f *fileStorageReader) URL(name string) (string, error) {
+	return path.Join(f.path, name), nil
+}

=== modified file 'cmd/juju/synctools_test.go'
--- cmd/juju/synctools_test.go	2013-06-20 13:42:24 +0000
+++ cmd/juju/synctools_test.go	2013-07-12 08:35:41 +0000
@@ -4,6 +4,10 @@
 package main
 
 import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/environs"
@@ -21,6 +25,7 @@
 	origVersion  version.Binary
 	origLocation string
 	storage      *envtesting.EC2HTTPTestStorage
+	localStorage string
 }
 
 func (s *syncToolsSuite) SetUpTest(c *C) {
@@ -42,12 +47,17 @@
 	c.Assert(err, IsNil)
 	envtesting.RemoveAllTools(c, s.targetEnv)
 
-	// Create a source environment and populate its public tools.
+	// Create a source storage.
 	s.storage, err = envtesting.NewEC2HTTPTestStorage("127.0.0.1")
 	c.Assert(err, IsNil)
 
+	// Create a local tools directory.
+	s.localStorage = c.MkDir()
+
+	// Populate both with the public tools.
 	for _, vers := range vAll {
 		s.storage.PutBinary(vers)
+		putBinary(c, s.localStorage, vers)
 	}
 
 	s.origLocation = defaultToolsLocation
@@ -91,6 +101,20 @@
 	c.Assert(err, Equals, tools.ErrNoTools)
 }
 
+func (s *syncToolsSuite) TestCopyNewestFromFilesystem(c *C) {
+	ctx, err := runSyncToolsCommand(c, "-e", "test-target", "--source", s.localStorage)
+	c.Assert(err, IsNil)
+	c.Assert(ctx, NotNil)
+
+	// Newest released v1 tools made available to target env.
+	targetTools, err := environs.FindAvailableTools(s.targetEnv, 1)
+	c.Assert(err, IsNil)
+	assertToolsList(c, targetTools, v100all)
+
+	// Public bucket was not touched.
+	assertEmpty(c, s.targetEnv.PublicStorage())
+}
+
 func (s *syncToolsSuite) TestCopyNewestFromDummy(c *C) {
 	ctx, err := runSyncToolsCommand(c, "-e", "test-target")
 	c.Assert(err, IsNil)
@@ -181,3 +205,15 @@
 	v200p64 = version.MustParseBinary("2.0.0-precise-amd64")
 	vAll    = append(v1all, v200p64)
 )
+
+// putBinary stores a faked binary in the test directory.
+func putBinary(c *C, storagePath string, v version.Binary) {
+	data := v.String()
+	name := tools.StorageName(v)
+	filename := filepath.Join(storagePath, name)
+	dir := filepath.Dir(filename)
+	err := os.MkdirAll(dir, 0755)
+	c.Assert(err, IsNil)
+	err = ioutil.WriteFile(filename, []byte(data), 0666)
+	c.Assert(err, IsNil)
+}