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

I've addressed most of the feedback, but there is a problem lurking.
When config.LatestLtsSeries() == "trusty", some of the tests are failing
due to tools availability. I was able to resolve some of them, but not
all. Could really use some advice.

To reproduce the failures, this will simulate a post-trusty release
scenario in the proposed branch:

go test -ldflags "-X
launchpad.net/juju-core/environs/config.latestLtsSeries trusty" ./...

These are the failures:

FAIL: bootstrap_test.go:506:
BootstrapSuite.TestAutoUploadAfterFailedSync
FAIL: bootstrap_test.go:555:
BootstrapSuite.TestMissingToolsUploadFailedError
FAIL: upgradejuju_test.go:301: UpgradeJujuSuite.TestUpgradeJuju
FAIL: imagemetadata_test.go:141:
ImageMetadataSuite.TestImageMetadataFilesDefaultSeries

Everything passes when the latest LTS is precise.

Please advise,
Casey

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

https://codereview.appspot.com/80280043/diff/60001/cmd/juju/addmachine.go#newcode129
cmd/juju/addmachine.go:129: series = conf.PreferredSeries()
On 2014/04/01 07:10:07, fwereade wrote:
> mm, I rather liked the PreferredSeries(conf) spelling, especially if
it were
> using an interface with just the DefaultSeries method. This doesn't
feel
> fundamental to a config -- does that seem sane?

Done.

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

https://codereview.appspot.com/80280043/diff/60001/cmd/juju/common.go#newcode82
cmd/juju/common.go:82: logger.Warningf(`ResolveCharm not supported by
the API server, falling back to default series "precise".`)
On 2014/04/01 07:10:07, fwereade wrote:
> PreferredSeries should surely be guaranteed to return non-""?

Done.

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

https://codereview.appspot.com/80280043/diff/60001/environs/config/config.go#newcode230
environs/config/config.go:230: return DefaultSeries
On 2014/04/01 15:05:12, cmars wrote:
> On 2014/04/01 07:10:07, fwereade wrote:
> > How do we determine the value of this? I'm feeling like it really
ought to
> > actually *be* the latest LTS, rather than just some global var set
by
> > who-knows-who.

> I can try to get this from 'distro-info --lts'. If that fails to exec,
I'll use
> a hard-coded fallback (named as such). What do you think?

Done.

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

https://codereview.appspot.com/80280043/diff/60001/state/api/params/params.go#newcode338
state/api/params/params.go:338: URLs []charm.URL
On 2014/04/01 07:10:07, fwereade wrote:
> I think we need an error per-result here, don't we?

Done.

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

https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/client.go#newcode963
state/apiserver/client/client.go:963: return
params.ResolveCharmResults{}, err
On 2014/04/01 07:10:07, fwereade wrote:
> sorry for the hassle, but we should always return one result per
request, and
> that result should contain either the answer or the error.

> Client is a dog's dinner in this regard, and can only gradually and
> incrementally improve, but the internal APIs are generally written as
I want
> them, and should be used as a model. The core idea is that bulk APIs
can be used
> for single calls, but single APIs can't be used in bulk; as humans we
are bad at
> predicting the future, and I'd rather just do everything bulk-style so
as to
> avoid needless churn.

Done.

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

https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/client_test.go#newcode1980
state/apiserver/client/client_test.go:1980: comment :=
gc.Commentf("defaultSeries:%s charmName:%s", test.defaultSeries,
test.charmName)
On 2014/04/01 07:10:07, fwereade wrote:
> This comment should be pretty much redundant now, with the logging.
Break a test
> and look at the output with/without the comment -- and then use
whichever you
> prefer, but do look at them :).

Done.

https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/client_test.go#newcode2158
state/apiserver/client/client_test.go:2158:
c.Assert(machines[2].Series(), gc.Equals, "non-default")
On 2014/04/01 07:10:07, fwereade wrote:
> would be pretty nice to dupe this test with an empty default-series. I
know it's
> not your code originally, but... :)

I'm not quite clear on where I'd set it.

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