Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(90)

Issue 10166044: deploy refactoring

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 5 months ago by fwereade
Modified:
8 years, 5 months ago
Reviewers:
mue, mp+168581, jameinel, rog
Visibility:
Public.

Description

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)

Patch Set 1 #

Total comments: 27

Patch Set 2 : deploy refactoring #

Unified diffs Side-by-side diffs Delta from patch set Stats (+607 lines, -542 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/cmd_test.go View 1 1 chunk +2 lines, -2 lines 0 comments Download
M cmd/juju/deploy.go View 1 3 chunks +39 lines, -15 lines 0 comments Download
M cmd/juju/deploy_test.go View 1 3 chunks +149 lines, -1 line 0 comments Download
M juju/conn.go View 1 1 chunk +32 lines, -28 lines 0 comments Download
M juju/conn_test.go View 1 2 chunks +145 lines, -47 lines 0 comments Download
M state/api/params/params.go View 1 chunk +0 lines, -1 line 0 comments Download
M state/apiserver/client.go View 2 chunks +38 lines, -22 lines 0 comments Download
M state/apiserver/client_test.go View 1 2 chunks +124 lines, -68 lines 0 comments Download
M state/apiserver/perm_test.go View 1 2 chunks +4 lines, -22 lines 0 comments Download
D state/statecmd/deploy.go View 1 chunk +0 lines, -63 lines 0 comments Download
D state/statecmd/deploy_test.go View 1 chunk +0 lines, -273 lines 0 comments Download
M testing/charm.go View 1 1 chunk +72 lines, -0 lines 0 comments Download

Messages

Total messages: 5
fwereade
Please take a look.
8 years, 5 months ago (2013-06-11 02:14:46 UTC) #1
rog
LGTM, with some thoughts and suggestions below. very nice, and thanks in particular for the ...
8 years, 5 months ago (2013-06-11 13:22:07 UTC) #2
jameinel
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#newcode305 state/apiserver/client_test.go:305: withMockCharmStore(func(store *coretesting.MockCharmStore) { On 2013/06/11 13:22:07, rog ...
8 years, 5 months ago (2013-06-12 08:19:55 UTC) #3
mue
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 ...
8 years, 5 months ago (2013-06-12 08:30:01 UTC) #4
fwereade
8 years, 5 months ago (2013-06-12 11:27:43 UTC) #5
*** 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b