sync: pull out sync tools logic

sync: pull out sync tools logic

The is the first of a number of CLs regarding
autosync. Here the logic for the synchronization
of the tools moved from the explicit command to
the package environs/sync and the command has
been changed to use this package. In a later
step the bootstrap command shall be changed to
use this logic too in case the need for an
automatic synchronization is detected.

https://code.launchpad.net/~themue/juju-core/034-pull-out-sync-tools-logic/+merge/175893

(do not edit description out of merge proposal)

[mue, 2013-07-19 16:58:03]

Please take a look.

[rog, 2013-07-23 11:53:41]

LGTM with a logging done better and a few other minor suggestions.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go
File environs/sync/sync.go (right):

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode27
environs/sync/sync.go:27: var defaultToolsLocation string =
"https://juju-dist.s3.amazonaws.com/"
s/string = //

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode31
environs/sync/sync.go:31: EnvName       string
i'd like to see doc comments on these fields.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode45
environs/sync/sync.go:45: func SyncTools(ctx *SyncContext) (err error) {
s/(err error)/error/
and declare inside the body. no need to clutter the
godoc output because of the first statement.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode48
environs/sync/sync.go:48: logger.Errorf("unable to select source: %v", err)
rather than logging these errors, i'd much rather return
a better error (which will be logged to the user anyway)

e.g. return fmt.Errorf("unable to select source: %v", err)

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode57
environs/sync/sync.go:57: fmt.Fprintf(ctx.Stderr, "listing the source bucket\n")
since this is no longer in cmd/juju, i think these print statements
should be log statements. probably log.Infof.

i can't really see how they differ from log statements in any other context.

actually, i have an idea!
we could log as usual here, and in the command we could register
a log writer that prints anything logged to juju.environs.sync
at info level.

it seems to me that might get the best of both worlds,
leaving this implementation logging conventionally,
but still producing desired output to stderr.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode199
environs/sync/sync.go:199: matches, err := filepath.Glob(pattern)
i'd much prefer to see code that just reads the directory
here. that way we don't need to worry about prefix
containing wildcard characters.

i appreciate that it's just copied from cmd/juju though,
so feel free to ignore this comment.

[mue, 2013-07-24 11:17:52]

Please take a look.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go
File environs/sync/sync.go (right):

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode27
environs/sync/sync.go:27: var defaultToolsLocation string =
"https://juju-dist.s3.amazonaws.com/"
On 2013/07/23 11:53:41, rog wrote:
> s/string = //

Done.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode31
environs/sync/sync.go:31: EnvName       string
On 2013/07/23 11:53:41, rog wrote:
> i'd like to see doc comments on these fields.

Done.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode45
environs/sync/sync.go:45: func SyncTools(ctx *SyncContext) (err error) {
On 2013/07/23 11:53:41, rog wrote:
> s/(err error)/error/
> and declare inside the body. no need to clutter the
> godoc output because of the first statement.

Done.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode48
environs/sync/sync.go:48: logger.Errorf("unable to select source: %v", err)
On 2013/07/23 11:53:41, rog wrote:
> rather than logging these errors, i'd much rather return
> a better error (which will be logged to the user anyway)
> 
> e.g. return fmt.Errorf("unable to select source: %v", err)

Done.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode57
environs/sync/sync.go:57: fmt.Fprintf(ctx.Stderr, "listing the source bucket\n")
On 2013/07/23 11:53:41, rog wrote:
> since this is no longer in cmd/juju, i think these print statements
> should be log statements. probably log.Infof.
> 
> i can't really see how they differ from log statements in any other context.
> 
> actually, i have an idea!
> we could log as usual here, and in the command we could register
> a log writer that prints anything logged to juju.environs.sync
> at info level.
> 
> it seems to me that might get the best of both worlds,
> leaving this implementation logging conventionally,
> but still producing desired output to stderr.

Done.

https://codereview.appspot.com/11588043/diff/1/environs/sync/sync.go#newcode199
environs/sync/sync.go:199: matches, err := filepath.Glob(pattern)
On 2013/07/23 11:53:41, rog wrote:
> i'd much prefer to see code that just reads the directory
> here. that way we don't need to worry about prefix
> containing wildcard characters.
> 
> i appreciate that it's just copied from cmd/juju though,
> so feel free to ignore this comment.

The passed prefix typically contains a dir part (tools/juju-...). So I would
have to split before and read the directory then. Afterwards the rest of the
prefix has to be compared. So I prefer to not refactor this with this CL, but
maybe later in an extra CL.

[dimitern, 2013-07-24 14:52:06]

Looking good, but I have some suggestions below

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

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools.go#newcode8
cmd/juju/synctools.go:8: 
d

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

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:66: syncToolsCalled := false
This seems racy, can you use chan bool instead?

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:68: c.Assert(sctx.EnvName, gc.Equals, "test-target")
A lot of this is repeated several times, can you please extract the common bits
into a helper?

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:84: syncToolsCalled := false
ditto

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:102: syncToolsCalled := false
ditto

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go
File environs/sync/sync.go (right):

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:27: var DefaultToolsLocation =
"https://juju-dist.s3.amazonaws.com/"
Why not const?

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:31: // EnvName names the target environment for
synchronization.
Please put some blank lines between each pair doc comment/field below. It's very
hard to read like this.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:51: // or a specified source directory into the users
environment.
s/users/user's/

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:63: logger.Infof("listing the source bucket\n")
"listing available tools"? Also, please remove the \n in the log messages (here
and below). It the log is getting displayed without new lines this is a problem,
but remember this also potentially logs to a file, where each line has a fixed
format and blank lines should be avoided.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:143: // copyOneTool copies one tool from the source to the
target.
copyOneTool is misleading. It's "tools" because there are multiple binaries in
there. Perhaps copyOneToolsPackage?

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:168: // fileStorageReader implements StorageReader backed
by the local filesystem.
..backed by
// the local filesystem.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:173: // newFileStorageReader return a new storage reader
for
s/return/returns/

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:243: // Write implements loggos Writer interface.
s/loggos/loggo's/

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync_test.go
File environs/sync/sync_test.go (right):

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync_test.go#...
environs/sync/sync_test.go:42: // It's important that this be v1 to match the
test data.
s/that this be v1/to use v1/ ?

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync_test.go#...
environs/sync/sync_test.go:77: c.Assert(s.storage.Stop(), gc.IsNil)
if s.storage != nil {
  c.Check(s.storage.Stop(), gc.IsNil)
}

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync_test.go#...
environs/sync/sync_test.go:85: func (s *syncSuite)
TestCopyNewestFromFilesystem(c *gc.C) {
All these test cases are an excellent candidate for a table-based test, because
they differ only slightly.

[mue, 2013-07-24 18:54:45]

Please take a look.

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

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools.go#newcode8
cmd/juju/synctools.go:8: 
On 2013/07/24 14:52:06, dimitern wrote:
> d

Done.

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

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:66: syncToolsCalled := false
On 2013/07/24 14:52:06, dimitern wrote:
> This seems racy, can you use chan bool instead?

Done.

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:68: c.Assert(sctx.EnvName, gc.Equals, "test-target")
On 2013/07/24 14:52:06, dimitern wrote:
> A lot of this is repeated several times, can you please extract the common
bits
> into a helper?

Removed the default values and only focus on the tested arguments.

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:84: syncToolsCalled := false
On 2013/07/24 14:52:06, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/11588043/diff/6001/cmd/juju/synctools_test.go#...
cmd/juju/synctools_test.go:102: syncToolsCalled := false
On 2013/07/24 14:52:06, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go
File environs/sync/sync.go (right):

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:27: var DefaultToolsLocation =
"https://juju-dist.s3.amazonaws.com/"
On 2013/07/24 14:52:06, dimitern wrote:
> Why not const?

Is changed during tests, as in other packages too.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:31: // EnvName names the target environment for
synchronization.
On 2013/07/24 14:52:06, dimitern wrote:
> Please put some blank lines between each pair doc comment/field below. It's
very
> hard to read like this.

Done.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:51: // or a specified source directory into the users
environment.
On 2013/07/24 14:52:06, dimitern wrote:
> s/users/user's/

Done.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:63: logger.Infof("listing the source bucket\n")
On 2013/07/24 14:52:06, dimitern wrote:
> "listing available tools"? Also, please remove the \n in the log messages
(here
> and below). It the log is getting displayed without new lines this is a
problem,
> but remember this also potentially logs to a file, where each line has a fixed
> format and blank lines should be avoided.

Done. Thx, the \n has been left from the usage via Fprintf before.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:143: // copyOneTool copies one tool from the source to the
target.
On 2013/07/24 14:52:06, dimitern wrote:
> copyOneTool is misleading. It's "tools" because there are multiple binaries in
> there. Perhaps copyOneToolsPackage?

Done.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:168: // fileStorageReader implements StorageReader backed
by the local filesystem.
On 2013/07/24 14:52:06, dimitern wrote:
> ..backed by
> // the local filesystem.

Done.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:173: // newFileStorageReader return a new storage reader
for
On 2013/07/24 14:52:06, dimitern wrote:
> s/return/returns/

Done.

https://codereview.appspot.com/11588043/diff/6001/environs/sync/sync.go#newco...
environs/sync/sync.go:243: // Write implements loggos Writer interface.
On 2013/07/24 14:52:06, dimitern wrote:
> s/loggos/loggo's/

Done.

[mue, 2013-07-25 10:18:00]

Please take a look.

[mue, 2013-07-25 13:07:28]

Please take a look.

[dimitern, 2013-07-25 13:33:02]

Thanks! LGTM.

[dimitern, 2013-07-25 13:33:03]

Thanks! LGTM.