environs/...: respect default series

environs/...: respect default series

Rought sketch, not for submission.

https://code.launchpad.net/~dave-cheney/juju-core/044-environs-respect-default-series/+merge/133012

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-11-07 05:23:09]

Please take a look.

[fwereade, 2012-11-09 07:19:52]

LGTM assuming you either write a trivial test for Config.ToolsVersion, or just
inline it.

https://codereview.appspot.com/6819097/diff/2001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6819097/diff/2001/environs/config/config.go#ne...
environs/config/config.go:113: func (c *Config) ToolsVersion() version.Binary {
I don't feel like this is quite accurately described... and I'm not comfortable
with the tendency for our APIs to grow little untested bits (I *know* it's
trivial... but still). How would you feel about just inlining this at the call
site?

[rog, 2012-11-09 11:56:26]

looks reasonable except the PutTools thing.

https://codereview.appspot.com/6819097/diff/2001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6819097/diff/2001/environs/config/config.go#ne...
environs/config/config.go:113: func (c *Config) ToolsVersion() version.Binary {
On 2012/11/09 07:19:52, fwereade wrote:
> I don't feel like this is quite accurately described... and I'm not
comfortable
> with the tendency for our APIs to grow little untested bits (I *know* it's
> trivial... but still). How would you feel about just inlining this at the call
> site?

+1

The entry points in environs/config should be returning info about the
configuration only, IMO.

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

https://codereview.appspot.com/6819097/diff/2001/environs/ec2/ec2.go#newcode235
environs/ec2/ec2.go:235: tools, err = environs.PutTools(e.Storage(), &vers)
i don't think this is right. if we're uploading tools, those tools are specific
to the current series. by doing the above, we're building on one series and
deploying to another, and there's no guarantee that'll work (that's why we have
the series in the version). i think that if the current series does not match
the config default series, we should fail here.

alternatively, we could have the current series override the default series in
this case (it is "default series" after all).

https://codereview.appspot.com/6819097/diff/2001/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6819097/diff/2001/environs/tools.go#newcode414
environs/tools.go:414: log.Debugf("environs: searching for tools: %v", vers)
i think this should probably be log.Printf - it's useful information.
Perhaps for debugging, it might be good to list the actual tools found.

[dave_cheney.net, 2012-11-13 08:18:01]

Still fighting with tests, responses below.

https://codereview.appspot.com/6819097/diff/2001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6819097/diff/2001/environs/config/config.go#ne...
environs/config/config.go:113: func (c *Config) ToolsVersion() version.Binary {
On 2012/11/09 11:56:26, rog wrote:
> On 2012/11/09 07:19:52, fwereade wrote:
> > I don't feel like this is quite accurately described... and I'm not
> comfortable
> > with the tendency for our APIs to grow little untested bits (I *know* it's
> > trivial... but still). How would you feel about just inlining this at the
call
> > site?
> 
> +1
> 
> The entry points in environs/config should be returning info about the
> configuration only, IMO.

Done.

https://codereview.appspot.com/6819097/diff/2001/environs/config/config.go#ne...
environs/config/config.go:113: func (c *Config) ToolsVersion() version.Binary {
On 2012/11/09 07:19:52, fwereade wrote:
> I don't feel like this is quite accurately described... and I'm not
comfortable
> with the tendency for our APIs to grow little untested bits (I *know* it's
> trivial... but still). How would you feel about just inlining this at the call
> site?

Done.

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

https://codereview.appspot.com/6819097/diff/2001/environs/ec2/ec2.go#newcode235
environs/ec2/ec2.go:235: tools, err = environs.PutTools(e.Storage(), &vers)
On 2012/11/09 11:56:26, rog wrote:
> i don't think this is right. if we're uploading tools, those tools are
specific
> to the current series. by doing the above, we're building on one series and
> deploying to another, and there's no guarantee that'll work (that's why we
have
> the series in the version). i think that if the current series does not match
> the config default series, we should fail here.
> 
> alternatively, we could have the current series override the default series in
> this case (it is "default series" after all).

This is all true, but creates a gordian knot which means we, as developers have
to maintain a machine running Precise to deploy development tools for Precise,
one for Quantal, one for Raring, and so forth.

This is clearly impractical, so this logic, which is only used during
--upload-tools which only works if you have the source, is a reasonable
compromise.

The tools will very likely work, as they are 99% pure Go, so I think this
compromise is valid.

https://codereview.appspot.com/6819097/diff/2001/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6819097/diff/2001/environs/tools.go#newcode414
environs/tools.go:414: log.Debugf("environs: searching for tools: %v", vers)
On 2012/11/09 11:56:26, rog wrote:
> i think this should probably be log.Printf - it's useful information.
> Perhaps for debugging, it might be good to list the actual tools found.

Done.

[rog, 2012-11-13 11:22:44]

BTW I don't think you've uploaded the new version yet.

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

https://codereview.appspot.com/6819097/diff/2001/environs/ec2/ec2.go#newcode235
environs/ec2/ec2.go:235: tools, err = environs.PutTools(e.Storage(), &vers)
> The tools will very likely work, as they are 99% pure Go, so I think this
> compromise is valid.

i'm of two minds here. on one hand, yes, it will work 99% of the time. on the
other hand, if we're going to rely on this, then why have we bothered putting
the series in the tools version at all?

as a developer, there's no problem AFAICS with simply deploying to the same
series that you're developing for. if you want to deploy to another series, yes,
you must compile it on that other series. this is the decision we took by making
the series part of the version.

of course, it's unfortunate that charms are series-specific too.

we're trying to have it both ways (pretending that the tools binaries both are
and aren't compatible across series), and i'm uncomfortable with that.

[dave_cheney.net, 2012-11-13 23:19:15]

Thanks for your comments, I'm going to try to solve this another way.