Merge lp://staging/~themue/juju-core/035-bootstrap-autosync into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+177119_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/bootstrap: integrate syncing

After the CL moving the synchronization logic into an
own package this CL now checks if tools can be found
during bootstrap. If this is not possible it will use
the synchronization logic and then retries the
bootstrapping.

https://code.launchpad.net/~themue/juju-core/035-bootstrap-autosync/+merge/177119

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/juju/bootstrap.go
   M cmd/juju/bootstrap_test.go

LGTM, apart from some notes.

I would like to suggest that you change your commenting habits in the
tests a little bit. Each of your comments there is just a summary of
the code that follows — a "code smell" that usually indicates a need for
function extraction. I don't mind this in itself: Go makes function
extraction less effective and so the unfortunate pattern still has a
place. But the comments don't say _why_ specific things need to be
done. This is vitally important to anyone maintaining or debugging the
code after you.

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

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap.go#newcode47
cmd/juju/bootstrap.go:47: f.StringVar(&c.Source, "source", "", "chose a
location on the file system as source of the tools")

Typo: the "chose" should be "choose" — but come to think of it, is it
worth having a verb there in the first place?

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap.go#newcode104
cmd/juju/bootstrap.go:104: // No tools available, so sync and retry.

You interpret "not-found" errors coming out of Bootstrap. For the sake
of thoroughness, do we actually know that a not-found error coming out
of Bootstrap *has* to mean that no tools are available? Or is that just
the most likely cause of a not-found error?

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

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap_test.go#newcode226
cmd/juju/bootstrap_test.go:226: func (s *BootstrapSuite) TestAutoSync(c
*gc.C) {

I'm not familiar with the autosync thing that this is about. And so I
have no idea how this test relates to it: where do you exercise
autosync? What aspect of it are you proving in this test? What exact
statement would be disproved by this test failing?

Explicit is better than implicit, and I find we often get better code
and better designs when we train ourselves to address a slightly less
specialized audience. Such as myself, in this case.

If what you're verifying is just that the command at the end returns
success, then consider testing something that the command actually does,
as well. Otherwise the command could fool you by doing nothing at all,
or perhaps by failing for some unrelated reason.

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap_test.go#newcode227
cmd/juju/bootstrap_test.go:227: // Prepare a the storage for testing.

Pick one: "a" storage or "the" storage!

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap_test.go#newcode251
cmd/juju/bootstrap_test.go:251: // Prepare a the storage for testing.

How embarrassing. You copied and pasted a typo. :)

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap_test.go#newcode268
cmd/juju/bootstrap_test.go:268: // Create and pupulate a local tools
directory.

Typo: "pupulate" should be "populate."

Also, this loop could be a good one to extract into a test helper, so
that the test itself is easier to grasp. The parameters and the return...

Please take a look.

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

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap.go#newcode47
cmd/juju/bootstrap.go:47: f.StringVar(&c.Source, "source", "", "chose a
location on the file system as source of the tools")
On 2013/07/26 10:29:48, jtv.canonical wrote:

> Typo: the "chose" should be "choose" — but come to think of it, is it
worth
> having a verb there in the first place?

Done. The verb is like "set" or "upload". But I'll change it to set here
too.

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap.go#newcode104
cmd/juju/bootstrap.go:104: // No tools available, so sync and retry.
On 2013/07/26 10:29:48, jtv.canonical wrote:

> You interpret "not-found" errors coming out of Bootstrap. For the
sake of
> thoroughness, do we actually know that a not-found error coming out of
Bootstrap
> *has* to mean that no tools are available? Or is that just the most
likely
> cause of a not-found error?

Moved it into an earlier called ensure method. Here
tools.FindBootstrapTools() is used, which returns this error.

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

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap_test.go#newcode226
cmd/juju/bootstrap_test.go:226: func (s *BootstrapSuite) TestAutoSync(c
*gc.C) {
On 2013/07/26 10:29:48, jtv.canonical wrote:

> I'm not familiar with the autosync thing that this is about. And so I
have no
> idea how this test relates to it: where do you exercise autosync?
What aspect
> of it are you proving in this test? What exact statement would be
disproved by
> this test failing?

> Explicit is better than implicit, and I find we often get better code
and better
> designs when we train ourselves to address a slightly less specialized
audience.
> Such as myself, in this case.

> If what you're verifying is just that the command at the end returns
success,
> then consider testing something that the command actually does, as
well.
> Otherwise the command could fool you by doing nothing at all, or
perhaps by
> failing for some unrelated reason.

Done. Made it more explicit by optical blocks and helpful comments.

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap_test.go#newcode227
cmd/juju/bootstrap_test.go:227: // Prepare a the storage for testing.
On 2013/07/26 10:29:48, jtv.canonical wrote:

> Pick one: "a" storage or "the" storage!

Done.

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap_test.go#newcode251
cmd/juju/bootstrap_test.go:251: // Prepare a the storage for testing.
On 2013/07/26 10:29:48, jtv.canonical wrote:

> How embarrassing. You copied and pasted a typo. :)

Done.

https://codereview.appspot.com/11910043/diff/1/cmd/juju/bootstrap_test.go#newcode268
cmd/juju/bootstrap_test.go:268: // Create and pupulate a local tools
directory.
On 2013/07/26 10:29:48, jtv.canonical wrote:

> Typo: "pupulate" should be "populate."

> Also, this loop could be a good one to extract int...

LGTM with a few suggestions below

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode48
cmd/juju/bootstrap.go:48: f.StringVar(&c.Source, "source", "", "set a
location on the file system as source of the tools")

How about:
f.StringVar(&c.Source, "source", "", "local path to use as tools
source")
?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode74
cmd/juju/bootstrap.go:74: // Ensure the availability of tools.

This comment is redundant, I think.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode111
cmd/juju/bootstrap.go:111: // ensureToolsAvailability looks for the
availibilty of tools. If it doesn't find

// ensureToolsAvailability verifies the tools are available. If no tools
are found, it will automatically synchronize them.
?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode114
cmd/juju/bootstrap.go:114: // Register writer for output of possible
syncing on screen.

// Capture possible output while syncing and log it.
?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode117
cmd/juju/bootstrap.go:117: // Try to find bootstrap tools.

Blank line before this one please.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode131
cmd/juju/bootstrap.go:131: // Try again.

// Synchronization done, retry.
?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap_test.go#newcode249
cmd/juju/bootstrap_test.go:249: // Create home with dummy provider and
remove all

Can you please extract this block into a suite method, called e.g.
purgeTools and call it here and in the test below?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap_test.go#newcode264
cmd/juju/bootstrap_test.go:264: // Now check the available tools which
are the 1.0.0 tools.

This block as well? s.ensureTools(c, v100all) here and below?

https://codereview.appspot.com/11910043/

LGTM with a few suggestions below

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode48
cmd/juju/bootstrap.go:48: f.StringVar(&c.Source, "source", "", "set a
location on the file system as source of the tools")

How about:
f.StringVar(&c.Source, "source", "", "local path to use as tools
source")
?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode74
cmd/juju/bootstrap.go:74: // Ensure the availability of tools.

This comment is redundant, I think.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode111
cmd/juju/bootstrap.go:111: // ensureToolsAvailability looks for the
availibilty of tools. If it doesn't find

// ensureToolsAvailability verifies the tools are available. If no tools
are found, it will automatically synchronize them.
?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode114
cmd/juju/bootstrap.go:114: // Register writer for output of possible
syncing on screen.

// Capture possible output while syncing and log it.
?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode117
cmd/juju/bootstrap.go:117: // Try to find bootstrap tools.

Blank line before this one please.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode131
cmd/juju/bootstrap.go:131: // Try again.

// Synchronization done, retry.
?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap_test.go#newcode249
cmd/juju/bootstrap_test.go:249: // Create home with dummy provider and
remove all

Can you please extract this block into a suite method, called e.g.
purgeTools and call it here and in the test below?

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap_test.go#newcode264
cmd/juju/bootstrap_test.go:264: // Now check the available tools which
are the 1.0.0 tools.

This block as well? s.ensureTools(c, v100all) here and below?

https://codereview.appspot.com/11910043/

The attempt to merge lp:~themue/juju-core/035-bootstrap-autosync into lp:juju-core failed. Below is the output from the failed tests.

/bin/sh: 1: go: not found

Using this as a bot test, please don't mind the spam.

The attempt to merge lp:~themue/juju-core/035-bootstrap-autosync into lp:juju-core failed. Below is the output from the failed tests.

worker/upgrader/upgrader.go

state/api/apiclient.go:7:2: cannot find package "code.google.com/p/go.net/websocket" in any of:
 /usr/lib/go/src/pkg/code.google.com/p/go.net/websocket (from $GOROOT)
 /home/tarmac/trees/src/code.google.com/p/go.net/websocket (from $GOPATH)
state/annotator.go:10:2: cannot find package "labix.org/v2/mgo" in any of:
 /usr/lib/go/src/pkg/labix.org/v2/mgo (from $GOROOT)
 /home/tarmac/trees/src/labix.org/v2/mgo (from $GOPATH)
agent/tools/marshal.go:7:2: cannot find package "labix.org/v2/mgo/bson" in any of:
 /usr/lib/go/src/pkg/labix.org/v2/mgo/bson (from $GOROOT)
 /home/tarmac/trees/src/labix.org/v2/mgo/bson (from $GOPATH)
state/annotator.go:11:2: cannot find package "labix.org/v2/mgo/txn" in any of:
 /usr/lib/go/src/pkg/labix.org/v2/mgo/txn (from $GOROOT)
 /home/tarmac/trees/src/labix.org/v2/mgo/txn (from $GOPATH)
agent/agent.go:13:2: cannot find package "launchpad.net/goyaml" in any of:
 /usr/lib/go/src/pkg/launchpad.net/goyaml (from $GOROOT)
 /home/tarmac/trees/src/launchpad.net/goyaml (from $GOPATH)
agent/tools/tools.go:9:2: cannot find package "launchpad.net/loggo" in any of:
 /usr/lib/go/src/pkg/launchpad.net/loggo (from $GOROOT)
 /home/tarmac/trees/src/launchpad.net/loggo (from $GOPATH)
state/watcher/helpers.go:7:2: cannot find package "launchpad.net/tomb" in any of:
 /usr/lib/go/src/pkg/launchpad.net/tomb (from $GOROOT)
 /home/tarmac/trees/src/launchpad.net/tomb (from $GOPATH)
cmd/cmd.go:16:2: cannot find package "launchpad.net/gnuflag" in any of:
 /usr/lib/go/src/pkg/launchpad.net/gnuflag (from $GOROOT)
 /home/tarmac/trees/src/launchpad.net/gnuflag (from $GOPATH)
environs/imagemetadata/decode.go:8:2: cannot find package "code.google.com/p/go.crypto/openpgp" in any of:
 /usr/lib/go/src/pkg/code.google.com/p/go.crypto/openpgp (from $GOROOT)
 /home/tarmac/trees/src/code.google.com/p/go.crypto/openpgp (from $GOPATH)
environs/imagemetadata/decode.go:9:2: cannot find package "code.google.com/p/go.crypto/openpgp/clearsign" in any of:
 /usr/lib/go/src/pkg/code.google.com/p/go.crypto/openpgp/clearsign (from $GOROOT)
 /home/tarmac/trees/src/code.google.com/p/go.crypto/openpgp/clearsign (from $GOPATH)
environs/ec2/config.go:8:2: cannot find package "launchpad.net/goamz/aws" in any of:
 /usr/lib/go/src/pkg/launchpad.net/goamz/aws (from $GOROOT)
 /home/tarmac/trees/src/launchpad.net/goamz/aws (from $GOPATH)
environs/ec2/ec2.go:10:2: cannot find package "launchpad.net/goamz/ec2" in any of:
 /usr/lib/go/src/pkg/launchpad.net/goamz/ec2 (from $GOROOT)
 /home/tarmac/trees/src/launchpad.net/goamz/ec2 (from $GOPATH)
environs/ec2/ec2.go:11:2: cannot find package "launchpad.net/goamz/s3" in any of:
 /usr/lib/go/src/pkg/launchpad.net/goamz/s3 (from $GOROOT)
 /home/tarmac/trees/src/launchpad.net/goamz/s3 (from $GOPATH)
container/lxc/test.go:11:2: cannot find package "launchpad.net/gocheck" in any of:
 /usr/lib/go/src/pkg/launchpad.net/gocheck (from $GOROOT)
 /home/tarmac/trees/src/launchpad.net/gocheck (from $GOPATH)
container/lxc/lxc.go:13:2: cannot find package "launchpad.net/golxc" in any of:
 /us...

The attempt to merge lp:~themue/juju-core/035-bootstrap-autosync into lp:juju-core failed. Below is the output from the failed tests.

worker/upgrader/upgrader.go
ok launchpad.net/juju-core/agent 1.825s
ok launchpad.net/juju-core/agent/tools 21.096s

----------------------------------------------------------------------
FAIL: bzr_test.go:59: BzrSuite.TestCommit

bzr_test.go:66:
    c.Assert(err, IsNil)
... value *errors.errorString = &errors.errorString{s:"error running \"bzr commit\": bzr: ERROR: Unable to determine your name.\nPlease, set your name with the 'whoami' command.\nE.g. bzr whoami \"Your Name <email address hidden>\"\nexit status 3"} ("error running \"bzr commit\": bzr: ERROR: Unable to determine your name.\nPlease, set your name with the 'whoami' command.\nE.g. bzr whoami \"Your Name <email address hidden>\"\nexit status 3")

----------------------------------------------------------------------
FAIL: bzr_test.go:77: BzrSuite.TestPush

bzr_test.go:92:
    c.Assert(err, IsNil)
... value *errors.errorString = &errors.errorString{s:"error running \"bzr commit\": bzr: ERROR: Unable to determine your name.\nPlease, set your name with the 'whoami' command.\nE.g. bzr whoami \"Your Name <email address hidden>\"\nexit status 3"} ("error running \"bzr commit\": bzr: ERROR: Unable to determine your name.\nPlease, set your name with the 'whoami' command.\nE.g. bzr whoami \"Your Name <email address hidden>\"\nexit status 3")

OOPS: 6 passed, 2 FAILED
--- FAIL: Test (5.11 seconds)
FAIL
FAIL launchpad.net/juju-core/bzr 5.120s
ok launchpad.net/juju-core/cert 2.135s
ok launchpad.net/juju-core/charm 0.555s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.015s
ok launchpad.net/juju-core/cmd 0.223s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
listing available toolsfound 6 toolsfound 6 recent tools (version 1.10.0)listing target bucketfound 0 tools in target; 6 tools to be copiedcopying 1.10.0-precise-amd64 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-precise-amd64.tgzcopying tools/juju-1.10.0-precise-amd64.tgzdownloaded tools/juju-1.10.0-precise-amd64.tgz (2205kB), uploadingdownload 2205kB, uploadingcopying 1.10.0-precise-i386 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-precise-i386.tgzcopying tools/juju-1.10.0-precise-i386.tgzdownloaded tools/juju-1.10.0-precise-i386.tgz (2306kB), uploadingdownload 2306kB, uploadingcopying 1.10.0-quantal-amd64 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-quantal-amd64.tgzcopying tools/juju-1.10.0-quantal-amd64.tgzdownloaded tools/juju-1.10.0-quantal-amd64.tgz (2209kB), uploadingdownload 2209kB, uploadingcopying 1.10.0-quantal-i386 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-quantal-i386.tgzcopying tools/juju-1.10.0-quantal-i386.tgzdownloaded tools/juju-1.10.0-quantal-i386.tgz (2311kB), uploadingdownload 2311kB, uploadingcopying 1.10.0-raring-amd64 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-raring-amd64.tgzcopying tools/juju-1.10.0-raring-amd64.tgzdownloaded tools/juju-1.10.0-ra...

The attempt to merge lp:~themue/juju-core/035-bootstrap-autosync into lp:juju-core failed. Below is the output from the failed tests.

worker/upgrader/upgrader.go
ok launchpad.net/juju-core/agent 1.654s
ok launchpad.net/juju-core/agent/tools 20.845s
ok launchpad.net/juju-core/bzr 6.355s
ok launchpad.net/juju-core/cert 2.423s
ok launchpad.net/juju-core/charm 0.516s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.015s
ok launchpad.net/juju-core/cmd 0.223s
? launchpad.net/juju-core/cmd/builddb [no test files]
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
listing available toolsfound 6 toolsfound 6 recent tools (version 1.10.0)listing target bucketfound 0 tools in target; 6 tools to be copiedcopying 1.10.0-precise-amd64 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-precise-amd64.tgzcopying tools/juju-1.10.0-precise-amd64.tgzdownloaded tools/juju-1.10.0-precise-amd64.tgz (2205kB), uploadingdownload 2205kB, uploadingcopying 1.10.0-precise-i386 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-precise-i386.tgzcopying tools/juju-1.10.0-precise-i386.tgzdownloaded tools/juju-1.10.0-precise-i386.tgz (2306kB), uploadingdownload 2306kB, uploadingcopying 1.10.0-quantal-amd64 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-quantal-amd64.tgzcopying tools/juju-1.10.0-quantal-amd64.tgzdownloaded tools/juju-1.10.0-quantal-amd64.tgz (2209kB), uploadingdownload 2209kB, uploadingcopying 1.10.0-quantal-i386 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-quantal-i386.tgzcopying tools/juju-1.10.0-quantal-i386.tgzdownloaded tools/juju-1.10.0-quantal-i386.tgz (2311kB), uploadingdownload 2311kB, uploadingcopying 1.10.0-raring-amd64 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-raring-amd64.tgzcopying tools/juju-1.10.0-raring-amd64.tgzdownloaded tools/juju-1.10.0-raring-amd64.tgz (2208kB), uploadingdownload 2208kB, uploadingcopying 1.10.0-raring-i386 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-raring-i386.tgzcopying tools/juju-1.10.0-raring-i386.tgzdownloaded tools/juju-1.10.0-raring-i386.tgz (2312kB), uploadingdownload 2312kB, uploadingcopied 6 toolslisting available toolsfound 6 toolsfound 6 recent tools (version 1.10.0)listing target bucketfound 0 tools in target; 6 tools to be copiedcopying 1.10.0-precise-amd64 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-precise-amd64.tgzcopying tools/juju-1.10.0-precise-amd64.tgzdownloaded tools/juju-1.10.0-precise-amd64.tgz (2205kB), uploadingdownload 2205kB, uploadingcopying 1.10.0-precise-i386 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-precise-i386.tgzcopying tools/juju-1.10.0-precise-i386.tgzdownloaded tools/juju-1.10.0-precise-i386.tgz (2306kB), uploadingdownload 2306kB, uploadingcopying 1.10.0-quantal-amd64 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-quantal-amd64.tgzcopying tools/juju-1.10.0-quantal-amd64.tgzdownloaded tools/juju-1.10.0-quantal-amd64.tgz (2209kB), uploadingdownload 2209kB, uploadingcopying 1.10.0-quantal-i386 from https://juju-dist.s3.amazonaws.com/tools/juju-1.10.0-qua...

Review handled, but new problems with old tests.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode48
cmd/juju/bootstrap.go:48: f.StringVar(&c.Source, "source", "", "set a
location on the file system as source of the tools")
On 2013/07/29 10:35:32, dimitern wrote:

> How about:
> f.StringVar(&c.Source, "source", "", "local path to use as tools
source")
> ?

Done.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode74
cmd/juju/bootstrap.go:74: // Ensure the availability of tools.
On 2013/07/29 10:35:32, dimitern wrote:

> This comment is redundant, I think.

Done.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode111
cmd/juju/bootstrap.go:111: // ensureToolsAvailability looks for the
availibilty of tools. If it doesn't find
On 2013/07/29 10:35:32, dimitern wrote:

> // ensureToolsAvailability verifies the tools are available. If no
tools are
> found, it will automatically synchronize them.
> ?

Done.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode114
cmd/juju/bootstrap.go:114: // Register writer for output of possible
syncing on screen.
On 2013/07/29 10:35:32, dimitern wrote:

> // Capture possible output while syncing and log it.
> ?

Done, but a bit changed. The log is captured and printed on the screen.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode117
cmd/juju/bootstrap.go:117: // Try to find bootstrap tools.
On 2013/07/29 10:35:32, dimitern wrote:

> Blank line before this one please.

Done.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap.go#newcode131
cmd/juju/bootstrap.go:131: // Try again.
On 2013/07/29 10:35:32, dimitern wrote:

> // Synchronization done, retry.
> ?

Done.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap_test.go#newcode249
cmd/juju/bootstrap_test.go:249: // Create home with dummy provider and
remove all
On 2013/07/29 10:35:32, dimitern wrote:

> Can you please extract this block into a suite method, called e.g.
purgeTools
> and call it here and in the test below?

Done.

https://codereview.appspot.com/11910043/diff/7001/cmd/juju/bootstrap_test.go#newcode264
cmd/juju/bootstrap_test.go:264: // Now check the available tools which
are the 1.0.0 tools.
On 2013/07/29 10:35:32, dimitern wrote:

> This block as well? s.ensureTools(c, v100all) here and below?

Done.

https://codereview.appspot.com/11910043/

Please take a look.

https://codereview.appspot.com/11910043/

Please take a look.

https://codereview.appspot.com/11910043/