|
|
Descriptioncmd/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 #
MessagesTotal messages: 22
Please take a look.
Sign in to reply to this message.
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 cmd.Command, name string) string { nice! 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 tools from the us-east-1 bucket to the local why us-east-1 precisely? https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode31 cmd/juju/sync_tools.go:31: Purpose: "copy tools from another public bucket", there should be more elaborate explanation of what it's doing, and a help text as well (to clarify what is copied where and why). https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode54 cmd/juju/sync_tools.go:54: // Find the set of tools at the 'newest' version maybe s/newest/latest/ throughout? https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode83 cmd/juju/sync_tools.go:83: res := make([]*state.Tools, 0) maybe res := []*state.Tools{} then? https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode95 cmd/juju/sync_tools.go:95: log.Warningf("tool was nil?") please use the usual format for the log messages: <package>: descriptive text. Also, why the question marks? https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode113 cmd/juju/sync_tools.go:113: tempFile, err := ioutil.TempFile("", "juju-tgz") how about using a bytes.Buffer to read into instead of a temp file? https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode125 cmd/juju/sync_tools.go:125: log.Infof("environs: downloaded %v (%dkB), uploading", toolsPath, (nBytes+512)/1024) environs? shouldn't this be "cmd/juju" or something? 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, tool.URL) s/C/c/ + package prefix https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode137 cmd/juju/sync_tools.go:137: if !dryRun { maybe it'll be better to: if dryRun -> return nil then copy (collapsing a level of indentation). https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode149 cmd/juju/sync_tools.go:149: log.Infof("Failed to create officialEnviron") prefix, lowercase, also a bit of rewording I think: "cmd/juju: cannot create (source? origin?) environment" ? you're masking the original error here. in fact, why bother and not return err instead? https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode165 cmd/juju/sync_tools.go:165: fmt.Printf("Found: %s\n", t) log.Debugf? https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode172 cmd/juju/sync_tools.go:172: fmt.Printf("Found Target: %s\n", t) ditto https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go File cmd/juju/sync_tools_test.go (right): https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:28: opc, errc := runCommand(new(SyncToolsCommand), "-h") i think it's better to encapsulate the run call into its own runSyncTools, like other commands do; this way it could be reused in other cmd tests if need be. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:39: Number: version.Number{Major: 1, Minor: 0, Patch: 0, Build: 0}, remove keys perhaps, to shorten line length, provided you're initializing all of them: version.Number{1,0,0,0} (in all cases below). https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:42: URL: "", remove this - URL will be "" when not initialized anyway. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:70: var onlyOneTests = []struct { move this outside the test func maybe? (all cases below) https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:87: var oneBestTests = []struct { ditto https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:91: {all: []*state.Tools{t1000precise, t1900quantal}, this reads harder than: { all: ..., best: ..., }, https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:142: res := findMissing([]*state.Tools{t1000precise, t1000quantal}, \n after ( for better readability?
Sign in to reply to this message.
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 tools from the us-east-1 bucket to the local On 2013/03/27 15:06:38, dimitern wrote: > why us-east-1 precisely? That is the official bucket that Dave Cheney uploads to. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode31 cmd/juju/sync_tools.go:31: Purpose: "copy tools from another public bucket", On 2013/03/27 15:06:38, dimitern wrote: > there should be more elaborate explanation of what it's doing, and a help text > as well (to clarify what is copied where and why). Thanks. I hadn't seen the "Doc" parameter. The one I copied only had Name and Purpose, and it is nice to have a longer form explanation. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode83 cmd/juju/sync_tools.go:83: res := make([]*state.Tools, 0) On 2013/03/27 15:06:38, dimitern wrote: > maybe res := []*state.Tools{} then? I'm pretty sure they are identical. As such it is unclear when constructor syntax is preferred to make(). (Certainly you have to use make if you want to set the capacity/length of the underlying array.) I think I've pretty much stuck with using make() to create new slices. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode95 cmd/juju/sync_tools.go:95: log.Warningf("tool was nil?") On 2013/03/27 15:06:38, dimitern wrote: > please use the usual format for the log messages: > <package>: descriptive text. > > Also, why the question marks? This was debug code that wasn't even triggering. The bug was that I wasn't properly handling an 'err' later on. I'll just remove them. I was getting a panic() and these were my debug prints to try and track it down. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode113 cmd/juju/sync_tools.go:113: tempFile, err := ioutil.TempFile("", "juju-tgz") On 2013/03/27 15:06:38, dimitern wrote: > how about using a bytes.Buffer to read into instead of a temp file? We build the tools into a TempFile, so it seemed we wanted to avoid doing it all in memory. Its a few MB (2.2MB ATM). I would be ok buffering 2MB, but I know the Mongo .tgz is more like 40MB. I think it is reasonable to try and avoid buffering whole files in memory as much as we can. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode125 cmd/juju/sync_tools.go:125: log.Infof("environs: downloaded %v (%dkB), uploading", toolsPath, (nBytes+512)/1024) On 2013/03/27 15:06:38, dimitern wrote: > environs? shouldn't this be "cmd/juju" or something? Yeah, I was just copying that code. I think I'll leave out any prefix, because the logging system already puts what command is running. Thanks for the catch. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode137 cmd/juju/sync_tools.go:137: if !dryRun { On 2013/03/27 15:06:38, dimitern wrote: > maybe it'll be better to: > > if dryRun -> return nil > then copy (collapsing a level of indentation). Actually if dryRun => continue since you want to print all the files you would copy. I had that originally (only I had the logic inverted with !dryRun) so I ended up nesting it. I'll invert the logic and pull things up a level. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode149 cmd/juju/sync_tools.go:149: log.Infof("Failed to create officialEnviron") On 2013/03/27 15:06:38, dimitern wrote: > prefix, lowercase, also a bit of rewording I think: > "cmd/juju: cannot create (source? origin?) environment" ? > > you're masking the original error here. > in fact, why bother and not return err instead? I'm returning err immediately after this, so I don't quite understand your point. The main reason I added this log message, is because you can get a failure to do with credentials, etc. But you don't know whether it failed to create the "officialEnviron" or to create the target environ. I did change the wording a bit. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode165 cmd/juju/sync_tools.go:165: fmt.Printf("Found: %s\n", t) On 2013/03/27 15:06:38, dimitern wrote: > log.Debugf? So the issue is trying to figure out, when you just run the command, what should it display if you *don't* supply --debug. Without --debug, if you don't Printf, you get 0 messages. I'm actually thinking this could probably be turned into debug messages, but some of the actual progress messages should be printed. Something like: listing source bucket found %d tools listing target bucket found %d tools, %d tools missing copying tools/foo-1.12.10.tgz (1242kB) copying tools/foo-1.13.10.tgz (2242kB) ... Mostly because those are the bits that sit waiting on RPC for a while. Does that seem good to you? https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go File cmd/juju/sync_tools_test.go (right): https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:28: opc, errc := runCommand(new(SyncToolsCommand), "-h") On 2013/03/27 15:06:38, dimitern wrote: > i think it's better to encapsulate the run call into its own runSyncTools, like > other commands do; this way it could be reused in other cmd tests if need be. There are quite a few commands tested in this way. I'd be happy to refactor once we have more than one test that actually needs the helper. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:39: Number: version.Number{Major: 1, Minor: 0, Patch: 0, Build: 0}, On 2013/03/27 15:06:38, dimitern wrote: > remove keys perhaps, to shorten line length, provided you're initializing all of > them: version.Number{1,0,0,0} (in all cases below). I was having some trouble with mixed named and unnamed fields, but it looks like it works as long as you are consistent within one set of {}. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:42: URL: "", On 2013/03/27 15:06:38, dimitern wrote: > remove this - URL will be "" when not initialized anyway. I personally really like being explicit about all the fields that you are initializing and what fields you can expect to be what later on. I realize you don't have to set fields to their null value, but then I wouldn't have to supply the 0 above either. I think that would be more confusing then helpful. At one point, I was thinking to set URL values, so you could use it for comparison, etc. If you find them really ugly, I can remove them, but I prefer being explicit here, so you don't have to cross-reference the full nested definition of state.Tools (which embeds 2 levels deep) to figure out what attributes are available. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:70: var onlyOneTests = []struct { On 2013/03/27 15:06:38, dimitern wrote: > move this outside the test func maybe? (all cases below) I intentionally had it inside. The aren't useful globally (nobody else needs to see them), and it didn't seem helpful to pollute the package namespace. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:91: {all: []*state.Tools{t1000precise, t1900quantal}, On 2013/03/27 15:06:38, dimitern wrote: > this reads harder than: > { > all: ..., > best: ..., > }, done
Sign in to reply to this message.
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: // SyncToolsCommand copies all the tools from the us-east-1 bucket to the local On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > why us-east-1 precisely? > > That is the official bucket that Dave Cheney uploads to. Thanks for explaining, fair enough. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode83 cmd/juju/sync_tools.go:83: res := make([]*state.Tools, 0) On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > maybe res := []*state.Tools{} then? > > I'm pretty sure they are identical. As such it is unclear when constructor > syntax is preferred to make(). (Certainly you have to use make if you want to > set the capacity/length of the underlying array.) > > I think I've pretty much stuck with using make() to create new slices. Ok, it's a matter of choice - both are acceptable. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode113 cmd/juju/sync_tools.go:113: tempFile, err := ioutil.TempFile("", "juju-tgz") On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > how about using a bytes.Buffer to read into instead of a temp file? > > We build the tools into a TempFile, so it seemed we wanted to avoid doing it all > in memory. Its a few MB (2.2MB ATM). I would be ok buffering 2MB, but I know the > Mongo .tgz is more like 40MB. I think it is reasonable to try and avoid > buffering whole files in memory as much as we can. I don't think this will be such an issue, but fair enough, up to you. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode137 cmd/juju/sync_tools.go:137: if !dryRun { On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > maybe it'll be better to: > > > > if dryRun -> return nil > > then copy (collapsing a level of indentation). > > Actually if dryRun => continue since you want to print all the files you would > copy. > I had that originally (only I had the logic inverted with !dryRun) so I ended up > nesting it. > I'll invert the logic and pull things up a level. Thanks! https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode149 cmd/juju/sync_tools.go:149: log.Infof("Failed to create officialEnviron") On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > prefix, lowercase, also a bit of rewording I think: > > "cmd/juju: cannot create (source? origin?) environment" ? > > > > you're masking the original error here. > > in fact, why bother and not return err instead? > > I'm returning err immediately after this, so I don't quite understand your > point. > > The main reason I added this log message, is because you can get a failure to do > with credentials, etc. But you don't know whether it failed to create the > "officialEnviron" or to create the target environ. > > I did change the wording a bit. > Thanks, I prefer sticking to the usual format for log messages. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode165 cmd/juju/sync_tools.go:165: fmt.Printf("Found: %s\n", t) On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > log.Debugf? > > So the issue is trying to figure out, when you just run the command, what should > it display if you *don't* supply --debug. > > Without --debug, if you don't Printf, you get 0 messages. > > I'm actually thinking this could probably be turned into debug messages, but > some of the actual progress messages should be printed. Something like: > > listing source bucket > found %d tools > listing target bucket > found %d tools, %d tools missing > copying tools/foo-1.12.10.tgz (1242kB) > copying tools/foo-1.13.10.tgz (2242kB) > ... > > Mostly because those are the bits that sit waiting on RPC for a while. Does that > seem good to you? You can use fmt.Fprtinf(ctx.Stdout, "...") to output these, so the user can see them, rather than log messages, which (I'm told just today) are basically dropped. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go File cmd/juju/sync_tools_test.go (right): https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:28: opc, errc := runCommand(new(SyncToolsCommand), "-h") On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > i think it's better to encapsulate the run call into its own runSyncTools, > like > > other commands do; this way it could be reused in other cmd tests if need be. > > There are quite a few commands tested in this way. I'd be happy to refactor once > we have more than one test that actually needs the helper. > Good point. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:42: URL: "", On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > remove this - URL will be "" when not initialized anyway. > > I personally really like being explicit about all the fields that you are > initializing and what fields you can expect to be what later on. > > I realize you don't have to set fields to their null value, but then I wouldn't > have to supply the 0 above either. I think that would be more confusing then > helpful. > > At one point, I was thinking to set URL values, so you could use it for > comparison, etc. > > If you find them really ugly, I can remove them, but I prefer being explicit > here, so you don't have to cross-reference the full nested definition of > state.Tools (which embeds 2 levels deep) to figure out what attributes are > available. > Well, I think it's kinda redundant and not having to look at the definition to know what's supposed to be in the struct is not a strong enough point. Whoever is looking at the code has to cross check definitions all the time anyway. If you really prefer to leave them there because it makes the tests more explicit and readable, ok. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:70: var onlyOneTests = []struct { On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > move this outside the test func maybe? (all cases below) > > I intentionally had it inside. The aren't useful globally (nobody else needs to > see them), and it didn't seem helpful to pollute the package namespace. This is a test package and we do this all the time, it's not like anybody is using these outside and they are private to the package anyway. On the flip side, sometimes it's nicer to have data related to a test close (or even inside) the test case itself - this may be such case. I'm not strongly against it anyway. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:142: res := findMissing([]*state.Tools{t1000precise, t1000quantal}, On 2013/03/27 15:06:38, dimitern wrote: > \n after ( for better readability? This however I think is worth reformatting as suggested.
Sign in to reply to this message.
LGTM, but I'd like a Go expert to comment on the explicit initialisation of "" strings and 0 ints. 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#newcode113 cmd/juju/sync_tools.go:113: tempFile, err := ioutil.TempFile("", "juju-tgz") On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > how about using a bytes.Buffer to read into instead of a temp file? > > We build the tools into a TempFile, so it seemed we wanted to avoid doing it all > in memory. Its a few MB (2.2MB ATM). I would be ok buffering 2MB, but I know the > Mongo .tgz is more like 40MB. I think it is reasonable to try and avoid > buffering whole files in memory as much as we can. Hopefully mongo is now being packaged properly so it's really down to a 2MB file. I lean to just doing it in memory but it's your call. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode165 cmd/juju/sync_tools.go:165: fmt.Printf("Found: %s\n", t) On 2013/03/27 18:25:44, dimitern wrote: > On 2013/03/27 16:20:40, jameinel wrote: > > On 2013/03/27 15:06:38, dimitern wrote: > > > log.Debugf? > > > > So the issue is trying to figure out, when you just run the command, what > should > > it display if you *don't* supply --debug. > > > > Without --debug, if you don't Printf, you get 0 messages. > > > > I'm actually thinking this could probably be turned into debug messages, but > > some of the actual progress messages should be printed. Something like: > > > > listing source bucket > > found %d tools > > listing target bucket > > found %d tools, %d tools missing > > copying tools/foo-1.12.10.tgz (1242kB) > > copying tools/foo-1.13.10.tgz (2242kB) > > ... > > > > Mostly because those are the bits that sit waiting on RPC for a while. Does > that > > seem good to you? > > You can use fmt.Fprtinf(ctx.Stdout, "...") to output these, so the user can see > them, rather than log messages, which (I'm told just today) are basically > dropped. We do need to ensure we use the ctx.Stdout I think. So I'd like to see the fmt.Printf stuff replaced. https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go File cmd/juju/sync_tools_test.go (right): https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools_test.go#new... cmd/juju/sync_tools_test.go:42: URL: "", On 2013/03/27 16:20:40, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > remove this - URL will be "" when not initialized anyway. > > I personally really like being explicit about all the fields that you are > initializing and what fields you can expect to be what later on. > > I realize you don't have to set fields to their null value, but then I wouldn't > have to supply the 0 above either. I think that would be more confusing then > helpful. > > At one point, I was thinking to set URL values, so you could use it for > comparison, etc. > > If you find them really ugly, I can remove them, but I prefer being explicit > here, so you don't have to cross-reference the full nested definition of > state.Tools (which embeds 2 levels deep) to figure out what attributes are > available. > AFAIK, idiomatic Go allows you to "know" that strings will be initialised to "" and ints to 0 by default. I see the point about exposing the available attributes but that argument could be used everywhere in Go. I lean to removing the URL: "" and even the Minor: 0 stuff simply because it doesn't seem to be the done thing. I'd like input from one of the Go experts to make the final decision. https://codereview.appspot.com/8051043/diff/6001/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/6001/cmd/juju/sync_tools.go#newco... cmd/juju/sync_tools.go:35: to run without having to access Amazon. Sometimes this isbecause the isbecause -> is because
Sign in to reply to this message.
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, tool.URL) On 2013/03/27 15:06:38, dimitern wrote: > s/C/c/ + package prefix So I didn't add the package prefix, because logging is adding the command prefix. So you end up with: <date> <time> DEBUG JUJU:juju:sync-tools message If I added 'sync-tools:' as the package prefix you end up with: <date> <time> DEBUG JUJU:juju:sync-tools sync_tools: message which certainly doesn't aid clarity (IMO). If it was code outside of the actual sync-tools, then I would be more likely to add it. Does that seem reasonable? https://codereview.appspot.com/8051043/diff/1/cmd/juju/sync_tools.go#newcode165 cmd/juju/sync_tools.go:165: fmt.Printf("Found: %s\n", t) I added a bunch of fmt.Fprintf(ctx.Stdout) calls. Roughly following the above. https://codereview.appspot.com/8051043/diff/6001/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/6001/cmd/juju/sync_tools.go#newco... cmd/juju/sync_tools.go:35: to run without having to access Amazon. Sometimes this isbecause the On 2013/03/28 04:50:07, wallyworld wrote: > isbecause -> is because done
Sign in to reply to this message.
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, tool.URL) On 2013/03/28 08:12:32, jameinel wrote: > On 2013/03/27 15:06:38, dimitern wrote: > > s/C/c/ + package prefix > > So I didn't add the package prefix, because logging is adding the command > prefix. So you end up with: > > <date> <time> DEBUG JUJU:juju:sync-tools message > > If I added 'sync-tools:' as the package prefix you end up with: > <date> <time> DEBUG JUJU:juju:sync-tools sync_tools: message > > which certainly doesn't aid clarity (IMO). > > If it was code outside of the actual sync-tools, then I would be more likely to > add it. > > Does that seem reasonable? my idea was to use "cmd/juju" as prefix, not "sync_tools" (like all other cmds use). The automatic prefix only denotes the command name, so having: <data> <time> DEBUG JUJU:juju:sync-tools cmd/juju: message is not as bad as it seems IMHO.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
This is great, but NOT LGTM without a test. Ping me if it seems harder to write than it should be, we might be able to figure something out. https://codereview.appspot.com/8051043/diff/6001/cmd/juju/sync_tools.go File cmd/juju/sync_tools.go (right): https://codereview.appspot.com/8051043/diff/6001/cmd/juju/sync_tools.go#newco... cmd/juju/sync_tools.go:35: to run without having to access Amazon. Sometimes this isbecause the s/isb/is b/ 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#newc... cmd/juju/sync_tools.go:16: // bucket s/bucket/bucket./ https://codereview.appspot.com/8051043/diff/15001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:124: target environs.Storage, dryRun bool, ctx *cmd.Context) error { FWIW I find the following style: func foo( long, list, of, args, ) error { blah(args) ... } ...much easier to read. https://codereview.appspot.com/8051043/diff/15001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:143: fmt.Fprintf(ctx.Stdout, "listing the source bucket\n") You know, we should be doing this a lot more. Thanks. https://codereview.appspot.com/8051043/diff/15001/cmd/juju/sync_tools_test.go File cmd/juju/sync_tools_test.go (right): https://codereview.appspot.com/8051043/diff/15001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:95: }{ If you glom the next { up here... https://codereview.appspot.com/8051043/diff/15001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:98: } ...and the preceding one in here, I think the test struct will be formatted more clearly.
Sign in to reply to this message.
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#newc... cmd/juju/sync_tools.go:124: target environs.Storage, dryRun bool, ctx *cmd.Context) error { On 2013/03/30 05:35:28, fwereade wrote: > FWIW I find the following style: > > func foo( > long, list, of, args, > ) error { > blah(args) > ... > } > > ...much easier to read. done
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
On 2013/03/30 05:35:28, fwereade wrote: > This is great, but NOT LGTM without a test. Ping me if it seems harder to write > than it should be, we might be able to figure something out. So I did end up adding a few tests. I set up some dummy environments, pop some tools in, and assert that they get copied. I think the setup is a bit clumsy (dummy envs always get fake tools put in there, and I didn't see a way to disable it to make the test more straightforward, etc.) But overall not as bad as I thought it would be. dummy envs do have a public and private, and each one has its own storage namespaced on the dummy env name. I didn't add tests about what gets written to the user. I was a little worried that it would be overly specific rather than being helpful. Do you want to see that sort of test? Also, there is a bit of common code between the tests, because I wanted to keep the "assert preconditions, do something, assert postconditions". I suppose we could just have one test that asserts preconditions based on the setup, and then have the other tests just assume that state?
Sign in to reply to this message.
Essentially LGTM, but quite a lot of random notes for your consideration. None of them's a blocker in itself though. 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#newc... cmd/juju/sync_tools.go:20: targetToolsList *environs.ToolsList I'd suggest keeping these fields off the type -- they're only used in Run and would be better off local, I think. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:22: dryRun bool It *would* be nice, though, to expose a --source flag that let you pick the environment to take tools from. Maybe add a bug for that? https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:49: return nil return cmd.CheckEmpty(args) https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:58: "authorized-keys": "not-really", // We shouldn't need ssh access /me sighs about environment config https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:100: target environs.Storage, ctx *cmd.Context) error { Since it seems you don't hate the concept entirely, I'd find this easier to read if it were formatted like copyTools below :). https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:103: toolFile, err := source.Get(toolsPath) srcFile? https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:110: buf := bytes.NewBuffer(nil) I'd prefer to put this in a temp file, we don't know how big charms might be. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:116: fmt.Fprintf(ctx.Stdout, ", download %dkB, uploading\n", (nBytes+512)/1024) Might be a little bit clearer to just output multiple lines: downloading %v downloaded %dkB uploading https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:129: log.Infof("cmd/juju: copying %s from %s\n", tool.Binary, tool.URL) I don't think you need the \n in Infof? https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go File cmd/juju/sync_tools_test.go (right): https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:173: var t1000precise = &state.Tools{ With something like: func mustParseTools(s string) *state.Tools ...this might look quite nice as a var block. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:211: for _, t := range onlyOneTests { FWIW, I would probably write this as: for i, t := range []*state.Tools { t1000precise, t1000quantal, t1900quantal, t2000precise, } { c.Logf("test %d", i) // ... } https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:220: var oneBestTests = []struct { ...and I'd probably make these anonymous: I don't think it sacrifices clarity, and it prevents one from accidentally giving it an unhelpful name ;p. I'd also roll this one into the next one, to get some immediate benefit from all the infrastructure: for i, t := range []struct{ all []*state.Tools best []*state.Tools }{ { all: toolsList(t1000precise, t1900quantal), best: toolsList(t1900quantal), }, { all: toolsList(t1000precise, t1000quantal), best: toolsList(t1000precise, t1000quantal), }, } { c.Logf("test %d", i) // ... } ...or alternatively just not bother with the table structure until it was needed. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:252: var allMissingTests = []struct { for i, t := range [][]*state.Tools { toolsList(...), } { ...I'll stop suggesting this now :). https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:287: c.Assert(res[0], Equals, t1000quantal) Can we have some explicit tests that arch differences and series differences are treated similarly please?
Sign in to reply to this message.
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#newc... cmd/juju/sync_tools.go:20: targetToolsList *environs.ToolsList On 2013/04/01 22:30:24, fwereade wrote: > I'd suggest keeping these fields off the type -- they're only used in Run and > would be better off local, I think. Yeah, I was cribbing from another command, but I ended up implementing all the functionality as local functions, rather than members, so it doesn't help to put them on here. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:22: dryRun bool On 2013/04/01 22:30:24, fwereade wrote: > It *would* be nice, though, to expose a --source flag that let you pick the > environment to take tools from. Maybe add a bug for that? It was certainly something I was trying to sort out https://bugs.launchpad.net/juju-core/+bug/1163164 https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:49: return nil On 2013/04/01 22:30:24, fwereade wrote: > return cmd.CheckEmpty(args) done https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:100: target environs.Storage, ctx *cmd.Context) error { On 2013/04/01 22:30:24, fwereade wrote: > Since it seems you don't hate the concept entirely, I'd find this easier to read > if it were formatted like copyTools below :). Done https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:110: buf := bytes.NewBuffer(nil) On 2013/04/01 22:30:24, fwereade wrote: > I'd prefer to put this in a temp file, we don't know how big charms might be. This is only the tools, and not charms. I started in a temp file (which is how PutTools does it), and the feedback from the first 2 reviews was that people wanted a bytes.Buffer. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:116: fmt.Fprintf(ctx.Stdout, ", download %dkB, uploading\n", (nBytes+512)/1024) When *using* it, I found having a single line per file was quite nice. Especially if you do '--all' to fully copy the bucket. I can change it, and I especially can change it if we go to logging. But as a UI, I found it pretty. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:129: log.Infof("cmd/juju: copying %s from %s\n", tool.Binary, tool.URL) On 2013/04/01 22:30:24, fwereade wrote: > I don't think you need the \n in Infof? No, it was a carry over from when this was probably a printf. Thanks for the catch. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go File cmd/juju/sync_tools_test.go (right): https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:173: var t1000precise = &state.Tools{ I had thought that static definitions would look readable, but it turns out nested structs look like crap. So thanks for the hint, it looks a lot better now. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:220: var oneBestTests = []struct { ... > ...or alternatively just not bother with the table structure until it was > needed. I tried both. I found the test much easier to read as 2 simple test cases. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:252: var allMissingTests = []struct { On 2013/04/01 22:30:24, fwereade wrote: > for i, t := range [][]*state.Tools { > toolsList(...), > } { > You've mentioned toolsList() rather than []*state.Tools. The function doesn't exist (though it is trivial). It doesn't make it much shorter (9 chars vs 14). I suppose it has fewer magic artifacts []*{}. Is it common to write these helpers? I'm used to python where a list of X is just a list, so you don't need local type declarations, and go seems like it is *this* close to being able to do the same. It can infer the type of vars, but not the type of a list of vars. > ...I'll stop suggesting this now :). https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:287: c.Assert(res[0], Equals, t1000quantal) On 2013/04/01 22:30:24, fwereade wrote: > Can we have some explicit tests that arch differences and series differences are > treated similarly please? Certainly. When I wrote these, Arch was always "", but I had to add it when writing the command level tests, because otherwise things don't like to parse "1.0.0-precise-". Just an oversight not to add tests that Arch isn't part of the Latest check, but is tracked for copying.
Sign in to reply to this message.
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#newc... cmd/juju/sync_tools.go:110: buf := bytes.NewBuffer(nil) On 2013/04/02 08:48:22, jameinel wrote: > On 2013/04/01 22:30:24, fwereade wrote: > > I'd prefer to put this in a temp file, we don't know how big charms might be. > > This is only the tools, and not charms. I started in a temp file (which is how > PutTools does it), and the feedback from the first 2 reviews was that people > wanted a bytes.Buffer. Then I defer to the wisdom of the crowd. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:116: fmt.Fprintf(ctx.Stdout, ", download %dkB, uploading\n", (nBytes+512)/1024) On 2013/04/02 08:48:22, jameinel wrote: > > When *using* it, I found having a single line per file was quite nice. > Especially if you do '--all' to fully copy the bucket. > > I can change it, and I especially can change it if we go to logging. But as a > UI, I found it pretty. Sounds sane to me. https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go File cmd/juju/sync_tools_test.go (right): https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:220: var oneBestTests = []struct { On 2013/04/02 08:48:22, jameinel wrote: > > ... > > ...or alternatively just not bother with the table structure until it was > > needed. > > I tried both. I found the test much easier to read as 2 simple test cases. LGTM https://codereview.appspot.com/8051043/diff/26001/cmd/juju/sync_tools_test.go... cmd/juju/sync_tools_test.go:252: var allMissingTests = []struct { On 2013/04/02 08:48:22, jameinel wrote: > On 2013/04/01 22:30:24, fwereade wrote: > > for i, t := range [][]*state.Tools { > > toolsList(...), > > } { > > > > You've mentioned toolsList() rather than []*state.Tools. > > The function doesn't exist (though it is trivial). It doesn't make it much > shorter (9 chars vs 14). I suppose it has fewer magic artifacts []*{}. > > Is it common to write these helpers? I'm used to python where a list of X is > just a list, so you don't need local type declarations, and go seems like it is > *this* close to being able to do the same. It can infer the type of vars, but > not the type of a list of vars. > > > ...I'll stop suggesting this now :). > Sorry, toolsList is a hangover from something I'd been thinking about before.
Sign in to reply to this message.
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#newc... cmd/juju/sync_tools.go:62: var curBest *state.Tools = nil Since you only care about the Number part in order to determine the best match, how about using something like: var currentBest version.Number Then you can just ignore the nil option and just check for less... if currentBest.Less(tool.Number) { // ... currentBest = tool.Number } I'd prefer a comment about why the capacity of the result is 1, especially when the looping seems to indicate that you are adding multiple values. This was initially confusing to me. Perhaps we should have a variable or constant used instead indicating the expected number of tools that match the version number? Or alternatively, don't use make at all, and instead do... if currentBest.Less(tool.Number) { // This tool has a later version number, so reset the result list. res = []*state.Tools{tool} currentBest = tool.Number } Since you don't set add to true, you don't add it again at the end. Actually, with an extra else, the loop drops out even more simply... for _, tool := range fullTools { if currentBest.Less(tool.Number) { // This tool has a later version number, so reset the result list. res = []*state.Tools{tool} currentBest = tool.Number } else if currentBest == tool.Number { res = append(res, tool) } } https://codereview.appspot.com/8051043/diff/33001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:100: target environs.Storage, ctx *cmd.Context, If we are going to split this over multiple lines, can we have each parameter on its own line? https://codereview.appspot.com/8051043/diff/33001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:130: log.Infof("cmd/juju: copying %s from %s", tool.Binary, tool.URL) Probably don't need the "cmd/juju:" prefix to the info.
Sign in to reply to this message.
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#newc... cmd/juju/sync_tools.go:62: var curBest *state.Tools = nil On 2013/04/02 21:48:27, thumper wrote: > Since you only care about the Number part in order to determine the best match, > how about using something like: > > var currentBest version.Number > It means you assume a lowest number, but that is practically fine. 0.0.0.0 should be earlier than all normal version numbers. > Then you can just ignore the nil option and just check for less... > > if currentBest.Less(tool.Number) { > // ... > currentBest = tool.Number > } > > I'd prefer a comment about why the capacity of the result is 1, especially when > the looping seems to indicate that you are adding multiple values. This was > initially confusing to me. Perhaps we should have a variable or constant used > instead indicating the expected number of tools that match the version number? > Or alternatively, don't use make at all, and instead do... > > if currentBest.Less(tool.Number) { > // This tool has a later version number, so reset the result list. > res = []*state.Tools{tool} > currentBest = tool.Number > } > > Since you don't set add to true, you don't add it again at the end. Actually, > with an extra else, the loop drops out even more simply... > > for _, tool := range fullTools { > if currentBest.Less(tool.Number) { > // This tool has a later version number, so reset the result list. > res = []*state.Tools{tool} > currentBest = tool.Number > } else if currentBest == tool.Number { > res = append(res, tool) > } > } > As for the rest, I made the change, but I have a strong feeling of "meh". It is a little bit nicer, but enough to actually change it? https://codereview.appspot.com/8051043/diff/33001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:100: target environs.Storage, ctx *cmd.Context, On 2013/04/02 21:48:27, thumper wrote: > If we are going to split this over multiple lines, can we have each parameter on > its own line? I would go with precedence, but "grep func.*($" only shows these two functions. It isn't my preferred style, so I don't really care how it is written, but I'm on about my 3rd coat of paint here, and the bikeshed is looking a bit too shiny. https://codereview.appspot.com/8051043/diff/33001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:130: log.Infof("cmd/juju: copying %s from %s", tool.Binary, tool.URL) On 2013/04/02 21:48:27, thumper wrote: > Probably don't need the "cmd/juju:" prefix to the info. It was explicitly requested in earlier reviews. There is discussion on the list about changing all of it, I'm trying not to just be different because we like to do it a new way. I certainly don't like it, and I agree with Gustavo's request to remove a lot of the extra (IMO) noise.
Sign in to reply to this message.
LGTM with a few trivials below. i have intentially not commented on any of the messages it prints - we can sort that out later. 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:62: // This assumes the nil version of Number is always less than a real s/nil/zero/ https://codereview.appspot.com/8051043/diff/40001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:65: var res []*state.Tools = nil s/= nil// https://codereview.appspot.com/8051043/diff/40001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:81: var target = make(map[version.Binary]bool, len(targetTools)) s/var target = /target := / https://codereview.appspot.com/8051043/diff/40001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:85: res := make([]*state.Tools, 0) s/, 0// or (preferably): var res []*state.Tools to match findNewest. https://codereview.appspot.com/8051043/diff/40001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:87: if present := target[tool.Binary]; !present { if !target[tool.Binary] { https://codereview.appspot.com/8051043/diff/40001/cmd/juju/sync_tools.go#newc... cmd/juju/sync_tools.go:107: buf := bytes.NewBuffer(nil) var buf bytes.Buffer would be more usual (we usually use NewBuffer only if there's existing data)
Sign in to reply to this message.
Please land this with stderr feedback; we will probably end up doing something that is different in some way, but there's no sense blocking this change on that discussion.
Sign in to reply to this message.
*** Submitted: 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. R=dimitern, wallyworld, fwereade, thumper, rog CC= https://codereview.appspot.com/8051043 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 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. 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?
Sign in to reply to this message.
*** Submitted: 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. R= CC= https://codereview.appspot.com/8051043
Sign in to reply to this message.
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.
|