deploy refactoring

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

https://code.launchpad.net/~fwereade/juju-core/config-7-deploy-sensible-layering/+merge/168581

Requires: https://code.launchpad.net/~fwereade/juju-core/config-6-state-service-sane-methods/+merge/168580

(do not edit description out of merge proposal)

[fwereade, 2013-06-11 02:14:46]

Please take a look.

[rog, 2013-06-11 13:22:07]

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...
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#n...
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.
?

[jameinel, 2013-06-12 08:19:55]

LGTM overall

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...
state/apiserver/client_test.go:305: withMockCharmStore(func(store
*coretesting.MockCharmStore) {
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?

[mue, 2013-06-12 08:30:01]

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

[fwereade, 2013-06-12 11:27:43]

*** 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...
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#n...
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.