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 into a test helper, so
that the
> test itself is easier to grasp.  The parameters and the return value
should be
> all that matters to the test; the rest is implementation detail.

Done.

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