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

LGTM, with some thoughts and suggestions below.
very nice, and thanks in particular for the extra test coverage.

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

https://codereview.appspot.com/10166044/diff/1/cmd/juju/deploy.go#newcode141
cmd/juju/deploy.go:141: return errors.New("cannot specify units for
subordinate service")
s/units/unit count/ ?

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

https://codereview.appspot.com/10166044/diff/1/cmd/juju/deploy_test.go#newcode62
cmd/juju/deploy_test.go:62:
tyvm for the extra tests.

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#newcode214
juju/conn.go:214: // (, minimumUnitCount, initialMachineIds?).
s/, //

https://codereview.appspot.com/10166044/diff/1/juju/conn.go#newcode227
juju/conn.go:227: if args.Constraints != emptyCons {
maybe we should define

var None Value

in the constraints package.

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) {
tbh i think i'd prefer:

store, restore := newMockCharmStore()
defer restore()

and lose the extra indentation.

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

https://codereview.appspot.com/10166044/diff/1/state/apiserver/perm_test.go#newcode290
state/apiserver/perm_test.go:290: withMockCharmStore(func(store
*coretesting.MockCharmStore) {
let's just lose all this and try to deploy an invalid charm.
various examples of that technique below (e.g. opClientAddServiceUnits).

then we could move the mock charm store stuff into the client test
suite.

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 {
doc comment please

https://codereview.appspot.com/10166044/diff/1/testing/charm.go#newcode123
testing/charm.go:123: func (s *MockCharmStore) SetCharm(curl *charm.URL,
bundle *charm.Bundle) error {
ditto

https://codereview.appspot.com/10166044/diff/1/testing/charm.go#newcode144
testing/charm.go:144: func (s *MockCharmStore) interpret(curl
*charm.URL) (base string, rev int) {
ditto (what does "interpret" mean in this context?)

https://codereview.appspot.com/10166044/diff/1/testing/charm.go#newcode156
testing/charm.go:156: func (s *MockCharmStore) Get(curl *charm.URL)
(charm.Charm, error) {
// Get implements charm.Repository.Get.
?

https://codereview.appspot.com/10166044/diff/1/testing/charm.go#newcode165
testing/charm.go:165: func (s *MockCharmStore) Latest(curl *charm.URL)
(int, error) {
// Latest implements charm.Repository.Latest.
?

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