environs: allow PutTools to specify a version.

environs: allow PutTools to specify a version.



https://code.launchpad.net/~rogpeppe/juju-core/029-environs-fix-puttools/+merge/120165

(do not edit description out of merge proposal)

[rog, 2012-08-17 13:29:56]

Please take a look.

[rog, 2012-08-17 14:49:58]

Please take a look.

[niemeyer, 2012-08-17 15:22:06]

So much nicer, thanks a lot rog.

https://codereview.appspot.com/6441158/diff/7001/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6441158/diff/7001/environs/tools.go#newcode63
environs/tools.go:63: // If vers is non-nil, a FORCE_VERSION file will be
written into the
We don't have to explain the implementation here. It can change while preserving
the intended semantics:

// If vers is provided the uploaded tools are forced into being that
// version rather than the real version in version.Current.

https://codereview.appspot.com/6441158/diff/7001/environs/tools.go#newcode65
environs/tools.go:65: // TODO(rog) find binaries from $PATH when not using a
development
This should be moved onto the function itself so the content isn't inlined into
the documentation.

https://codereview.appspot.com/6441158/diff/7001/environs/tools_test.go
File environs/tools_test.go (right):

https://codereview.appspot.com/6441158/diff/7001/environs/tools_test.go#newco...
environs/tools_test.go:120: // This test actually tests three things:
It's a nice test, by the way.

https://codereview.appspot.com/6441158/diff/7001/version/version.go
File version/version.go (right):

https://codereview.appspot.com/6441158/diff/7001/version/version.go#newcode17
version/version.go:17: // If the file "FORCE-VERSION" is present in the same
I guess it's nice to have this here, though.

https://codereview.appspot.com/6441158/diff/7001/version/version.go#newcode29
version/version.go:29: return
Hmm.. an error while reading it is fine, but if it contains invalid data we
panic? Seems inconsistent. Either it's fine to ignore a bogus file, or it's not,
IMO. I'd vote for panicking in both cases, since this is rarely used and
ignoring such an error will make us dig for what's wrong longer.

[niemeyer, 2012-08-17 15:22:16]

LGTM, btw.

[rog, 2012-08-17 15:37:39]

*** Submitted:

environs: allow PutTools to specify a version.

R=niemeyer
CC=
https://codereview.appspot.com/6441158

https://codereview.appspot.com/6441158/diff/7001/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6441158/diff/7001/environs/tools.go#newcode63
environs/tools.go:63: // If vers is non-nil, a FORCE_VERSION file will be
written into the
On 2012/08/17 15:22:06, niemeyer wrote:
> We don't have to explain the implementation here. It can change while
preserving
> the intended semantics:
> 
> // If vers is provided the uploaded tools are forced into being that
> // version rather than the real version in version.Current.

Done.

https://codereview.appspot.com/6441158/diff/7001/environs/tools.go#newcode65
environs/tools.go:65: // TODO(rog) find binaries from $PATH when not using a
development
On 2012/08/17 15:22:06, niemeyer wrote:
> This should be moved onto the function itself so the content isn't inlined
into
> the documentation.

Done.

https://codereview.appspot.com/6441158/diff/7001/version/version.go
File version/version.go (right):

https://codereview.appspot.com/6441158/diff/7001/version/version.go#newcode29
version/version.go:29: return
On 2012/08/17 15:22:06, niemeyer wrote:
> Hmm.. an error while reading it is fine, but if it contains invalid data we
> panic? Seems inconsistent. Either it's fine to ignore a bogus file, or it's
not,
> IMO. I'd vote for panicking in both cases, since this is rarely used and
> ignoring such an error will make us dig for what's wrong longer.

Done.