juju: add code for building and uploading juju client

juju: add code for building and uploading juju client



https://code.launchpad.net/~rogpeppe/juju/go-dummy-upload/+merge/103909

(do not edit description out of merge proposal)

[rog, 2012-04-27 15:57:00]

Please take a look.

[fwereade, 2012-04-27 16:12:53]

LGTM... I'm suspicious of a couple of bits, but I don;t have any better ideas ;)

https://codereview.appspot.com/6128046/diff/2001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6128046/diff/2001/environs/dummy/environs.go#n...
environs/dummy/environs.go:24: Version version.Version
I'm a bit suspicious of adding these for only one use case, but we'll see how it
matures ;).

https://codereview.appspot.com/6128046/diff/2001/juju/tools.go
File juju/tools.go (right):

https://codereview.appspot.com/6128046/diff/2001/juju/tools.go#newcode17
juju/tools.go:17: // tarHeader returns a file header given the
incomplete?

https://codereview.appspot.com/6128046/diff/2001/juju/tools.go#newcode81
juju/tools.go:81: // Find out the juju root by introspecting a locally defined
type.
Eww!

...but I'm not aware of a better way to do something like this.

[rog, 2012-04-27 16:20:21]

Please take a look.

[niemeyer, 2012-04-27 19:40:43]

This is going well, but I suggest a few tweaks in the direction:

https://codereview.appspot.com/6128046/diff/7001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6128046/diff/7001/environs/dummy/environs.go#n...
environs/dummy/environs.go:21: // Valid for OpUploadTools only. The receiver
must close the reader
Please see comment about UploadTools. We need better support for PutFile here
instead.

Rather than having more data in this struct, I'd prefer we had a dummy.GetFile
function that allowed pulling the given file at any point. This is also nice
because it disconnects implementation from testing in some ways (don't care
about which ops.. care about the file being there).

https://codereview.appspot.com/6128046/diff/7001/juju/export_test.go
File juju/export_test.go (right):

https://codereview.appspot.com/6128046/diff/7001/juju/export_test.go#newcode3
juju/export_test.go:3: func SetJujuRoot(new string) (old string) {
Doesn't seem necessary. Please see comment in the respective test.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go
File juju/tools.go (right):

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode34
juju/tools.go:34: // represents a regular file executable by (at least) the
user.
s/(at least)//; implementation checks user bit, only.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode40
juju/tools.go:40: // directory in gzipped tar format to w.
This should probably be called archiveExecutables, and we should also be
explicit in docs about the fact this doesn't work at all if there's more
content:

// An error is returned if an entry inside dir is not a
// regular executable file.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode55
juju/tools.go:55: panic(fmt.Errorf("go install has created non-executable file
%q", ent.Name()))
Surprised to see a panic here. This function is also not associated with go
install itself. Sounds like this should be something like:

return fmt.Errorf("found non-executable %q inside directory %q", ent.Name(),
dir)

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode85
juju/tools.go:85: defer f.Close()
Use closeErrorCheck here?

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode90
juju/tools.go:90: var jujuRoot string
Not worth the trouble, IMO. This should do:

const cmdPath = "launchpad.net/juju/go/cmd"

If the base for that changes, we'll have several other imports changing as well,
even in this file.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode123
juju/tools.go:123: 
// UploadTools ...

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode124
juju/tools.go:124: func (c *Conn) UploadTools() error {
The implementation of this would probably be more suitable for
environs.UploadTools(environ), even if we have this as a wrapper here still.
This would be in similar spirit to Bootstrap, etc, with the exception that we
have a common implementation to all environments.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode127
juju/tools.go:127: // so that we can be sure we have archived correctly.
This needs re-flowing.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode146
juju/tools.go:146: return c.Environ.UploadTools(f, fi.Size(), version.Current)
Sorry for not observing that earlier, but we've made a mistake in this API.
There's really no point in reimplementing UploadTools on every single provider
when we already have a perfectly good API that is used in many circumstances for
similar scenarios.

The call above ought to be:

return c.Environ.PutFile(fmt.Sprintf("tools/juju-%s-%s.tar.gz", version, arch),
f, fi.Size())

BOOM! All environments supported, consistently. Feel free to drop the dummy
UploadTools in this CL already.

https://codereview.appspot.com/6128046/diff/7001/juju/tools_test.go
File juju/tools_test.go (right):

https://codereview.appspot.com/6128046/diff/7001/juju/tools_test.go#newcode24
juju/tools_test.go:24: zookeeper: false
This is way more readable if we put in its own const out of the init function:

const environConf = `
...
`

func init() {
    envs, err = environs.ReadEnvironsBytes([]byte(environConf))
    if err != nil {
        ...
    }
}

https://codereview.appspot.com/6128046/diff/7001/juju/tools_test.go#newcode74
juju/tools_test.go:74: for _, t := range tools {
for _, tool := range []string{"juju", jujud"} {

https://codereview.appspot.com/6128046/diff/7001/juju/tools_test.go#newcode86
juju/tools_test.go:86: oldJujuRoot := juju.SetJujuRoot("-/invalid")
That's a sightly odd thing to be testing, IMO. It's verifying that corrupting
the implementation arbitrarily fails in an arbitrary way.

Unsetting PATH would give you a similar benefit more easily, although I still
question how great that benefit is. Testing failure scenarios is good, but this
test isn't even ensuring anything about the failure except for the fact a vague
error message is returned. Anything could be blowing up, in any way.

[rog, 2012-04-30 14:45:52]

Please take a look.

https://codereview.appspot.com/6128046/diff/7001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6128046/diff/7001/environs/dummy/environs.go#n...
environs/dummy/environs.go:21: // Valid for OpUploadTools only. The receiver
must close the reader
On 2012/04/27 19:40:43, niemeyer wrote:
> Please see comment about UploadTools. We need better support for PutFile here
> instead.
> 
> Rather than having more data in this struct, I'd prefer we had a dummy.GetFile
> function that allowed pulling the given file at any point. This is also nice
> because it disconnects implementation from testing in some ways (don't care
> about which ops.. care about the file being there).

Done.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go
File juju/tools.go (right):

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode34
juju/tools.go:34: // represents a regular file executable by (at least) the
user.
On 2012/04/27 19:40:43, niemeyer wrote:
> s/(at least)//; implementation checks user bit, only.

... which means that the file is executable by the
user at least, and possibly by group and others.
done anyway, as it doesn't add much to the comment.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode40
juju/tools.go:40: // directory in gzipped tar format to w.
On 2012/04/27 19:40:43, niemeyer wrote:
> This should probably be called archiveExecutables, and we should also be
> explicit in docs about the fact this doesn't work at all if there's more
> content:
> 
> // An error is returned if an entry inside dir is not a
> // regular executable file.

Done.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode55
juju/tools.go:55: panic(fmt.Errorf("go install has created non-executable file
%q", ent.Name()))
On 2012/04/27 19:40:43, niemeyer wrote:
> Surprised to see a panic here. This function is also not associated with go
> install itself. Sounds like this should be something like:
> 
> return fmt.Errorf("found non-executable %q inside directory %q", ent.Name(),
> dir)

Done.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode85
juju/tools.go:85: defer f.Close()
On 2012/04/27 19:40:43, niemeyer wrote:
> Use closeErrorCheck here?

i wouldn't bother. the error case when closing a regular file is rare and
usually not a problem - we ignore these errors all the time. the reason for
using closeErrorCheck in archive is that both tar.Writer.Close and
gzip.Writer.Close actually do a write under the covers, and we really don't want
to ignore that.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode90
juju/tools.go:90: var jujuRoot string
On 2012/04/27 19:40:43, niemeyer wrote:
> Not worth the trouble, IMO. This should do:
> 
> const cmdPath = "launchpad.net/juju/go/cmd"
> 
> If the base for that changes, we'll have several other imports changing as
well,
> even in this file.

ok. i was hoping to make it amenable to forking by tools that automatically
rewrite imports, but i don't care too much. done.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode124
juju/tools.go:124: func (c *Conn) UploadTools() error {
On 2012/04/27 19:40:43, niemeyer wrote:
> The implementation of this would probably be more suitable for
> environs.UploadTools(environ), even if we have this as a wrapper here still.
> This would be in similar spirit to Bootstrap, etc, with the exception that we
> have a common implementation to all environments.

Done.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode127
juju/tools.go:127: // so that we can be sure we have archived correctly.
On 2012/04/27 19:40:43, niemeyer wrote:
> This needs re-flowing.

Done.

https://codereview.appspot.com/6128046/diff/7001/juju/tools.go#newcode146
juju/tools.go:146: return c.Environ.UploadTools(f, fi.Size(), version.Current)
On 2012/04/27 19:40:43, niemeyer wrote:
> Sorry for not observing that earlier, but we've made a mistake in this API.
> There's really no point in reimplementing UploadTools on every single provider
> when we already have a perfectly good API that is used in many circumstances
for
> similar scenarios.
> 
> The call above ought to be:
> 
> return c.Environ.PutFile(fmt.Sprintf("tools/juju-%s-%s.tar.gz", version,
arch),
> f, fi.Size())
> 
> BOOM! All environments supported, consistently. Feel free to drop the dummy
> UploadTools in this CL already.

Done.

https://codereview.appspot.com/6128046/diff/7001/juju/tools_test.go
File juju/tools_test.go (right):

https://codereview.appspot.com/6128046/diff/7001/juju/tools_test.go#newcode24
juju/tools_test.go:24: zookeeper: false
On 2012/04/27 19:40:43, niemeyer wrote:
> This is way more readable if we put in its own const out of the init function:
> 
> const environConf = `
> ...
> `
> 
> func init() {
>     envs, err = environs.ReadEnvironsBytes([]byte(environConf))
>     if err != nil {
>         ...
>     }
> }

Done.

https://codereview.appspot.com/6128046/diff/7001/juju/tools_test.go#newcode74
juju/tools_test.go:74: for _, t := range tools {
On 2012/04/27 19:40:43, niemeyer wrote:
> for _, tool := range []string{"juju", jujud"} {

i don't think so. "t" is for "test" here - i'm not sure that there's a single
set of arguments that will result in characteristic for all commands, so we use
the test table above.

i've renamed "tools" to "toolTests" to try to make this clearer.

https://codereview.appspot.com/6128046/diff/7001/juju/tools_test.go#newcode86
juju/tools_test.go:86: oldJujuRoot := juju.SetJujuRoot("-/invalid")
On 2012/04/27 19:40:43, niemeyer wrote:
> That's a sightly odd thing to be testing, IMO. It's verifying that corrupting
> the implementation arbitrarily fails in an arbitrary way.
> 
> Unsetting PATH would give you a similar benefit more easily, although I still
> question how great that benefit is. Testing failure scenarios is good, but
this
> test isn't even ensuring anything about the failure except for the fact a
vague
> error message is returned. Anything could be blowing up, in any way.

you're right - my intention was to test that the error output of the go tool was
added to the error message, but i'm not testing that.

better this time, i hope

[fwereade, 2012-04-30 14:56:49]

LGTM

[rog, 2012-04-30 16:10:06]

Please take a look.

[niemeyer, 2012-04-30 22:13:47]

That's awesome, thanks. LGTM

[rog, 2012-05-01 07:22:40]

*** Submitted:

juju: add code for building and uploading juju client

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