cmd/juju: added source flag to sync-tools

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)

[mue, 2013-07-10 08:54:13]

Please take a look.

[jameinel, 2013-07-10 10:59:09]

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#new...
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?

[mue, 2013-07-11 15:24:06]

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#new...
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.

[dimitern, 2013-07-11 20:40:40]

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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
cmd/juju/synctools.go:243: sort.Strings(list)
why are you sorting it?

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newco...
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#...
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#...
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#...
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#...
cmd/juju/synctools_test.go:216: file, err := os.Create(path)
why not ioutil.WriteFile instead of most of the block below?

[mue, 2013-07-12 08:34:15]

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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#...
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#...
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://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:213: dir, _ := filepath.Split(path)
On 2013/07/11 20:40:40, dimitern wrote:
> isn't dir the same as storagePath after this, use that instead? why the split?
> why not Base or Dir?

It's not the same. storagePath may be /foo, the tool name
tools/bar-1.2.3.tar.gz. So the dir will be /foo/tools. But using Dir() is good,
simply used Split() too often. ;)

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:216: file, err := os.Create(path)
On 2013/07/11 20:40:40, dimitern wrote:
> why not ioutil.WriteFile instead of most of the block below?

Aaargh, good catch. I had in mind that there's a simple helper, but somehow I
missed to use it. Thanks.

[dimitern, 2013-07-12 08:39:41]

Thanks for the changes, LGTM.