Code review comment for lp:~cmars/juju-core/resolve-cs-series

Looks good, would appreciate at least a short chat before landing -- in
particular, I'm not quite clear on the forces that lead us to use
FakeDefaultSeries sometimes, and LatestLts at others.

https://codereview.appspot.com/80280043/diff/120001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/80280043/diff/120001/cmd/juju/bootstrap_test.go#newcode123
cmd/juju/bootstrap_test.go:123: useVersion :=
strings.Replace(test.version, "%LTS%", config.LatestLtsSeries(), 1)
not quite sure that config is the right package for these -- but I'm not
sure I can think of a better one. Unless you want to create a tiny,
hyper-focused, series package somewhere? I'm sure it'll grow excitingly
as we deal with other OSs...

https://codereview.appspot.com/80280043/diff/120001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/80280043/diff/120001/cmd/juju/upgradecharm.go#newcode209
cmd/juju/upgradecharm.go:209: repo, err :=
charm.InferRepository(newURL.Reference, c.RepoPath)
We still need ctx.AbsPath, I think. Would you change the relevant test
to use a path relative to the working dir so we check this properly
please?

https://codereview.appspot.com/80280043/diff/120001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/80280043/diff/120001/environs/config/config.go#newcode106
environs/config/config.go:106:
yeah, this feels a bit tacked-on here. Let's give it its own package if
we don't think of anywhere better.

https://codereview.appspot.com/80280043/diff/120001/environs/config/config.go#newcode456
environs/config/config.go:456: series := s.(string)
this is guaranteed to be a string if it's got this far, right?

https://codereview.appspot.com/80280043/diff/120001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/80280043/diff/120001/provider/common/bootstrap.go#newcode45
provider/common/bootstrap.go:45: selectedTools, err :=
EnsureBootstrapTools(ctx, env, config.PreferredSeries(env.Config()),
cons.Arch)
quibble quibble, we should probably allow bootstrap on any series we can
find tools for. Out of scope today, but maybe worth a bug?

https://codereview.appspot.com/80280043/diff/120001/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/80280043/diff/120001/provider/ec2/ec2.go#newcode367
provider/ec2/ec2.go:367: Series:
config.PreferredSeries(e.ecfg()),
this seems a bit weird, but so did the original. let it stand.

https://codereview.appspot.com/80280043/diff/120001/provider/ec2/live_test.go
File provider/ec2/live_test.go (right):

https://codereview.appspot.com/80280043/diff/120001/provider/ec2/live_test.go#newcode104
provider/ec2/live_test.go:104: Series: coretesting.FakeDefaultSeries,
wondering if this should be LatestLts? I presume you have a good reason
it isn't, but it's not immediately clear ;)

https://codereview.appspot.com/80280043/diff/120001/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/80280043/diff/120001/state/api/client.go#newcode613
state/api/client.go:613: func (c *Client) ResolveCharms(refs
...charm.Reference) ([]params.ResolveCharmResult, error) {
I think this client-side method might be cleaner as non-bulk -- I don't
really care about bulk calls and compatibility for state/api, just for
state/apiserver.

https://codereview.appspot.com/80280043/diff/120001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/80280043/diff/120001/state/api/params/params.go#newcode335
state/api/params/params.go:335: References []charm.Reference
remind me, does this jsonify to a string?

https://codereview.appspot.com/80280043/diff/120001/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/80280043/diff/120001/state/apiserver/client/client.go#newcode576
state/apiserver/client/client.go:576: prefSeries =
config.PreferredSeries(conf)
is it rational to move all this block...

https://codereview.appspot.com/80280043/diff/120001/state/apiserver/client/client.go#newcode595
state/apiserver/client/client.go:595: p.Series = prefSeries
...into here? and not hit the db for config unless it's really needed?

https://codereview.appspot.com/80280043/diff/120001/state/apiserver/client/client.go#newcode985
state/apiserver/client/client.go:985: return store.Resolve(ref)
I suspect it would be nicer to move this stuff into ResolveCharm to
avoid the repeated db hits

https://codereview.appspot.com/80280043/