Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(602)

Issue 8051043: cmd/juju: add juju sync-tools

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years ago by jameinel
Modified:
10 years, 12 months ago
Reviewers:
mp+155705, rog, wallyworld, fwereade, dimitern
Visibility:
Public.

Description

cmd/juju: add juju sync-tools This is a command to list the us-east-1 bucket, and automate copying all the new(--all) tools to your private bucket. It takes care that all the bits and pieces are put into the right paths. By default it finds just the latest release (as determined by Version.Less()). You can, however, supply --all to create a full copy of the public bucket. At present, there is a known issue that it needs ec2 credentials, when it should only really need your target credentials. (This is because FindTools always looks in the Private bucket, and doesn't expose a way to look in just the Public one.) The actual command isn't very well tested end-to-end, because of the difficulty in setting up environments, putting tools in them, etc. However, all the helper functions should be well tested, and I've manually tested copying to Canonistack. I imagine this will need a bit more polishing to land, but it's definitely at the point where I want to solicit feedback. https://code.launchpad.net/~jameinel/juju-core/sync-tools/+merge/155705 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 50

Patch Set 2 : cmd/juju: add juju sync-tools #

Total comments: 3

Patch Set 3 : cmd/juju: add juju sync-tools #

Patch Set 4 : cmd/juju: add juju sync-tools #

Total comments: 6

Patch Set 5 : cmd/juju: add juju sync-tools #

Patch Set 6 : cmd/juju: add juju sync-tools #

Total comments: 29

Patch Set 7 : cmd/juju: add juju sync-tools #

Total comments: 6

Patch Set 8 : cmd/juju: add juju sync-tools #

Total comments: 8

Patch Set 9 : cmd/juju: add juju sync-tools #

Patch Set 10 : cmd/juju: add juju sync-tools #

Unified diffs Side-by-side diffs Delta from patch set Stats (+103 lines, -125 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/main_test.go View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M cmd/juju/sync_tools.go View 1 2 3 4 5 6 7 8 9 8 chunks +41 lines, -43 lines 0 comments Download
M cmd/juju/sync_tools_test.go View 1 2 3 4 5 6 7 8 9 2 chunks +59 lines, -82 lines 0 comments Download

Messages

Total messages: 22
jameinel
Please take a look.
11 years ago (2013-03-27 11:16:07 UTC) #1
dimitern
Looks good so far, some suggestions below. https://codereview.appspot.com/8051043/diff/1/cmd/juju/main_test.go File cmd/juju/main_test.go (right): https://codereview.appspot.com/8051043/diff/1/cmd/juju/main_test.go#newcode49 cmd/juju/main_test.go:49: func helpText(command ...
11 years ago (2013-03-27 15:06:38 UTC) #2
jameinel
Please take a look. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode16 cmd/juju/sync_tools.go:16: // SyncToolsCommand copies all the ...
11 years ago (2013-03-27 16:20:40 UTC) #3
dimitern
Thanks, with the suggestions about log messages format, LGTM. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode16 cmd/juju/sync_tools.go:16: ...
11 years ago (2013-03-27 18:25:44 UTC) #4
wallyworld
LGTM, but I'd like a Go expert to comment on the explicit initialisation of "" ...
11 years ago (2013-03-28 04:50:06 UTC) #5
jameinel
Please take a look. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode136 cmd/juju/sync_tools.go:136: log.Infof("Copying %s from %s\n", tool.Binary, ...
11 years ago (2013-03-28 08:12:32 UTC) #6
dimitern
Explanation on log prefixes. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode136 cmd/juju/sync_tools.go:136: log.Infof("Copying %s from %s\n", tool.Binary, ...
11 years ago (2013-03-28 08:26:42 UTC) #7
jameinel
Please take a look.
11 years ago (2013-03-28 09:54:31 UTC) #8
fwereade
This is great, but NOT LGTM without a test. Ping me if it seems harder ...
11 years ago (2013-03-30 05:35:28 UTC) #9
jameinel
Please take a look. https://codereview.appspot.com/8051043/diff/15001/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/15001/cmd/juju/sync_tools.go#newcode124 cmd/juju/sync_tools.go:124: target environs.Storage, dryRun bool, ctx ...
11 years ago (2013-03-31 09:19:46 UTC) #10
jameinel
Please take a look.
11 years ago (2013-03-31 10:14:05 UTC) #11
jameinel
On 2013/03/30 05:35:28, fwereade wrote: > This is great, but NOT LGTM without a test. ...
11 years ago (2013-03-31 10:19:35 UTC) #12
fwereade
Essentially LGTM, but quite a lot of random notes for your consideration. None of them's ...
10 years, 12 months ago (2013-04-01 22:30:24 UTC) #13
jameinel
Please take a look. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newcode20 cmd/juju/sync_tools.go:20: targetToolsList *environs.ToolsList On 2013/04/01 22:30:24, ...
10 years, 12 months ago (2013-04-02 08:48:22 UTC) #14
fwereade
LGTM https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newcode110 cmd/juju/sync_tools.go:110: buf := bytes.NewBuffer(nil) On 2013/04/02 08:48:22, jameinel wrote: ...
10 years, 12 months ago (2013-04-02 10:22:22 UTC) #15
thumper
https://codereview.appspot.com/8051043/diff/33001/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/33001/cmd/juju/sync_tools.go#newcode62 cmd/juju/sync_tools.go:62: var curBest *state.Tools = nil Since you only care ...
10 years, 12 months ago (2013-04-02 21:48:27 UTC) #16
jameinel
Please take a look. https://codereview.appspot.com/8051043/diff/33001/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/33001/cmd/juju/sync_tools.go#newcode62 cmd/juju/sync_tools.go:62: var curBest *state.Tools = nil ...
10 years, 12 months ago (2013-04-03 07:52:15 UTC) #17
rog
LGTM with a few trivials below. i have intentially not commented on any of the ...
10 years, 12 months ago (2013-04-03 08:32:54 UTC) #18
fwereade
Please land this with stderr feedback; we will probably end up doing something that is ...
10 years, 12 months ago (2013-04-03 09:11:05 UTC) #19
jameinel
*** Submitted: cmd/juju: add juju sync-tools This is a command to list the us-east-1 bucket, ...
10 years, 12 months ago (2013-04-03 12:04:48 UTC) #20
jameinel
*** Submitted: cmd/juju: add juju sync-tools This is a command to list the us-east-1 bucket, ...
10 years, 12 months ago (2013-04-03 12:15:04 UTC) #21
rog
10 years, 12 months ago (2013-04-03 13:16:59 UTC) #22
thanks!

https://codereview.appspot.com/8051043/diff/40001/cmd/juju/sync_tools.go
File cmd/juju/sync_tools.go (right):

https://codereview.appspot.com/8051043/diff/40001/cmd/juju/sync_tools.go#newc...
cmd/juju/sync_tools.go:107: buf := bytes.NewBuffer(nil)
On 2013/04/03 12:04:48, jameinel wrote:
> On 2013/04/03 08:32:55, rog wrote:
> > var buf bytes.Buffer
> > 
> > would be more usual (we usually use NewBuffer only if there's existing data)
> 
> That doesn't work because type "bytes.Buffer" isn't an io.Reader but
> "(*bytes.Buffer)" is. So I could pass &buf everywhere, but it seems better to
> just start with a pointer.

Yeah, I tend to use &buf.

> I went with
>  buf := &bytes.Buffer{}
> 
> Since you don't seem to like make/NewStuff sort of functions. Though from what
I
> can tell buf := bytes.NewBuffer(nil) is identical to buf := &bytes.Buffer{}.
> 
> Does it actually matter?

just consistency, really.
grep for NewBuffer and 'bytes.Buffer' in the juju source.

(the version i suggested is used 26 out of 41 times; the version you used is
used the rest of the time)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b