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

Nearly there -- a quibble with the location of the PreferredSeries code,
and a bit of work on the API.

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

https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/client_test.go#newcode1977
state/apiserver/client/client_test.go:1977: store.DefaultSeries =
t.defaultSeries
On 2014/03/31 19:57:07, cmars wrote:
> On 2014/03/28 10:43:11, fwereade wrote:
> > set this outside loop?

> Varying the default series in the mock charm store among the test
conditions
> helps ensure the value isn't being hard-coded anywhere in the
apiserver, and
> it's useful for simulating a failure to resolve.

Ofc, thanks. I think my eye slipped over the `t.` -- but it's much
harder to miss the `test.`. Thanks :).

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()
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?

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".`)
PreferredSeries should surely be guaranteed to return non-""?

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
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.

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
I think we need an error per-result here, don't we?

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
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.

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)
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 :).

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")
would be pretty nice to dupe this test with an empty default-series. I
know it's not your code originally, but... :)

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