Code review comment for lp:~thumper/juju-core/find-jujud

Working on writing extra tests now.

https://codereview.appspot.com/11326043/diff/1/environs/localstorage/storage.go
File environs/localstorage/storage.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/localstorage/storage.go#newcode95
environs/localstorage/storage.go:95: // struct so the Close method is
not exposed.
On 2013/07/16 11:09:50, fwereade wrote:
> On 2013/07/16 09:43:00, gz wrote:
> > Unrelated drive-by? Kind of thing I'd like a failing test for.

> +1

Was needed as part of this branch as Putting from a file into storage,
which has a Close method, caused the Seek(0,0) as the localstorage used
http.NewRequest, which had the side effect of closing the file.

I've added a test now to confirm that Close isn't called.

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go
File environs/tools/build.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcode131
environs/tools/build.go:131: if info.Mode()&0111 != 0 {
On 2013/07/16 11:09:50, fwereade wrote:
> On 2013/07/16 09:43:00, gz wrote:
> > I like spaces even around bitops.

> I think go fmt decides that for you.

I like spaces too, but go fmt doesn't.

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcode212
environs/tools/build.go:212: if err := copyExistingJujud(dir); err !=
nil {
On 2013/07/16 11:21:33, fwereade wrote:
> Actually, this has percolated a bit and I'm not so happy. This is
quite a big
> and magical behaviour change... I have on a few occasions
needed/wanted to
> upload tools that *don't* match the juju binary I'm using, and this
kinda breaks
> that. Can still be worked around; and should probably only affect
developers;
> but I'm starting to think that it's wrong to piggyback on the
upload-tools
> mechanism for this use case.

> I should be around tonight to talk about it a little... my big concern
is that
> we're changing behavior that *will* affect all the users who are using
> upload-tools. Maybe I can be convinced that it's only a little, and
maybe even a
> beneficial, change; but let's discuss tonight.

As discussed, we'll go with this for now.

https://codereview.appspot.com/11326043/diff/1/environs/tools/storage.go
File environs/tools/storage.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/tools/storage.go#newcode49
environs/tools/storage.go:49: logger.Debugf("reading v%d.* tools",
majorVersion)
On 2013/07/16 09:43:00, gz wrote:
> Having logger declared in build.go and used without import in
storage.go is
> slightly confusing. I will get my head around the go package model at
some
> point.

Yeah. I'd really prefer the ability to have file local variables, but
go doesn't do that.

https://codereview.appspot.com/11326043/