Issue 11910043: cmd/bootstrap: integrate syncing

Issue 11910043: cmd/bootstrap: integrate syncing (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years, 9 months ago by mue

Modified:
10 years, 8 months ago

Reviewers:
dimitern, mp+177119, jtv.canonical

Visibility:
Public.

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)

Patch Set 1 #

Total comments: 12

Patch Set 2 : cmd/bootstrap: integrate syncing #

Total comments: 16

Patch Set 3 : cmd/bootstrap: integrate syncing #

Patch Set 4 : cmd/bootstrap: integrate syncing #

Created: 10 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+227 lines, -41 lines)			Patch
A	[revision details]	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	cmd/juju/bootstrap.go	View	1 2	4 chunks	+39 lines, -2 lines	0 comments	Download
M	cmd/juju/bootstrap_test.go	View	1 2 3	5 chunks	+183 lines, -36 lines	0 comments	Download
M	environs/ec2/storage.go	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	environs/sync/sync.go	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 8

Expand All Messages | Collapse All Messages

jtv.canonical

LGTM, apart from some notes. I would like to suggest that you change your commenting ...

10 years, 9 months ago (2013-07-26 10:29:48 UTC) #2

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#new...
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#new...
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#new...
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#new...
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 value should be
all that matters to the test; the rest is implementation detail.

mue

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

10 years, 9 months ago (2013-07-26 13:13:39 UTC) #3

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

dimitern

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", "", ...

10 years, 9 months ago (2013-07-29 10:35:31 UTC) #4

dimitern

10 years, 9 months ago (2013-07-29 10:35:31 UTC) #5

mue

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, ...

10 years, 9 months ago (2013-07-29 16:48:54 UTC) #6

Please take a look.

Expand All Messages | Collapse All Messages