Merge lp:~makyo/juju-core/upgradecharm3-1171548 into lp:~juju/juju-core/trunk

Proposed by Madison Scott-Clary
Status: Merged
Merged at revision: 1279
Proposed branch: lp:~makyo/juju-core/upgradecharm3-1171548
Merge into: lp:~juju/juju-core/trunk
Diff against target: 192 lines (+131/-0)
5 files modified
state/api/client.go (+10/-0)
state/api/params/params.go (+7/-0)
state/apiserver/client.go (+27/-0)
state/apiserver/client_test.go (+75/-0)
state/apiserver/perm_test.go (+12/-0)
To merge this branch: bzr merge lp:~makyo/juju-core/upgradecharm3-1171548
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+167331@code.launchpad.net

Description of the change

Add UpgradeCharm to the API

This branch adds upgrade charm functionality to the API (branch 3/3 in the process).

https://codereview.appspot.com/10237043/

To post a comment you must log in.
Revision history for this message
Madison Scott-Clary (makyo) wrote :

Reviewers: mp+167331_code.launchpad.net,

Message:
Please take a look.

Description:
Add UpgradeCharm to the API

This branch adds upgrade charm functionality to the API (branch 3/3 in
the process).

https://code.launchpad.net/~makyo/juju-core/upgradecharm3-1171548/+merge/167331

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9975045/

Affected files:
   A [revision details]
   M state/api/client.go
   M state/api/params/params.go
   M state/apiserver/client.go
   M state/apiserver/perm_test.go
   A state/statecmd/upgradecharm.go
   A state/statecmd/upgradecharm_test.go

Revision history for this message
William Reade (fwereade) wrote :

NOT LGTM until we've spoken -- but I might be convinced to go ahead with
this on the condition of followups we agree on live.

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

https://codereview.appspot.com/9975045/diff/1/state/apiserver/client.go#newcode134
state/apiserver/client.go:134: // Set bumpRevision to false when working
with the CharmStore.
We cannot help but work with the charm store here. Looking back in time
a bit, it seems that we failed in reviewing ServiceDeploy: no blame
attaches for continuing along the "approved" path, ofc, but we can't go
on this way.

The issue is simply that there is no local repo on the API server. The
local repo is in practice a pretty broken concept regardless, but that's
by the by; the fundamental problem is that the client API for deploying
and upgrading charms does need to be able to use charms from local
repos. The only way to do this, AFAICT, is to implement a PutCharm(?)
API that causes a bundled charm on the client to be stored in the
environment and in state, and then to explicitly reference that charm in
subsequent deploy and upgrade ops.

I thought I'd kinda handed the details of this problem over to rogpeppe;
rog, do you have anything to add to this? What's the plan with charm
store charms here? It looks as though we're not getting necessarily
using revisioned charm URLs... not sure how this is sane.

https://codereview.appspot.com/9975045/diff/1/state/statecmd/upgradecharm.go
File state/statecmd/upgradecharm.go (right):

https://codereview.appspot.com/9975045/diff/1/state/statecmd/upgradecharm.go#newcode16
state/statecmd/upgradecharm.go:16: sch, err := conn.PutCharm(curl, repo,
bumpRevision)
I'm not sure this should happen on the server side at all; the CLI and
the GUI are each separately in a position to determine, and upload,
*exactly* the desired charm. The bumpRevision logic only makes sense in
the context of a local repo, and AFAIK nobody has yet implemented the
AddCharm(?) API method that's a prerequisite for use of local charms
over the API.

I can see the use case for something *like* this PutCharm method, that
just copies direct from the charm store into environment storage, but
that's more an AddCharm: PutCharm STM like it implies actually uploading
some bytes from the other end of the connection (and that's what it
actually does... right?).

https://codereview.appspot.com/9975045/

Revision history for this message
Madison Scott-Clary (makyo) wrote :

Reviewers: mp+167331_code.launchpad.net,

Message:
Please take a look.

Description:
Add UpgradeCharm to the API

This branch adds upgrade charm functionality to the API (branch 3/3 in
the process).

https://code.launchpad.net/~makyo/juju-core/upgradecharm3-1171548/+merge/167331

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10237043/

Affected files:
   A [revision details]
   M state/api/client.go
   M state/api/params/params.go
   M state/apiserver/client.go
   M state/apiserver/client_test.go
   M state/apiserver/perm_test.go

Revision history for this message
William Reade (fwereade) wrote :

Code looks great, but a couple of tests need to be rather more
unforgiving ;)

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

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go#newcode417
state/apiserver/client_test.go:417: mem4g :=
constraints.MustParse("mem=4G")
Drop the constraints, they're just a distraction here.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go#newcode425
state/apiserver/client_test.go:425: c.Assert(err, IsNil)
AFAICT this is just calling SetCharm with the existing charm in a
slightly obfuscated way. We really ought to actually change the charm,
and check that the change stuck.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go#newcode440
state/apiserver/client_test.go:440: c.Assert(err, IsNil)
Here it's fine to use the same charm, but you should still test that the
force flag was written.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go#newcode456
state/apiserver/client_test.go:456: s.setUpScenario(c)
do we need setUpScenario here? I can accept it's useful for permTest but
in general it's an awfully big fixture... better, I think, to just add a
wordpress service directly. Can't we just do:

s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))

?

https://codereview.appspot.com/10237043/

Revision history for this message
Madison Scott-Clary (makyo) wrote :

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

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go#newcode417
state/apiserver/client_test.go:417: mem4g :=
constraints.MustParse("mem=4G")
On 2013/06/13 08:01:08, fwereade wrote:
> Drop the constraints, they're just a distraction here.

Done.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go#newcode425
state/apiserver/client_test.go:425: c.Assert(err, IsNil)
On 2013/06/13 08:01:08, fwereade wrote:
> AFAICT this is just calling SetCharm with the existing charm in a
slightly
> obfuscated way. We really ought to actually change the charm, and
check that the
> change stuck.

Done.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go#newcode440
state/apiserver/client_test.go:440: c.Assert(err, IsNil)
On 2013/06/13 08:01:08, fwereade wrote:
> Here it's fine to use the same charm, but you should still test that
the force
> flag was written.

Done.

https://codereview.appspot.com/10237043/diff/1/state/apiserver/client_test.go#newcode456
state/apiserver/client_test.go:456: s.setUpScenario(c)
On 2013/06/13 08:01:08, fwereade wrote:
> do we need setUpScenario here? I can accept it's useful for permTest
but in
> general it's an awfully big fixture... better, I think, to just add a
wordpress
> service directly. Can't we just do:

> s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))

> ?

Ah, much cleaner. Done.

https://codereview.appspot.com/10237043/

Revision history for this message
Madison Scott-Clary (makyo) wrote :
Revision history for this message
William Reade (fwereade) wrote :
Revision history for this message
Frank Mueller (themue) wrote :
Revision history for this message
Madison Scott-Clary (makyo) wrote :

*** Submitted:

Add UpgradeCharm to the API

This branch adds upgrade charm functionality to the API (branch 3/3 in
the process).

R=fwereade, mue
CC=
https://codereview.appspot.com/10237043

https://codereview.appspot.com/10237043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/api/client.go'
2--- state/api/client.go 2013-06-11 00:58:34 +0000
3+++ state/api/client.go 2013-06-13 16:48:27 +0000
4@@ -111,6 +111,16 @@
5 return c.st.Call("Client", "", "ServiceDeploy", params, nil)
6 }
7
8+// ServiceSetCharm sets the charm for a given service.
9+func (c *Client) ServiceSetCharm(serviceName string, charmUrl string, force bool) error {
10+ args := params.ServiceSetCharm{
11+ ServiceName: serviceName,
12+ CharmUrl: charmUrl,
13+ Force: force,
14+ }
15+ return c.st.Call("Client", "", "ServiceSetCharm", args, nil)
16+}
17+
18 // AddServiceUnits adds a given number of units to a service.
19 func (c *Client) AddServiceUnits(service string, numUnits int) ([]string, error) {
20 args := params.AddServiceUnits{
21
22=== modified file 'state/api/params/params.go'
23--- state/api/params/params.go 2013-06-11 01:23:05 +0000
24+++ state/api/params/params.go 2013-06-13 16:48:27 +0000
25@@ -97,6 +97,13 @@
26 ForceMachineId string
27 }
28
29+// ServiceSetCharm sets the charm for a given service.
30+type ServiceSetCharm struct {
31+ ServiceName string
32+ CharmUrl string
33+ Force bool
34+}
35+
36 // ServiceExpose holds the parameters for making the ServiceExpose call.
37 type ServiceExpose struct {
38 ServiceName string
39
40=== modified file 'state/apiserver/client.go'
41--- state/apiserver/client.go 2013-06-11 01:23:05 +0000
42+++ state/apiserver/client.go 2013-06-13 16:48:27 +0000
43@@ -144,6 +144,33 @@
44 return err
45 }
46
47+// ServiceSetCharm sets the charm for a given service.
48+func (c *srvClient) ServiceSetCharm(args params.ServiceSetCharm) error {
49+ service, err := c.root.srv.state.Service(args.ServiceName)
50+ if err != nil {
51+ return err
52+ }
53+ curl, err := charm.ParseURL(args.CharmUrl)
54+ if err != nil {
55+ return err
56+ }
57+ if curl.Schema != "cs" {
58+ return fmt.Errorf(`charm url has unsupported schema %q`, curl.Schema)
59+ }
60+ if curl.Revision < 0 {
61+ return fmt.Errorf("charm url must include revision")
62+ }
63+ conn, err := juju.NewConnFromState(c.root.srv.state)
64+ if err != nil {
65+ return err
66+ }
67+ ch, err := conn.PutCharm(curl, CharmStore, false)
68+ if err != nil {
69+ return err
70+ }
71+ return service.SetCharm(ch, args.Force)
72+}
73+
74 // AddServiceUnits adds a given number of units to a service.
75 func (c *srvClient) AddServiceUnits(args params.AddServiceUnits) (params.AddServiceUnitsResults, error) {
76 units, err := statecmd.AddServiceUnits(c.root.srv.state, args)
77
78=== modified file 'state/apiserver/client_test.go'
79--- state/apiserver/client_test.go 2013-06-12 11:25:32 +0000
80+++ state/apiserver/client_test.go 2013-06-13 16:48:27 +0000
81@@ -410,6 +410,81 @@
82 c.Assert(errors.IsNotFoundError(err), Equals, true)
83 }
84
85+func (s *clientSuite) TestClientServiceSetCharm(c *C) {
86+ store, restore := makeMockCharmStore()
87+ defer restore()
88+ curl, _ := addCharm(c, store, "dummy")
89+ err := s.APIState.Client().ServiceDeploy(
90+ curl.String(), "service", 3, "", constraints.Value{},
91+ )
92+ c.Assert(err, IsNil)
93+ addCharm(c, store, "wordpress")
94+ err = s.APIState.Client().ServiceSetCharm(
95+ "service", "cs:precise/wordpress-3", false,
96+ )
97+ c.Assert(err, IsNil)
98+
99+ // Ensure that the charm is not marked as forced.
100+ service, err := s.State.Service("service")
101+ c.Assert(err, IsNil)
102+ charm, force, err := service.Charm()
103+ c.Assert(err, IsNil)
104+ c.Assert(charm.URL().String(), Equals, "cs:precise/wordpress-3")
105+ c.Assert(force, Equals, false)
106+}
107+
108+func (s *clientSuite) TestClientServiceSetCharmForce(c *C) {
109+ store, restore := makeMockCharmStore()
110+ defer restore()
111+ curl, _ := addCharm(c, store, "dummy")
112+ err := s.APIState.Client().ServiceDeploy(
113+ curl.String(), "service", 3, "", constraints.Value{},
114+ )
115+ c.Assert(err, IsNil)
116+ addCharm(c, store, "wordpress")
117+ err = s.APIState.Client().ServiceSetCharm(
118+ "service", "cs:precise/wordpress-3", true,
119+ )
120+ c.Assert(err, IsNil)
121+
122+ // Ensure that the charm is marked as forced.
123+ service, err := s.State.Service("service")
124+ c.Assert(err, IsNil)
125+ charm, force, err := service.Charm()
126+ c.Assert(err, IsNil)
127+ c.Assert(charm.URL().String(), Equals, "cs:precise/wordpress-3")
128+ c.Assert(force, Equals, true)
129+}
130+
131+func (s *clientSuite) TestClientServiceSetCharmInvalidService(c *C) {
132+ _, restore := makeMockCharmStore()
133+ defer restore()
134+ err := s.APIState.Client().ServiceSetCharm(
135+ "badservice", "cs:precise/wordpress-3", true,
136+ )
137+ c.Assert(err, ErrorMatches, `service "badservice" not found`)
138+}
139+
140+func (s *clientSuite) TestClientServiceSetCharmErrors(c *C) {
141+ _, restore := makeMockCharmStore()
142+ defer restore()
143+ s.State.AddService("wordpress", s.AddTestingCharm(c, "wordpress"))
144+ for url, expect := range map[string]string{
145+ // TODO(fwereade,Makyo) make these errors consistent one day.
146+ "wordpress": `charm URL has invalid schema: "wordpress"`,
147+ "cs:wordpress": `charm URL without series: "cs:wordpress"`,
148+ "cs:precise/wordpress": "charm url must include revision",
149+ "cs:precise/wordpress-999999": `cannot get charm: charm not found in mock store: cs:precise/wordpress-999999`,
150+ "local:precise/wordpress-999999": `charm url has unsupported schema "local"`,
151+ } {
152+ c.Logf("test %s", url)
153+ err := s.APIState.Client().ServiceSetCharm(
154+ "wordpress", url, false,
155+ )
156+ c.Check(err, ErrorMatches, expect)
157+ }
158+}
159+
160 func makeMockCharmStore() (store *coretesting.MockCharmStore, restore func()) {
161 mockStore := coretesting.NewMockCharmStore()
162 origStore := apiserver.CharmStore
163
164=== modified file 'state/apiserver/perm_test.go'
165--- state/apiserver/perm_test.go 2013-06-12 11:25:32 +0000
166+++ state/apiserver/perm_test.go 2013-06-13 16:48:27 +0000
167@@ -64,6 +64,10 @@
168 op: opClientServiceDeploy,
169 allow: []string{"user-admin", "user-other"},
170 }, {
171+ about: "Client.ServiceSetCharm",
172+ op: opClientServiceSetCharm,
173+ allow: []string{"user-admin", "user-other"},
174+}, {
175 about: "Client.GetAnnotations",
176 op: opClientGetAnnotations,
177 allow: []string{"user-admin", "user-other"},
178@@ -292,6 +296,14 @@
179 return func() {}, err
180 }
181
182+func opClientServiceSetCharm(c *C, st *api.State, mst *state.State) (func(), error) {
183+ err := st.Client().ServiceSetCharm("nosuch", "local:series/wordpress", false)
184+ if api.ErrCode(err) == api.CodeNotFound {
185+ err = nil
186+ }
187+ return func() {}, err
188+}
189+
190 func opClientAddServiceUnits(c *C, st *api.State, mst *state.State) (func(), error) {
191 _, err := st.Client().AddServiceUnits("nosuch", 1)
192 if api.ErrCode(err) == api.CodeNotFound {

Subscribers

People subscribed via source and target branches