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

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 {
On 2014/03/28 10:43:11, fwereade wrote:
> 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 "".

Done.

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()}
On 2014/03/28 10:43:11, fwereade wrote:
> 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...

Done.

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 {
On 2014/03/28 10:43:11, fwereade wrote:
> 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.

Done.

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)
On 2014/03/28 10:43:11, fwereade wrote:
> yeah, calling it URL is jarring -- new type OK?

Done.

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

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)
On 2014/03/28 10:43:11, fwereade wrote:
> 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 :).

Done.

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
On 2014/03/28 10:43:11, fwereade wrote:
> 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.

Done.

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