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

*** Submitted:

deploy refactoring

GUI deploy and CLI deploy are different enough that the common statecmd
caused more problems than it solved. Testing is noticeably improved.

juju.Conn.DeployService and juju.Conn.AddUnits are now probably ready to
move to some other place that just requires a state connection (and not
an environment as well); juju.Conn.PutCharm needs some love too, and
thought devoted to how we're going to put local charms over the API.

But, for now, the various bits all happen in the right place (*except*
that
the CLI once again downloads store charms and uploads them itself,
rather
than taking advantage of that functionality on the server side. This can
and will be fixed, but not this CL).

R=rog, jameinel, mue
CC=
https://codereview.appspot.com/10166044

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")
On 2013/06/11 13:22:07, rog wrote:
> s/units/unit count/ ?

Done differently.

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?).
On 2013/06/11 13:22:07, rog wrote:
> s/, //

Done.

https://codereview.appspot.com/10166044/diff/1/juju/conn.go#newcode227
juju/conn.go:227: if args.Constraints != emptyCons {
On 2013/06/12 08:30:01, mue wrote:
> On 2013/06/11 13:22:07, rog wrote:
> > maybe we should define
> >
> > var None Value
> >
> > in the constraints package.

> +1

Yeah, it is a bit annoying, but I recoil at the possibility/likelihood
of someone modifying Empty. Any objections to
constraints.Value.IsEmpty() in a followup?

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,
On 2013/06/12 08:30:01, mue wrote:
> s/ServiceDeploy/DeployService/

Ha, thanks. Done.

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:30:01, mue wrote:
> 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()

Was originally written to support a table-based test that wanted a fresh
CharmStore for each loop iteration, but I guess that's academic in
practice. Hmm. I think a package-internal `store, restore :=
makeMockCharmStore(); defer restore()` makes most sense here: we can
only put it in coretesting if we either parameterize which store to
patch, or just remove the second independent reference (I think we
should remove it, but... focus :)).

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) {
On 2013/06/11 13:22:07, rog wrote:
> 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.

Good thought, done.

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/12 08:30:01, mue wrote:
> On 2013/06/11 13:22:07, rog wrote:
> > doc comment please

> +1

Done.

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 {
On 2013/06/11 13:22:07, rog wrote:
> ditto

Done.

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) {
On 2013/06/11 13:22:07, rog wrote:
> ditto (what does "interpret" mean in this context?)

Done.

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) {
On 2013/06/11 13:22:07, rog wrote:
> // Get implements charm.Repository.Get.
> ?

Done.

https://codereview.appspot.com/10166044/diff/1/testing/charm.go#newcode165
testing/charm.go:165: func (s *MockCharmStore) Latest(curl *charm.URL)
(int, error) {
On 2013/06/11 13:22:07, rog wrote:
> // Latest implements charm.Repository.Latest.
> ?

Done.

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