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

A few issues -- don't think any of them should be too much hassle to
resolve, the DefaultSeries return value and the API bulk call thing
should both be pretty mechanical.

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

https://codereview.appspot.com/80280043/diff/20001/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".`)
We should in fact be guaranteed a value given a 1.16 state server, but I
heartily endorse this behaviour all the same.

https://codereview.appspot.com/80280043/diff/20001/environs/config/config.go
File environs/config/config.go (left):

https://codereview.appspot.com/80280043/diff/20001/environs/config/config.go#oldcode411
environs/config/config.go:411: func (c *Config) DefaultSeries() string {
In-band errors squick me out... now that it's possible to have no
default-series, please add a ,ok return value. And I guess return false
if it *is* set, but is set to "".

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

https://codereview.appspot.com/80280043/diff/20001/state/api/client.go#newcode614
state/api/client.go:614: args := params.CharmURL{URL: ref.String()}
mmmmm I don't quite like that... can we make this a separate type?
params.CharmURL now means something different (although I'm surprised, a
little... I could have sworn that charm url fields marshalled to strings
without any hassle at all [0], and so I'd expect us to be using actual
*charm.URLs, and to have to add a new type for a reference anyway.)

[0] yeah, they should do, they have MarshalJSON and UnmarshalJSON.
Wonder why wedon't make use of it...

https://codereview.appspot.com/80280043/diff/20001/state/api/client.go#newcode616
state/api/client.go:616: if err := c.st.Call("Client", "",
"ResolveCharm", args, result); err != nil {
Bulk calls please, they don't have to be exposed in api.Client but it's
reasonable to expect to be able to resolve a few urls in one go, and we
should accommodate that in the interface.

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

https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/client.go#newcode959
state/apiserver/client/client.go:959: ref, series, err :=
charm.ParseReference(args.URL)
yeah, calling it URL is jarring -- new type OK?

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
set this outside loop?

https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/client_test.go#newcode1979
state/apiserver/client/client_test.go:1979: comment :=
gc.Commentf("defaultSeries:%s charmName:%s", t.defaultSeries,
t.charmName)
for i, test := range tests {
     c.Logf("test %d: %#v", i, test)

...is a reasonably compact way to make the test logs somewhat useful.

I do really like calling it "test", not "t", though :).

https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/client_test.go#newcode1984
state/apiserver/client/client_test.go:1984: c.Assert(err, gc.IsNil,
comment) // All of these should parse
can we Check and continue, here and below? check returns a bool (false
on failure iirc?), and while one piece of test data may have a problem I
think it's good to carry on and run the others.

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