environs: upload juju tools to cloud.

environs: upload juju tools to cloud.



https://code.launchpad.net/~rogpeppe/juju/go-ec2-find-tools/+merge/105382

(do not edit description out of merge proposal)

[rog, 2012-05-10 19:29:36]

Please take a look.

[dave_cheney.net, 2012-05-10 23:23:16]

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/ec2.go#newcode115
environs/ec2/ec2.go:115: if err != nil && s3ErrorStatusCode(err) != 404 {
Is err guaranteed to be of type s3ErrorStatusCode ?

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/export_test.go#...
environs/ec2/export_test.go:14: var VersionCurrentMajor = &versionCurrentMajor
Can't you just refer directly to versionCurrentMajor in local_test.go ?

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/image.go#newcode78
environs/ec2/image.go:78: ImageId:      f[7],
little bit worried about the magic numbers here, i know you're just extending
this code, but could I suggest adding some constants.

https://codereview.appspot.com/6198064/diff/2001/environs/jujutest/test.go
File environs/jujutest/test.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/jujutest/test.go#ne...
environs/jujutest/test.go:78: err := t.Env.Bootstrap(false) // TODO true - i.e.
use Go tools on server side.
// TODO, use Go tools on the server, thus setting this to true.

https://codereview.appspot.com/6198064/diff/2001/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/jujutest/tests.go#n...
environs/jujutest/tests.go:42: func (t *Tests) TestBootstrap(c *C) {
Should there be tests for e.Bootstrap(true) ?

https://codereview.appspot.com/6198064/diff/2001/version/version.go
File version/version.go (right):

https://codereview.appspot.com/6198064/diff/2001/version/version.go#newcode16
version/version.go:16: func ubuntuArch(arch string) string {
What about arm? I'm thinking about armel vs armhf. Maybe it just works.

[niemeyer, 2012-05-15 22:06:40]

This is touched by the conversation we've had on IRC today as well.

[fwereade, 2012-05-17 10:03:46]

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/image.go#newcode33
environs/ec2/image.go:33: OS           string // The OS the image will run on.
Shouldn't this be UbuntuSeries or something?

(unrelated question: why are we messing around with desktop and daily images in
this file?)

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/image.go#newcode80
environs/ec2/image.go:80: OS:           "linux",
As above, surely this should be the ubuntu series... "linux" conveys no useful
information at all AFAICT.

https://codereview.appspot.com/6198064/diff/2001/version/version.go
File version/version.go (right):

https://codereview.appspot.com/6198064/diff/2001/version/version.go#newcode13
version/version.go:13: var CurrentOS = runtime.GOOS
I'm not at all comfortable about this competing "OS" concept... I really think
we should be consistent about using "series" throughout the project until we
have cause to expand our world.

[rog, 2012-05-21 17:40:30]

Please take a look.

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/ec2.go#newcode115
environs/ec2/ec2.go:115: if err != nil && s3ErrorStatusCode(err) != 404 {
On 2012/05/10 23:23:16, dfc wrote:
> Is err guaranteed to be of type s3ErrorStatusCode ?

it's a function, not a type. if it's not the right type, it returns 0.

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/export_test.go
File environs/ec2/export_test.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/export_test.go#...
environs/ec2/export_test.go:14: var VersionCurrentMajor = &versionCurrentMajor
On 2012/05/10 23:23:16, dfc wrote:
> Can't you just refer directly to versionCurrentMajor in local_test.go ?

no, because versionCurrentMajor is inside the ec2 package, but local_test.go is
not. this file (export_test.go) defines internal stuff to be exposed to the
tests only.

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/ec2/image.go#newcode78
environs/ec2/image.go:78: ImageId:      f[7],
On 2012/05/10 23:23:16, dfc wrote:
> little bit worried about the magic numbers here, i know you're just extending
> this code, but could I suggest adding some constants.

you're right; i'm a bit uncomfortable too (the code was originally derived quite
closely from the python).

will make constants.

https://codereview.appspot.com/6198064/diff/2001/environs/jujutest/test.go
File environs/jujutest/test.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/jujutest/test.go#ne...
environs/jujutest/test.go:78: err := t.Env.Bootstrap(false) // TODO true - i.e.
use Go tools on server side.
On 2012/05/10 23:23:16, dfc wrote:
> // TODO, use Go tools on the server, thus setting this to true.

done, kinda.

https://codereview.appspot.com/6198064/diff/2001/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/6198064/diff/2001/environs/jujutest/tests.go#n...
environs/jujutest/tests.go:42: func (t *Tests) TestBootstrap(c *C) {
On 2012/05/10 23:23:16, dfc wrote:
> Should there be tests for e.Bootstrap(true) ?

probably.

https://codereview.appspot.com/6198064/diff/2001/version/version.go
File version/version.go (right):

https://codereview.appspot.com/6198064/diff/2001/version/version.go#newcode16
version/version.go:16: func ubuntuArch(arch string) string {
On 2012/05/10 23:23:16, dfc wrote:
> What about arm? I'm thinking about armel vs armhf. Maybe it just works.

i'll try and find a definitive reference.

[niemeyer, 2012-05-21 21:29:10]

Quite nice, thanks. LGTM, given the below is addressed. Feel free to leave the
renaming to a later branch if you please, but we should really do it. We already
have multiple conventions in place for no reason.

https://codereview.appspot.com/6198064/diff/17001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6198064/diff/17001/cmd/juju/cmd_test.go#newcod...
cmd/juju/cmd_test.go:139: defer dummy.Reset()
Why is this necessary, considering that TearDownTest is doing it too?

https://codereview.appspot.com/6198064/diff/17001/environs/ec2/image.go
File environs/ec2/image.go (right):

https://codereview.appspot.com/6198064/diff/17001/environs/ec2/image.go#newco...
environs/ec2/image.go:33: UbuntuRelease string // The Ubuntu series the image
will run on.
Please see later comment about naming.

https://codereview.appspot.com/6198064/diff/17001/environs/ec2/image_test.go
File environs/ec2/image_test.go (right):

https://codereview.appspot.com/6198064/diff/17001/environs/ec2/image_test.go#...
environs/ec2/image_test.go:76: func (imageSuite) TestFindImageSpec(c *C) {
The implementation changed but the test did not?

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode19
environs/tools.go:19: var CurrentUbuntuRelease = "precise"         // TODO find
out actual version.
version, series, or release? Can we please stick to "series" across the whole
code base whenever we mean the Ubuntu series?

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode20
environs/tools.go:20: var CurrentArch = ubuntuArch(runtime.GOARCH) // TODO
better than this
On the same token, can we stick to "arch" whenever we mean architecture in the
code base? (which means, the above is fine)

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode45
environs/tools.go:45: // TODO find binaries from $PATH when go dev environment
not available.
// TODO find binaries from $PATH when not using a development
// version of juju within a $GOPATH.

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode105
environs/tools.go:105: // readFile writes the contents of the given file to w.
copyFile was a better name ("readFile writes")

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode199
environs/tools.go:199: // found StorageReader and the path within that storage.
"the found StorageReader"? Would be good to clarify this.

https://codereview.appspot.com/6198064/diff/17001/environs/tools_test.go
File environs/tools_test.go (right):

https://codereview.appspot.com/6198064/diff/17001/environs/tools_test.go#newc...
environs/tools_test.go:216: version.Current = originalVersion
Rather than hacking it, it'd be nicer to provide FindTools with a version
number. We'll need this anyway (and arch/series too).

[rog, 2012-05-22 09:00:58]

*** Submitted:

environs: upload juju tools to cloud.

R=dfc, niemeyer, fwereade
CC=
https://codereview.appspot.com/6198064

https://codereview.appspot.com/6198064/diff/17001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6198064/diff/17001/cmd/juju/cmd_test.go#newcod...
cmd/juju/cmd_test.go:139: defer dummy.Reset()
On 2012/05/21 21:29:10, niemeyer wrote:
> Why is this necessary, considering that TearDownTest is doing it too?

Done.

https://codereview.appspot.com/6198064/diff/17001/environs/ec2/image_test.go
File environs/ec2/image_test.go (right):

https://codereview.appspot.com/6198064/diff/17001/environs/ec2/image_test.go#...
environs/ec2/image_test.go:76: func (imageSuite) TestFindImageSpec(c *C) {
On 2012/05/21 21:29:10, niemeyer wrote:
> The implementation changed but the test did not?

Done.

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode19
environs/tools.go:19: var CurrentUbuntuRelease = "precise"         // TODO find
out actual version.
On 2012/05/21 21:29:10, niemeyer wrote:
> version, series, or release? Can we please stick to "series" across the whole
> code base whenever we mean the Ubuntu series?

i'll do this, but in general i'm not particularly keen on using this very
generic term in such a specific way. the vast majority of this code base is not
ubuntu-specific, and for the sake of future readers and porting efforts, i think
it would be nicer if parochial stuff like this was more obviously flagged. using
the word "ubuntu" seems like an easy and clear way to do that.

BTW if i google for "ubuntu series", apart from juju stuff, i get a single hit
(https://launchpad.net/ubuntu/+series), and even in there it refers to each
version as an "ubuntu release". it seems to me that using the term "series" to
refer to a particular ubuntu release is not greatly widespread.

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode20
environs/tools.go:20: var CurrentArch = ubuntuArch(runtime.GOARCH) // TODO
better than this
On 2012/05/21 21:29:10, niemeyer wrote:
> On the same token, can we stick to "arch" whenever we mean architecture in the
> code base? (which means, the above is fine)

Done.

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode45
environs/tools.go:45: // TODO find binaries from $PATH when go dev environment
not available.
On 2012/05/21 21:29:10, niemeyer wrote:
> // TODO find binaries from $PATH when not using a development
> // version of juju within a $GOPATH.

Done.

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode105
environs/tools.go:105: // readFile writes the contents of the given file to w.
On 2012/05/21 21:29:10, niemeyer wrote:
> copyFile was a better name ("readFile writes")

Done.

https://codereview.appspot.com/6198064/diff/17001/environs/tools.go#newcode199
environs/tools.go:199: // found StorageReader and the path within that storage.
On 2012/05/21 21:29:10, niemeyer wrote:
> "the found StorageReader"? Would be good to clarify this.

Done.

https://codereview.appspot.com/6198064/diff/17001/environs/tools_test.go
File environs/tools_test.go (right):

https://codereview.appspot.com/6198064/diff/17001/environs/tools_test.go#newc...
environs/tools_test.go:216: version.Current = originalVersion
On 2012/05/21 21:29:10, niemeyer wrote:
> Rather than hacking it, it'd be nicer to provide FindTools with a version
> number. We'll need this anyway (and arch/series too).

i didn't do this because AFAIR you originally objected to passing a version to
Environ.FindTools. but i'm very happy to do it now.