Code review comment for lp:~fwereade/juju-core/config-7-deploy-sensible-layering

LGTM with the comments of Roger and John.

https://codereview.appspot.com/10166044/diff/1/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/10166044/diff/1/juju/conn.go#newcode227
juju/conn.go:227: if args.Constraints != emptyCons {
On 2013/06/11 13:22:07, rog wrote:
> maybe we should define

> var None Value

> in the constraints package.

+1

https://codereview.appspot.com/10166044/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/10166044/diff/1/juju/conn_test.go#newcode408
juju/conn_test.go:408: // test, because ServiceDeploy demands that a
charm already exists in state,
s/ServiceDeploy/DeployService/

https://codereview.appspot.com/10166044/diff/1/state/apiserver/client_test.go
File state/apiserver/client_test.go (right):

https://codereview.appspot.com/10166044/diff/1/state/apiserver/client_test.go#newcode305
state/apiserver/client_test.go:305: withMockCharmStore(func(store
*coretesting.MockCharmStore) {
On 2013/06/12 08:19:56, jameinel wrote:
> On 2013/06/11 13:22:07, rog wrote:
> > tbh i think i'd prefer:
> >
> > store, restore := newMockCharmStore()
> > defer restore()
> >
> > and lose the extra indentation.

> Given I don't think you'll ever want to have a test call
'withMockCharmStore' 2
> times, I think I agree. We have the pattern:

> defer MakeFakeHome().Restore()

> elsewhere, but I guess you need the return parameter?

store := coretesting.MockCharmStore{}
defer makeMockCharmStore(&store).restore()

https://codereview.appspot.com/10166044/diff/1/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/10166044/diff/1/testing/charm.go#newcode115
testing/charm.go:115: type MockCharmStore struct {
On 2013/06/11 13:22:07, rog wrote:
> doc comment please

+1

https://codereview.appspot.com/10166044/