Merge lp:~fwereade/juju-core/config-7-deploy-sensible-layering into lp:~juju/juju-core/trunk
- config-7-deploy-sensible-layering
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 1277 | ||||
Proposed branch: | lp:~fwereade/juju-core/config-7-deploy-sensible-layering | ||||
Merge into: | lp:~juju/juju-core/trunk | ||||
Prerequisite: | lp:~fwereade/juju-core/config-6-state-service-sane-methods | ||||
Diff against target: |
1318 lines (+590/-530) 11 files modified
cmd/juju/deploy.go (+38/-14) cmd/juju/deploy_test.go (+148/-0) juju/conn.go (+32/-28) juju/conn_test.go (+145/-47) state/api/params/params.go (+0/-1) state/apiserver/client.go (+38/-22) state/apiserver/client_test.go (+124/-67) state/apiserver/perm_test.go (+6/-15) state/statecmd/deploy.go (+0/-63) state/statecmd/deploy_test.go (+0/-273) testing/charm.go (+59/-0) |
||||
To merge this branch: | bzr merge lp:~fwereade/juju-core/config-7-deploy-sensible-layering | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+168581@code.launchpad.net |
Commit message
Description of the change
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.
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).
William Reade (fwereade) wrote : | # |
Roger Peppe (rogpeppe) wrote : | # |
LGTM, with some thoughts and suggestions below.
very nice, and thanks in particular for the extra test coverage.
https:/
File cmd/juju/deploy.go (right):
https:/
cmd/juju/
subordinate service")
s/units/unit count/ ?
https:/
File cmd/juju/
https:/
cmd/juju/
tyvm for the extra tests.
https:/
File juju/conn.go (right):
https:/
juju/conn.go:214: // (, minimumUnitCount, initialMachineI
s/, //
https:/
juju/conn.go:227: if args.Constraints != emptyCons {
maybe we should define
var None Value
in the constraints package.
https:/
File state/apiserver
https:/
state/apiserver
*coretesting.
tbh i think i'd prefer:
store, restore := newMockCharmStore()
defer restore()
and lose the extra indentation.
https:/
File state/apiserver
https:/
state/apiserver
*coretesting.
let's just lose all this and try to deploy an invalid charm.
various examples of that technique below (e.g. opClientAddServ
then we could move the mock charm store stuff into the client test
suite.
https:/
File testing/charm.go (right):
https:/
testing/
doc comment please
https:/
testing/
bundle *charm.Bundle) error {
ditto
https:/
testing/
*charm.URL) (base string, rev int) {
ditto (what does "interpret" mean in this context?)
https:/
testing/
(charm.Charm, error) {
// Get implements charm.Repositor
?
https:/
testing/
(int, error) {
// Latest implements charm.Repositor
?
John A Meinel (jameinel) wrote : | # |
LGTM overall
https:/
File state/apiserver
https:/
state/apiserver
*coretesting.
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
'withMockCharmS
defer MakeFakeHome(
elsewhere, but I guess you need the return parameter?
Frank Mueller (themue) wrote : | # |
LGTM with the comments of Roger and John.
https:/
File juju/conn.go (right):
https:/
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:/
File juju/conn_test.go (right):
https:/
juju/conn_
charm already exists in state,
s/ServiceDeploy
https:/
File state/apiserver
https:/
state/apiserver
*coretesting.
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
'withMockCharmS
> times, I think I agree. We have the pattern:
> defer MakeFakeHome(
> elsewhere, but I guess you need the return parameter?
store := coretesting.
defer makeMockCharmSt
https:/
File testing/charm.go (right):
https:/
testing/
On 2013/06/11 13:22:07, rog wrote:
> doc comment please
+1
William Reade (fwereade) wrote : | # |
*** 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.
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:/
https:/
File cmd/juju/deploy.go (right):
https:/
cmd/juju/
subordinate service")
On 2013/06/11 13:22:07, rog wrote:
> s/units/unit count/ ?
Done differently.
https:/
File juju/conn.go (right):
https:/
juju/conn.go:214: // (, minimumUnitCount, initialMachineI
On 2013/06/11 13:22:07, rog wrote:
> s/, //
Done.
https:/
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/
of someone modifying Empty. Any objections to
constraints.
https:/
File juju/conn_test.go (right):
https:/
juju/conn_
charm already exists in state,
On 2013/06/12 08:30:01, mue wrote:
> s/ServiceDeploy
Ha, thanks. Done.
https:/
File state/apiserver
https:/
state/apiserver
*coretesting.
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
'withMockCharmS
> 2
> > times, I think I agree. We have the pattern:
> >
> > defer MakeFakeHome(
> >
> > elsewhere, but I guess you need the return parameter?
> store := coretesting.
> defer makeMockCharmSt
Was originally written to support a table-based test that wanted a fresh
Charm...
Preview Diff
1 | === modified file 'cmd/juju/deploy.go' |
2 | --- cmd/juju/deploy.go 2013-06-11 02:14:27 +0000 |
3 | +++ cmd/juju/deploy.go 2013-06-11 02:14:27 +0000 |
4 | @@ -6,15 +6,12 @@ |
5 | import ( |
6 | "errors" |
7 | "fmt" |
8 | - "io/ioutil" |
9 | "launchpad.net/gnuflag" |
10 | "launchpad.net/juju-core/charm" |
11 | "launchpad.net/juju-core/cmd" |
12 | "launchpad.net/juju-core/constraints" |
13 | "launchpad.net/juju-core/juju" |
14 | "launchpad.net/juju-core/state" |
15 | - "launchpad.net/juju-core/state/api/params" |
16 | - "launchpad.net/juju-core/state/statecmd" |
17 | "os" |
18 | ) |
19 | |
20 | @@ -123,22 +120,49 @@ |
21 | if err != nil { |
22 | return err |
23 | } |
24 | - var configYAML []byte |
25 | + // TODO(fwereade) it's annoying to roundtrip the bytes through the client |
26 | + // here, but it's the original behaviour and not convenient to change. |
27 | + // PutCharm will always be required in some form for local charms; and we |
28 | + // will need an EnsureStoreCharm method somewhere that gets the state.Charm |
29 | + // for use in the following checks. |
30 | + ch, err := conn.PutCharm(curl, repo, c.BumpRevision) |
31 | + if err != nil { |
32 | + return err |
33 | + } |
34 | + numUnits := c.NumUnits |
35 | + if ch.Meta().Subordinate { |
36 | + empty := constraints.Value{} |
37 | + if c.Constraints != empty { |
38 | + return errors.New("cannot specify constraints for subordinate service") |
39 | + } |
40 | + if numUnits == 1 && c.ForceMachineId == "" { |
41 | + numUnits = 0 |
42 | + } else { |
43 | + return errors.New("cannot specify units for subordinate service") |
44 | + } |
45 | + } |
46 | + serviceName := c.ServiceName |
47 | + if serviceName == "" { |
48 | + serviceName = ch.Meta().Name |
49 | + } |
50 | + var settings charm.Settings |
51 | if c.Config.Path != "" { |
52 | - configYAML, err = ioutil.ReadFile(c.Config.Path) |
53 | + configYAML, err := c.Config.Read(ctx) |
54 | + if err != nil { |
55 | + return err |
56 | + } |
57 | + settings, err = ch.Config().ParseSettingsYAML(configYAML, serviceName) |
58 | if err != nil { |
59 | return err |
60 | } |
61 | } |
62 | - args := params.ServiceDeploy{ |
63 | - ServiceName: c.ServiceName, |
64 | - CharmUrl: c.CharmName, |
65 | - NumUnits: c.NumUnits, |
66 | - // BUG(lp:1162122): --config has no tests. |
67 | - ConfigYAML: string(configYAML), |
68 | + _, err = conn.DeployService(juju.DeployServiceParams{ |
69 | + ServiceName: serviceName, |
70 | + Charm: ch, |
71 | + NumUnits: numUnits, |
72 | + ConfigSettings: settings, |
73 | Constraints: c.Constraints, |
74 | - BumpRevision: c.BumpRevision, |
75 | ForceMachineId: c.ForceMachineId, |
76 | - } |
77 | - return statecmd.ServiceDeploy(conn.State, args, conn, curl, repo) |
78 | + }) |
79 | + return err |
80 | } |
81 | |
82 | === modified file 'cmd/juju/deploy_test.go' |
83 | --- cmd/juju/deploy_test.go 2013-06-11 02:14:27 +0000 |
84 | +++ cmd/juju/deploy_test.go 2013-06-11 02:14:27 +0000 |
85 | @@ -5,7 +5,11 @@ |
86 | |
87 | import ( |
88 | . "launchpad.net/gocheck" |
89 | + "launchpad.net/juju-core/charm" |
90 | + "launchpad.net/juju-core/constraints" |
91 | + "launchpad.net/juju-core/errors" |
92 | "launchpad.net/juju-core/juju/testing" |
93 | + "launchpad.net/juju-core/state" |
94 | coretesting "launchpad.net/juju-core/testing" |
95 | ) |
96 | |
97 | @@ -55,3 +59,147 @@ |
98 | c.Assert(err, ErrorMatches, t.err) |
99 | } |
100 | } |
101 | + |
102 | +func (s *DeploySuite) TestNoCharm(c *C) { |
103 | + err := runDeploy(c, "local:unknown-123") |
104 | + c.Assert(err, ErrorMatches, `cannot get charm: charm not found in ".*": local:precise/unknown-123`) |
105 | +} |
106 | + |
107 | +func (s *DeploySuite) TestCharmDir(c *C) { |
108 | + coretesting.Charms.ClonedDirPath(s.SeriesPath, "dummy") |
109 | + err := runDeploy(c, "local:dummy") |
110 | + c.Assert(err, IsNil) |
111 | + curl := charm.MustParseURL("local:precise/dummy-1") |
112 | + s.AssertService(c, "dummy", curl, 1, 0) |
113 | +} |
114 | + |
115 | +func (s *DeploySuite) TestUpgradeCharmDir(c *C) { |
116 | + dirPath := coretesting.Charms.ClonedDirPath(s.SeriesPath, "dummy") |
117 | + err := runDeploy(c, "local:dummy", "-u") |
118 | + c.Assert(err, IsNil) |
119 | + curl := charm.MustParseURL("local:precise/dummy-2") |
120 | + s.AssertService(c, "dummy", curl, 1, 0) |
121 | + // Check the charm really was upgraded. |
122 | + ch, err := charm.ReadDir(dirPath) |
123 | + c.Assert(err, IsNil) |
124 | + c.Assert(ch.Revision(), Equals, 2) |
125 | +} |
126 | + |
127 | +func (s *DeploySuite) TestCharmBundle(c *C) { |
128 | + coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
129 | + err := runDeploy(c, "local:dummy", "some-service-name") |
130 | + c.Assert(err, IsNil) |
131 | + curl := charm.MustParseURL("local:precise/dummy-1") |
132 | + s.AssertService(c, "some-service-name", curl, 1, 0) |
133 | +} |
134 | + |
135 | +func (s *DeploySuite) TestCannotUpgradeCharmBundle(c *C) { |
136 | + coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
137 | + err := runDeploy(c, "local:dummy", "-u") |
138 | + c.Assert(err, ErrorMatches, `cannot increment revision of charm "local:precise/dummy-1": not a directory`) |
139 | + // Verify state not touched... |
140 | + curl := charm.MustParseURL("local:precise/dummy-1") |
141 | + _, err = s.State.Charm(curl) |
142 | + c.Assert(err, ErrorMatches, `charm "local:precise/dummy-1" not found`) |
143 | + _, err = s.State.Service("dummy") |
144 | + c.Assert(err, ErrorMatches, `service "dummy" not found`) |
145 | +} |
146 | + |
147 | +func (s *DeploySuite) TestSubordinateCharm(c *C) { |
148 | + coretesting.Charms.BundlePath(s.SeriesPath, "logging") |
149 | + err := runDeploy(c, "local:logging") |
150 | + c.Assert(err, IsNil) |
151 | + curl := charm.MustParseURL("local:precise/logging-1") |
152 | + s.AssertService(c, "logging", curl, 0, 0) |
153 | +} |
154 | + |
155 | +func (s *DeploySuite) TestConfig(c *C) { |
156 | + coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
157 | + path := setupConfigfile(c, c.MkDir()) |
158 | + err := runDeploy(c, "local:dummy", "dummy-service", "--config", path) |
159 | + c.Assert(err, IsNil) |
160 | + service, err := s.State.Service("dummy-service") |
161 | + c.Assert(err, IsNil) |
162 | + settings, err := service.ConfigSettings() |
163 | + c.Assert(err, IsNil) |
164 | + c.Assert(settings, DeepEquals, charm.Settings{ |
165 | + "skill-level": int64(9000), |
166 | + "username": "admin001", |
167 | + }) |
168 | +} |
169 | + |
170 | +func (s *DeploySuite) TestConfigError(c *C) { |
171 | + coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
172 | + path := setupConfigfile(c, c.MkDir()) |
173 | + err := runDeploy(c, "local:dummy", "other-service", "--config", path) |
174 | + c.Assert(err, ErrorMatches, `no settings found for "other-service"`) |
175 | + _, err = s.State.Service("other-service") |
176 | + c.Assert(errors.IsNotFoundError(err), Equals, true) |
177 | +} |
178 | + |
179 | +func (s *DeploySuite) TestConstraints(c *C) { |
180 | + coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
181 | + err := runDeploy(c, "local:dummy", "--constraints", "mem=2G cpu-cores=2") |
182 | + c.Assert(err, IsNil) |
183 | + curl := charm.MustParseURL("local:precise/dummy-1") |
184 | + service, _ := s.AssertService(c, "dummy", curl, 1, 0) |
185 | + cons, err := service.Constraints() |
186 | + c.Assert(err, IsNil) |
187 | + c.Assert(cons, DeepEquals, constraints.MustParse("mem=2G cpu-cores=2")) |
188 | +} |
189 | + |
190 | +func (s *DeploySuite) TestSubordinateConstraints(c *C) { |
191 | + coretesting.Charms.BundlePath(s.SeriesPath, "logging") |
192 | + err := runDeploy(c, "local:logging", "--constraints", "mem=1G") |
193 | + c.Assert(err, ErrorMatches, "cannot specify constraints for subordinate service") |
194 | +} |
195 | + |
196 | +func (s *DeploySuite) TestNumUnits(c *C) { |
197 | + coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
198 | + err := runDeploy(c, "local:dummy", "-n", "13") |
199 | + c.Assert(err, IsNil) |
200 | + curl := charm.MustParseURL("local:precise/dummy-1") |
201 | + s.AssertService(c, "dummy", curl, 13, 0) |
202 | +} |
203 | + |
204 | +func (s *DeploySuite) TestNumUnitsSubordinate(c *C) { |
205 | + coretesting.Charms.BundlePath(s.SeriesPath, "logging") |
206 | + err := runDeploy(c, "--num-units", "3", "local:logging") |
207 | + c.Assert(err, ErrorMatches, `cannot specify units for subordinate service`) |
208 | + _, err = s.State.Service("dummy") |
209 | + c.Assert(err, ErrorMatches, `service "dummy" not found`) |
210 | +} |
211 | + |
212 | +func (s *DeploySuite) TestForceMachine(c *C) { |
213 | + coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
214 | + machine, err := s.State.AddMachine("precise", state.JobHostUnits) |
215 | + c.Assert(err, IsNil) |
216 | + err = runDeploy(c, "--force-machine", machine.Id(), "local:dummy", "portlandia") |
217 | + c.Assert(err, IsNil) |
218 | + svc, err := s.State.Service("portlandia") |
219 | + c.Assert(err, IsNil) |
220 | + units, err := svc.AllUnits() |
221 | + c.Assert(err, IsNil) |
222 | + c.Assert(units, HasLen, 1) |
223 | + mid, err := units[0].AssignedMachineId() |
224 | + c.Assert(err, IsNil) |
225 | + c.Assert(mid, Equals, machine.Id()) |
226 | +} |
227 | + |
228 | +func (s *DeploySuite) TestForceMachineNotFound(c *C) { |
229 | + coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
230 | + err := runDeploy(c, "--force-machine", "42", "local:dummy", "portlandia") |
231 | + c.Assert(err, ErrorMatches, `cannot assign unit "portlandia/0" to machine: machine 42 not found`) |
232 | + _, err = s.State.Service("dummy") |
233 | + c.Assert(err, ErrorMatches, `service "dummy" not found`) |
234 | +} |
235 | + |
236 | +func (s *DeploySuite) TestForceMachineSubordinate(c *C) { |
237 | + machine, err := s.State.AddMachine("precise", state.JobHostUnits) |
238 | + c.Assert(err, IsNil) |
239 | + coretesting.Charms.BundlePath(s.SeriesPath, "logging") |
240 | + err = runDeploy(c, "--force-machine", machine.Id(), "local:logging") |
241 | + c.Assert(err, ErrorMatches, `cannot specify units for subordinate service`) |
242 | + _, err = s.State.Service("dummy") |
243 | + c.Assert(err, ErrorMatches, `service "dummy" not found`) |
244 | +} |
245 | |
246 | === modified file 'juju/conn.go' |
247 | --- juju/conn.go 2013-06-11 02:14:27 +0000 |
248 | +++ juju/conn.go 2013-06-11 02:14:27 +0000 |
249 | @@ -186,51 +186,55 @@ |
250 | // DeployServiceParams contains the arguments required to deploy the referenced |
251 | // charm. |
252 | type DeployServiceParams struct { |
253 | - ServiceName string |
254 | - Charm *state.Charm |
255 | - NumUnits int |
256 | - // Config is used only by the API. |
257 | - Config map[string]string |
258 | - // ConfigYAML takes precedence over Config if both are provided. |
259 | - ConfigYAML string |
260 | - Constraints constraints.Value |
261 | + ServiceName string |
262 | + Charm *state.Charm |
263 | + ConfigSettings charm.Settings |
264 | + Constraints constraints.Value |
265 | + NumUnits int |
266 | // Use string for deploy-to machine to avoid ambiguity around machine 0. |
267 | ForceMachineId string |
268 | } |
269 | |
270 | // DeployService takes a charm and various parameters and deploys it. |
271 | func (conn *Conn) DeployService(args DeployServiceParams) (*state.Service, error) { |
272 | - var err error |
273 | - var settings charm.Settings |
274 | - config := args.Charm.Config() |
275 | - if args.ConfigYAML != "" { |
276 | - settings, err = config.ParseSettingsYAML([]byte(args.ConfigYAML), args.ServiceName) |
277 | - } else if args.Config != nil { |
278 | - settings, err = config.ParseSettingsStrings(args.Config) |
279 | - } |
280 | + settings, err := args.Charm.Config().ValidateSettings(args.ConfigSettings) |
281 | if err != nil { |
282 | return nil, err |
283 | } |
284 | - svc, err := conn.State.AddService(args.ServiceName, args.Charm) |
285 | + emptyCons := constraints.Value{} |
286 | + if args.Charm.Meta().Subordinate { |
287 | + if args.NumUnits != 0 || args.ForceMachineId != "" { |
288 | + return nil, fmt.Errorf("subordinate service must be deployed without units") |
289 | + } |
290 | + if args.Constraints != emptyCons { |
291 | + return nil, fmt.Errorf("subordinate service must be deployed without constraints") |
292 | + } |
293 | + } |
294 | + // TODO(fwereade): transactional State.AddService including settings, constraints |
295 | + // (, minimumUnitCount, initialMachineIds?). |
296 | + service, err := conn.State.AddService(args.ServiceName, args.Charm) |
297 | if err != nil { |
298 | return nil, err |
299 | } |
300 | if len(settings) > 0 { |
301 | - if err := svc.UpdateConfigSettings(settings); err != nil { |
302 | + if err := service.UpdateConfigSettings(settings); err != nil { |
303 | return nil, err |
304 | } |
305 | } |
306 | if args.Charm.Meta().Subordinate { |
307 | - return svc, nil |
308 | - } |
309 | - if err := svc.SetConstraints(args.Constraints); err != nil { |
310 | - return nil, err |
311 | - } |
312 | - _, err = conn.AddUnits(svc, args.NumUnits, args.ForceMachineId) |
313 | - if err != nil { |
314 | - return nil, err |
315 | - } |
316 | - return svc, nil |
317 | + return service, nil |
318 | + } |
319 | + if args.Constraints != emptyCons { |
320 | + if err := service.SetConstraints(args.Constraints); err != nil { |
321 | + return nil, err |
322 | + } |
323 | + } |
324 | + if args.NumUnits > 0 { |
325 | + if _, err := conn.AddUnits(service, args.NumUnits, args.ForceMachineId); err != nil { |
326 | + return nil, err |
327 | + } |
328 | + } |
329 | + return service, nil |
330 | } |
331 | |
332 | func (conn *Conn) addCharm(curl *charm.URL, ch charm.Charm) (*state.Charm, error) { |
333 | |
334 | === modified file 'juju/conn_test.go' |
335 | --- juju/conn_test.go 2013-06-11 02:14:27 +0000 |
336 | +++ juju/conn_test.go 2013-06-11 02:14:27 +0000 |
337 | @@ -11,11 +11,13 @@ |
338 | "launchpad.net/juju-core/environs" |
339 | "launchpad.net/juju-core/environs/config" |
340 | "launchpad.net/juju-core/environs/dummy" |
341 | + "launchpad.net/juju-core/errors" |
342 | "launchpad.net/juju-core/juju" |
343 | "launchpad.net/juju-core/juju/testing" |
344 | "launchpad.net/juju-core/state" |
345 | coretesting "launchpad.net/juju-core/testing" |
346 | "launchpad.net/juju-core/utils" |
347 | + "launchpad.net/juju-core/utils/set" |
348 | "os" |
349 | "path/filepath" |
350 | stdtesting "testing" |
351 | @@ -402,63 +404,159 @@ |
352 | |
353 | } |
354 | |
355 | +// DeployLocalSuite uses a fresh copy of the same local dummy charm for each |
356 | +// test, because ServiceDeploy demands that a charm already exists in state, |
357 | +// and that's is the simplest way to get one in there. |
358 | type DeployLocalSuite struct { |
359 | testing.JujuConnSuite |
360 | - repo *charm.LocalRepository |
361 | - defaultSeries string |
362 | - seriesPath string |
363 | - charmUrl *charm.URL |
364 | + repo charm.Repository |
365 | + charm *state.Charm |
366 | + oldCacheDir string |
367 | } |
368 | |
369 | var _ = Suite(&DeployLocalSuite{}) |
370 | |
371 | +func (s *DeployLocalSuite) SetUpSuite(c *C) { |
372 | + s.JujuConnSuite.SetUpSuite(c) |
373 | + s.repo = &charm.LocalRepository{Path: coretesting.Charms.Path} |
374 | + s.oldCacheDir, charm.CacheDir = charm.CacheDir, c.MkDir() |
375 | +} |
376 | + |
377 | +func (s *DeployLocalSuite) TearDownSuite(c *C) { |
378 | + charm.CacheDir = s.oldCacheDir |
379 | + s.JujuConnSuite.TearDownSuite(c) |
380 | +} |
381 | + |
382 | func (s *DeployLocalSuite) SetUpTest(c *C) { |
383 | s.JujuConnSuite.SetUpTest(c) |
384 | - repoPath := c.MkDir() |
385 | - s.defaultSeries = "precise" |
386 | - s.repo = &charm.LocalRepository{Path: repoPath} |
387 | - s.seriesPath = filepath.Join(repoPath, s.defaultSeries) |
388 | - err := os.Mkdir(s.seriesPath, 0777) |
389 | - c.Assert(err, IsNil) |
390 | - coretesting.Charms.BundlePath(s.seriesPath, "wordpress") |
391 | - s.charmUrl, err = charm.InferURL("local:wordpress", s.defaultSeries) |
392 | - c.Assert(err, IsNil) |
393 | -} |
394 | - |
395 | -func (s *DeployLocalSuite) TestDeploy(c *C) { |
396 | - ch, err := s.Conn.PutCharm(s.charmUrl, s.repo, false) |
397 | - c.Assert(err, IsNil) |
398 | - cons := constraints.MustParse("mem=4G") |
399 | - args := juju.DeployServiceParams{ |
400 | - ServiceName: "bob", |
401 | - Charm: ch, |
402 | - NumUnits: 3, |
403 | - Constraints: cons, |
404 | - ConfigYAML: "bob: {blog-title: aspidistra flagpole}", |
405 | - } |
406 | - svc, err := s.Conn.DeployService(args) |
407 | - c.Assert(err, IsNil) |
408 | - scons, err := svc.Constraints() |
409 | - c.Assert(err, IsNil) |
410 | - c.Assert(scons, DeepEquals, cons) |
411 | - settings, err := svc.ConfigSettings() |
412 | - c.Assert(err, IsNil) |
413 | - c.Assert(settings, DeepEquals, charm.Settings{ |
414 | - "blog-title": "aspidistra flagpole", |
415 | - }) |
416 | - |
417 | - units, err := svc.AllUnits() |
418 | - c.Assert(err, IsNil) |
419 | - c.Assert(len(units), Equals, 3) |
420 | + curl := charm.MustParseURL("local:series/dummy") |
421 | + charm, err := s.Conn.PutCharm(curl, s.repo, false) |
422 | + c.Assert(err, IsNil) |
423 | + s.charm = charm |
424 | +} |
425 | + |
426 | +func (s *DeployLocalSuite) TestDeployMinimal(c *C) { |
427 | + service, err := s.Conn.DeployService(juju.DeployServiceParams{ |
428 | + ServiceName: "bob", |
429 | + Charm: s.charm, |
430 | + }) |
431 | + c.Assert(err, IsNil) |
432 | + s.assertCharm(c, service, s.charm.URL()) |
433 | + s.assertSettings(c, service, charm.Settings{}) |
434 | + s.assertConstraints(c, service, constraints.Value{}) |
435 | + s.assertMachines(c, service, constraints.Value{}) |
436 | +} |
437 | + |
438 | +func (s *DeployLocalSuite) TestDeploySettings(c *C) { |
439 | + service, err := s.Conn.DeployService(juju.DeployServiceParams{ |
440 | + ServiceName: "bob", |
441 | + Charm: s.charm, |
442 | + ConfigSettings: charm.Settings{ |
443 | + "title": "banana cupcakes", |
444 | + "skill-level": 9901, |
445 | + }, |
446 | + }) |
447 | + c.Assert(err, IsNil) |
448 | + s.assertSettings(c, service, charm.Settings{ |
449 | + "title": "banana cupcakes", |
450 | + "skill-level": int64(9901), |
451 | + }) |
452 | +} |
453 | + |
454 | +func (s *DeployLocalSuite) TestDeploySettingsError(c *C) { |
455 | + _, err := s.Conn.DeployService(juju.DeployServiceParams{ |
456 | + ServiceName: "bob", |
457 | + Charm: s.charm, |
458 | + ConfigSettings: charm.Settings{ |
459 | + "skill-level": 99.01, |
460 | + }, |
461 | + }) |
462 | + c.Assert(err, ErrorMatches, `option "skill-level" expected int, got 99.01`) |
463 | + _, err = s.State.Service("bob") |
464 | + c.Assert(errors.IsNotFoundError(err), Equals, true) |
465 | +} |
466 | + |
467 | +func (s *DeployLocalSuite) TestDeployConstraints(c *C) { |
468 | + err := s.State.SetEnvironConstraints(constraints.MustParse("mem=2G")) |
469 | + c.Assert(err, IsNil) |
470 | + serviceCons := constraints.MustParse("cpu-cores=2") |
471 | + service, err := s.Conn.DeployService(juju.DeployServiceParams{ |
472 | + ServiceName: "bob", |
473 | + Charm: s.charm, |
474 | + Constraints: serviceCons, |
475 | + }) |
476 | + c.Assert(err, IsNil) |
477 | + s.assertConstraints(c, service, serviceCons) |
478 | +} |
479 | + |
480 | +func (s *DeployLocalSuite) TestDeployNumUnits(c *C) { |
481 | + err := s.State.SetEnvironConstraints(constraints.MustParse("mem=2G")) |
482 | + c.Assert(err, IsNil) |
483 | + serviceCons := constraints.MustParse("cpu-cores=2") |
484 | + service, err := s.Conn.DeployService(juju.DeployServiceParams{ |
485 | + ServiceName: "bob", |
486 | + Charm: s.charm, |
487 | + Constraints: serviceCons, |
488 | + NumUnits: 2, |
489 | + }) |
490 | + c.Assert(err, IsNil) |
491 | + s.assertConstraints(c, service, serviceCons) |
492 | + s.assertMachines(c, service, constraints.MustParse("mem=2G cpu-cores=2"), "0", "1") |
493 | +} |
494 | + |
495 | +func (s *DeployLocalSuite) TestDeployForceMachineId(c *C) { |
496 | + machine, err := s.State.AddMachine("series", state.JobHostUnits) |
497 | + c.Assert(err, IsNil) |
498 | + c.Assert(machine.Id(), Equals, "0") |
499 | + err = s.State.SetEnvironConstraints(constraints.MustParse("mem=2G")) |
500 | + c.Assert(err, IsNil) |
501 | + serviceCons := constraints.MustParse("cpu-cores=2") |
502 | + service, err := s.Conn.DeployService(juju.DeployServiceParams{ |
503 | + ServiceName: "bob", |
504 | + Charm: s.charm, |
505 | + Constraints: serviceCons, |
506 | + NumUnits: 1, |
507 | + ForceMachineId: "0", |
508 | + }) |
509 | + c.Assert(err, IsNil) |
510 | + s.assertConstraints(c, service, serviceCons) |
511 | + s.assertMachines(c, service, constraints.Value{}, "0") |
512 | +} |
513 | + |
514 | +func (s *DeployLocalSuite) assertCharm(c *C, service *state.Service, expect *charm.URL) { |
515 | + curl, force := service.CharmURL() |
516 | + c.Assert(curl, DeepEquals, expect) |
517 | + c.Assert(force, Equals, false) |
518 | +} |
519 | + |
520 | +func (s *DeployLocalSuite) assertSettings(c *C, service *state.Service, expect charm.Settings) { |
521 | + settings, err := service.ConfigSettings() |
522 | + c.Assert(err, IsNil) |
523 | + c.Assert(settings, DeepEquals, expect) |
524 | +} |
525 | + |
526 | +func (s *DeployLocalSuite) assertConstraints(c *C, service *state.Service, expect constraints.Value) { |
527 | + cons, err := service.Constraints() |
528 | + c.Assert(err, IsNil) |
529 | + c.Assert(cons, DeepEquals, expect) |
530 | +} |
531 | + |
532 | +func (s *DeployLocalSuite) assertMachines(c *C, service *state.Service, expectCons constraints.Value, expectIds ...string) { |
533 | + units, err := service.AllUnits() |
534 | + c.Assert(err, IsNil) |
535 | + c.Assert(units, HasLen, len(expectIds)) |
536 | + unseenIds := set.NewStrings(expectIds...) |
537 | for _, unit := range units { |
538 | - mid, err := unit.AssignedMachineId() |
539 | - c.Assert(err, IsNil) |
540 | - machine, err := s.State.Machine(mid) |
541 | - c.Assert(err, IsNil) |
542 | - mcons, err := machine.Constraints() |
543 | - c.Assert(err, IsNil) |
544 | - c.Assert(mcons, DeepEquals, cons) |
545 | + id, err := unit.AssignedMachineId() |
546 | + c.Assert(err, IsNil) |
547 | + unseenIds.Remove(id) |
548 | + machine, err := s.State.Machine(id) |
549 | + c.Assert(err, IsNil) |
550 | + cons, err := machine.Constraints() |
551 | + c.Assert(err, IsNil) |
552 | + c.Assert(cons, DeepEquals, expectCons) |
553 | } |
554 | + c.Assert(unseenIds, DeepEquals, set.NewStrings()) |
555 | } |
556 | |
557 | type InitJujuHomeSuite struct { |
558 | |
559 | === modified file 'state/api/params/params.go' |
560 | --- state/api/params/params.go 2013-06-04 13:00:57 +0000 |
561 | +++ state/api/params/params.go 2013-06-11 02:14:27 +0000 |
562 | @@ -94,7 +94,6 @@ |
563 | Config map[string]string |
564 | ConfigYAML string // Takes precedence over config if both are present. |
565 | Constraints constraints.Value |
566 | - BumpRevision bool |
567 | ForceMachineId string |
568 | } |
569 | |
570 | |
571 | === modified file 'state/apiserver/client.go' |
572 | --- state/apiserver/client.go 2013-06-11 02:14:27 +0000 |
573 | +++ state/apiserver/client.go 2013-06-11 02:14:27 +0000 |
574 | @@ -4,6 +4,7 @@ |
575 | package apiserver |
576 | |
577 | import ( |
578 | + "fmt" |
579 | "launchpad.net/juju-core/charm" |
580 | "launchpad.net/juju-core/juju" |
581 | "launchpad.net/juju-core/state/api" |
582 | @@ -102,30 +103,45 @@ |
583 | |
584 | var CharmStore charm.Repository = charm.Store |
585 | |
586 | -// ServiceDeploy fetches the charm from the charm store and deploys it. Local |
587 | +// ServiceDeploy fetches the charm from the charm store and deploys it. Local |
588 | // charms are not supported. |
589 | func (c *srvClient) ServiceDeploy(args params.ServiceDeploy) error { |
590 | - state := c.root.srv.state |
591 | - conf, err := state.EnvironConfig() |
592 | - if err != nil { |
593 | - return err |
594 | - } |
595 | - curl, err := charm.InferURL(args.CharmUrl, conf.DefaultSeries()) |
596 | - if err != nil { |
597 | - return err |
598 | - } |
599 | - conn, err := juju.NewConnFromState(state) |
600 | - if err != nil { |
601 | - return err |
602 | - } |
603 | - if args.NumUnits == 0 { |
604 | - args.NumUnits = 1 |
605 | - } |
606 | - serviceName := args.ServiceName |
607 | - if serviceName == "" { |
608 | - serviceName = curl.Name |
609 | - } |
610 | - return statecmd.ServiceDeploy(state, args, conn, curl, CharmStore) |
611 | + curl, err := charm.ParseURL(args.CharmUrl) |
612 | + if err != nil { |
613 | + return err |
614 | + } |
615 | + if curl.Schema != "cs" { |
616 | + return fmt.Errorf(`charm url has unsupported schema %q`, curl.Schema) |
617 | + } |
618 | + if curl.Revision < 0 { |
619 | + return fmt.Errorf("charm url must include revision") |
620 | + } |
621 | + conn, err := juju.NewConnFromState(c.root.srv.state) |
622 | + if err != nil { |
623 | + return err |
624 | + } |
625 | + ch, err := conn.PutCharm(curl, CharmStore, false) |
626 | + if err != nil { |
627 | + return err |
628 | + } |
629 | + var settings charm.Settings |
630 | + if len(args.ConfigYAML) > 0 { |
631 | + settings, err = ch.Config().ParseSettingsYAML([]byte(args.ConfigYAML), args.ServiceName) |
632 | + } else if len(args.Config) > 0 { |
633 | + settings, err = ch.Config().ParseSettingsStrings(args.Config) |
634 | + } |
635 | + if err != nil { |
636 | + return err |
637 | + } |
638 | + _, err = conn.DeployService(juju.DeployServiceParams{ |
639 | + ServiceName: args.ServiceName, |
640 | + Charm: ch, |
641 | + NumUnits: args.NumUnits, |
642 | + ConfigSettings: settings, |
643 | + Constraints: args.Constraints, |
644 | + ForceMachineId: args.ForceMachineId, |
645 | + }) |
646 | + return err |
647 | } |
648 | |
649 | // AddServiceUnits adds a given number of units to a service. |
650 | |
651 | === modified file 'state/apiserver/client_test.go' |
652 | --- state/apiserver/client_test.go 2013-06-11 02:14:27 +0000 |
653 | +++ state/apiserver/client_test.go 2013-06-11 02:14:27 +0000 |
654 | @@ -4,6 +4,7 @@ |
655 | package apiserver_test |
656 | |
657 | import ( |
658 | + "fmt" |
659 | . "launchpad.net/gocheck" |
660 | "launchpad.net/juju-core/charm" |
661 | "launchpad.net/juju-core/constraints" |
662 | @@ -300,74 +301,130 @@ |
663 | c.Assert(mode, Equals, state.ResolvedNoHooks) |
664 | } |
665 | |
666 | -var serviceDeployTests = []struct { |
667 | - about string |
668 | - charmUrl string |
669 | - numUnits int |
670 | - expectedNumUnits int |
671 | - constraints constraints.Value |
672 | -}{{ |
673 | - about: "Normal deploy", |
674 | - charmUrl: "local:series/wordpress", |
675 | - expectedNumUnits: 1, |
676 | - constraints: constraints.MustParse("mem=1G"), |
677 | -}, { |
678 | - about: "Two units", |
679 | - charmUrl: "local:series/wordpress", |
680 | - numUnits: 2, |
681 | - expectedNumUnits: 2, |
682 | - constraints: constraints.MustParse("mem=4G"), |
683 | -}, |
684 | -} |
685 | - |
686 | -func (s *clientSuite) TestClientServiceDeploy(c *C) { |
687 | - s.setUpScenario(c) |
688 | - |
689 | - for i, test := range serviceDeployTests { |
690 | - c.Logf("test %d; %s", i, test.about) |
691 | - parsedUrl := charm.MustParseURL(test.charmUrl) |
692 | - localRepo, err := charm.InferRepository(parsedUrl, coretesting.Charms.Path) |
693 | - c.Assert(err, IsNil) |
694 | - withRepo(localRepo, func() { |
695 | - serviceName := "mywordpress" |
696 | - _, err = s.State.Service(serviceName) |
697 | +func (s *clientSuite) TestClientServiceDeployCharmErrors(c *C) { |
698 | + withMockCharmStore(func(store *coretesting.MockCharmStore) { |
699 | + for url, expect := range map[string]string{ |
700 | + // TODO(fwereade) make these errors consistent one day. |
701 | + "wordpress": `charm URL has invalid schema: "wordpress"`, |
702 | + "cs:wordpress": `charm URL without series: "cs:wordpress"`, |
703 | + "cs:precise/wordpress": "charm url must include revision", |
704 | + "cs:precise/wordpress-999999": `cannot get charm: charm not found in mock store: cs:precise/wordpress-999999`, |
705 | + "local:precise/wordpress-999999": `charm url has unsupported schema "local"`, |
706 | + } { |
707 | + c.Logf("test %s", url) |
708 | + err := s.APIState.Client().ServiceDeploy( |
709 | + url, "service", 1, "", constraints.Value{}, |
710 | + ) |
711 | + c.Check(err, ErrorMatches, expect) |
712 | + _, err = s.State.Service("service") |
713 | c.Assert(errors.IsNotFoundError(err), Equals, true) |
714 | - err = s.APIState.Client().ServiceDeploy( |
715 | - test.charmUrl, serviceName, test.numUnits, "mywordpress: {}", test.constraints, |
716 | - ) |
717 | - c.Assert(err, IsNil) |
718 | - |
719 | - service, err := s.State.Service(serviceName) |
720 | - c.Assert(err, IsNil) |
721 | - defer removeServiceAndUnits(c, service) |
722 | - scons, err := service.Constraints() |
723 | - c.Assert(err, IsNil) |
724 | - c.Assert(scons, DeepEquals, test.constraints) |
725 | - |
726 | - units, err := service.AllUnits() |
727 | - c.Assert(err, IsNil) |
728 | - c.Assert(units, HasLen, test.expectedNumUnits) |
729 | - for _, unit := range units { |
730 | - mid, err := unit.AssignedMachineId() |
731 | - c.Assert(err, IsNil) |
732 | - machine, err := s.State.Machine(mid) |
733 | - c.Assert(err, IsNil) |
734 | - mcons, err := machine.Constraints() |
735 | - c.Assert(err, IsNil) |
736 | - c.Assert(mcons, DeepEquals, test.constraints) |
737 | - } |
738 | - }) |
739 | - } |
740 | -} |
741 | - |
742 | -func withRepo(repo charm.Repository, f func()) { |
743 | - // Monkey-patch server repository. |
744 | - originalServerCharmStore := apiserver.CharmStore |
745 | - apiserver.CharmStore = repo |
746 | - defer func() { |
747 | - apiserver.CharmStore = originalServerCharmStore |
748 | - }() |
749 | - f() |
750 | + } |
751 | + }) |
752 | +} |
753 | + |
754 | +func (s *clientSuite) TestClientServiceDeployPrincipal(c *C) { |
755 | + // TODO(fwereade): test ForceMachineId directly on srvClient, when we |
756 | + // manage to extract it as a package and can thus do it conveniently. |
757 | + withMockCharmStore(func(store *coretesting.MockCharmStore) { |
758 | + curl, bundle := addCharm(c, store, "dummy") |
759 | + mem4g := constraints.MustParse("mem=4G") |
760 | + err := s.APIState.Client().ServiceDeploy( |
761 | + curl.String(), "service", 3, "", mem4g, |
762 | + ) |
763 | + c.Assert(err, IsNil) |
764 | + service, err := s.State.Service("service") |
765 | + c.Assert(err, IsNil) |
766 | + charm, force, err := service.Charm() |
767 | + c.Assert(err, IsNil) |
768 | + c.Assert(force, Equals, false) |
769 | + c.Assert(charm.URL(), DeepEquals, curl) |
770 | + c.Assert(charm.Meta(), DeepEquals, bundle.Meta()) |
771 | + c.Assert(charm.Config(), DeepEquals, bundle.Config()) |
772 | + |
773 | + cons, err := service.Constraints() |
774 | + c.Assert(err, IsNil) |
775 | + c.Assert(cons, DeepEquals, mem4g) |
776 | + units, err := service.AllUnits() |
777 | + c.Assert(err, IsNil) |
778 | + for _, unit := range units { |
779 | + mid, err := unit.AssignedMachineId() |
780 | + c.Assert(err, IsNil) |
781 | + machine, err := s.State.Machine(mid) |
782 | + c.Assert(err, IsNil) |
783 | + cons, err := machine.Constraints() |
784 | + c.Assert(err, IsNil) |
785 | + c.Assert(cons, DeepEquals, mem4g) |
786 | + } |
787 | + }) |
788 | +} |
789 | + |
790 | +func (s *clientSuite) TestClientServiceDeploySubordinate(c *C) { |
791 | + withMockCharmStore(func(store *coretesting.MockCharmStore) { |
792 | + curl, bundle := addCharm(c, store, "logging") |
793 | + err := s.APIState.Client().ServiceDeploy( |
794 | + curl.String(), "service-name", 0, "", constraints.Value{}, |
795 | + ) |
796 | + service, err := s.State.Service("service-name") |
797 | + c.Assert(err, IsNil) |
798 | + charm, force, err := service.Charm() |
799 | + c.Assert(err, IsNil) |
800 | + c.Assert(force, Equals, false) |
801 | + c.Assert(charm.URL(), DeepEquals, curl) |
802 | + c.Assert(charm.Meta(), DeepEquals, bundle.Meta()) |
803 | + c.Assert(charm.Config(), DeepEquals, bundle.Config()) |
804 | + |
805 | + units, err := service.AllUnits() |
806 | + c.Assert(err, IsNil) |
807 | + c.Assert(units, HasLen, 0) |
808 | + }) |
809 | +} |
810 | + |
811 | +func (s *clientSuite) TestClientServiceDeployConfig(c *C) { |
812 | + // TODO(fwereade): test Config/ConfigYAML handling directly on srvClient. |
813 | + // Can't be done cleanly until it's extracted similarly to Machiner. |
814 | + withMockCharmStore(func(store *coretesting.MockCharmStore) { |
815 | + curl, _ := addCharm(c, store, "dummy") |
816 | + err := s.APIState.Client().ServiceDeploy( |
817 | + curl.String(), "service-name", 1, "service-name:\n username: fred", constraints.Value{}, |
818 | + ) |
819 | + c.Assert(err, IsNil) |
820 | + service, err := s.State.Service("service-name") |
821 | + c.Assert(err, IsNil) |
822 | + settings, err := service.ConfigSettings() |
823 | + c.Assert(err, IsNil) |
824 | + c.Assert(settings, DeepEquals, charm.Settings{"username": "fred"}) |
825 | + }) |
826 | +} |
827 | + |
828 | +func (s *clientSuite) TestClientServiceDeployConfigError(c *C) { |
829 | + // TODO(fwereade): test Config/ConfigYAML handling directly on srvClient. |
830 | + // Can't be done cleanly until it's extracted similarly to Machiner. |
831 | + withMockCharmStore(func(store *coretesting.MockCharmStore) { |
832 | + curl, _ := addCharm(c, store, "dummy") |
833 | + err := s.APIState.Client().ServiceDeploy( |
834 | + curl.String(), "service-name", 1, "service-name:\n skill-level: fred", constraints.Value{}, |
835 | + ) |
836 | + c.Assert(err, ErrorMatches, `option "skill-level" expected int, got "fred"`) |
837 | + _, err = s.State.Service("service-name") |
838 | + c.Assert(errors.IsNotFoundError(err), Equals, true) |
839 | + }) |
840 | +} |
841 | + |
842 | +func withMockCharmStore(f func(*coretesting.MockCharmStore)) { |
843 | + mockStore := coretesting.NewMockCharmStore() |
844 | + origStore := apiserver.CharmStore |
845 | + apiserver.CharmStore = mockStore |
846 | + defer func() { apiserver.CharmStore = origStore }() |
847 | + f(mockStore) |
848 | +} |
849 | + |
850 | +func addCharm(c *C, store *coretesting.MockCharmStore, name string) (*charm.URL, charm.Charm) { |
851 | + bundle := coretesting.Charms.Bundle(c.MkDir(), name) |
852 | + scurl := fmt.Sprintf("cs:precise/%s-%d", name, bundle.Revision()) |
853 | + curl := charm.MustParseURL(scurl) |
854 | + err := store.SetCharm(curl, bundle) |
855 | + c.Assert(err, IsNil) |
856 | + return curl, bundle |
857 | } |
858 | |
859 | func (s *clientSuite) TestSuccessfulAddRelation(c *C) { |
860 | |
861 | === modified file 'state/apiserver/perm_test.go' |
862 | --- state/apiserver/perm_test.go 2013-06-11 02:14:27 +0000 |
863 | +++ state/apiserver/perm_test.go 2013-06-11 02:14:27 +0000 |
864 | @@ -5,11 +5,9 @@ |
865 | |
866 | import ( |
867 | . "launchpad.net/gocheck" |
868 | - "launchpad.net/juju-core/charm" |
869 | "launchpad.net/juju-core/constraints" |
870 | "launchpad.net/juju-core/state" |
871 | "launchpad.net/juju-core/state/api" |
872 | - "launchpad.net/juju-core/state/apiserver" |
873 | coretesting "launchpad.net/juju-core/testing" |
874 | "strings" |
875 | ) |
876 | @@ -288,23 +286,16 @@ |
877 | } |
878 | |
879 | func opClientServiceDeploy(c *C, st *api.State, mst *state.State) (func(), error) { |
880 | - // We are cheating and using a local repo only. |
881 | - |
882 | - // Set the CharmStore to the test repository. |
883 | - serviceName := "mywordpress" |
884 | - charmUrl := "local:series/wordpress" |
885 | - parsedUrl := charm.MustParseURL(charmUrl) |
886 | - repo, err := charm.InferRepository(parsedUrl, coretesting.Charms.Path) |
887 | - originalServerCharmStore := apiserver.CharmStore |
888 | - apiserver.CharmStore = repo |
889 | - |
890 | - err = st.Client().ServiceDeploy(charmUrl, serviceName, 1, "mywordpress: {}", constraints.Value{}) |
891 | + var err error |
892 | + withMockCharmStore(func(store *coretesting.MockCharmStore) { |
893 | + curl, _ := addCharm(c, store, "dummy") |
894 | + err = st.Client().ServiceDeploy(curl.String(), "deploy-service", 1, "", constraints.Value{}) |
895 | + }) |
896 | if err != nil { |
897 | return func() {}, err |
898 | } |
899 | return func() { |
900 | - apiserver.CharmStore = originalServerCharmStore |
901 | - service, err := mst.Service(serviceName) |
902 | + service, err := mst.Service("deploy-service") |
903 | c.Assert(err, IsNil) |
904 | removeServiceAndUnits(c, service) |
905 | }, nil |
906 | |
907 | === removed file 'state/statecmd/deploy.go' |
908 | --- state/statecmd/deploy.go 2013-05-31 00:13:28 +0000 |
909 | +++ state/statecmd/deploy.go 1970-01-01 00:00:00 +0000 |
910 | @@ -1,63 +0,0 @@ |
911 | -package statecmd |
912 | - |
913 | -import ( |
914 | - "errors" |
915 | - "fmt" |
916 | - "launchpad.net/juju-core/charm" |
917 | - "launchpad.net/juju-core/constraints" |
918 | - "launchpad.net/juju-core/juju" |
919 | - "launchpad.net/juju-core/state" |
920 | - "launchpad.net/juju-core/state/api/params" |
921 | -) |
922 | - |
923 | -// ServiceDeploy deploys a service to the environment from the given repository. |
924 | -// The connection, charm URL, and repository will be provided by the caller, |
925 | -// since these values already exist in the calling locations and can be |
926 | -// defaulted in different scenarios (i.e.: only the charmstore will be used |
927 | -// when called from the websocket API). |
928 | -func ServiceDeploy(st *state.State, args params.ServiceDeploy, conn *juju.Conn, curl *charm.URL, repo charm.Repository) error { |
929 | - if args.ServiceName != "" && !state.IsServiceName(args.ServiceName) { |
930 | - return fmt.Errorf("invalid service name %q", args.ServiceName) |
931 | - } |
932 | - if args.ForceMachineId != "" { |
933 | - if !state.IsMachineId(args.ForceMachineId) { |
934 | - return fmt.Errorf("invalid machine id %q", args.ForceMachineId) |
935 | - } |
936 | - if args.NumUnits > 1 { |
937 | - return fmt.Errorf("force-machine cannot be used for multiple units") |
938 | - } |
939 | - } |
940 | - |
941 | - charm, err := conn.PutCharm(curl, repo, args.BumpRevision) |
942 | - if err != nil { |
943 | - return err |
944 | - } |
945 | - if charm.Meta().Subordinate { |
946 | - empty := constraints.Value{} |
947 | - if args.Constraints != empty { |
948 | - return state.ErrSubordinateConstraints |
949 | - } |
950 | - if args.ForceMachineId != "" { |
951 | - return fmt.Errorf("subordinate service cannot specify force-machine") |
952 | - } |
953 | - } else { |
954 | - if args.NumUnits < 1 { |
955 | - return errors.New("must deploy at least one unit") |
956 | - } |
957 | - } |
958 | - serviceName := args.ServiceName |
959 | - if serviceName == "" { |
960 | - serviceName = curl.Name |
961 | - } |
962 | - deployArgs := juju.DeployServiceParams{ |
963 | - Charm: charm, |
964 | - ServiceName: serviceName, |
965 | - NumUnits: args.NumUnits, |
966 | - Config: args.Config, |
967 | - ConfigYAML: args.ConfigYAML, |
968 | - Constraints: args.Constraints, |
969 | - ForceMachineId: args.ForceMachineId, |
970 | - } |
971 | - _, err = conn.DeployService(deployArgs) |
972 | - return err |
973 | -} |
974 | |
975 | === removed file 'state/statecmd/deploy_test.go' |
976 | --- state/statecmd/deploy_test.go 2013-06-11 02:14:27 +0000 |
977 | +++ state/statecmd/deploy_test.go 1970-01-01 00:00:00 +0000 |
978 | @@ -1,273 +0,0 @@ |
979 | -package statecmd_test |
980 | - |
981 | -import ( |
982 | - . "launchpad.net/gocheck" |
983 | - "launchpad.net/juju-core/charm" |
984 | - "launchpad.net/juju-core/constraints" |
985 | - "launchpad.net/juju-core/juju/testing" |
986 | - "launchpad.net/juju-core/state" |
987 | - "launchpad.net/juju-core/state/api/params" |
988 | - "launchpad.net/juju-core/state/statecmd" |
989 | - coretesting "launchpad.net/juju-core/testing" |
990 | - "os" |
991 | -) |
992 | - |
993 | -type DeploySuite struct { |
994 | - testing.RepoSuite |
995 | -} |
996 | - |
997 | -var _ = Suite(&DeploySuite{}) |
998 | - |
999 | -func (s *DeploySuite) runDeploy(c *C, args params.ServiceDeploy) error { |
1000 | - c.Logf("Running deploy: %+v", args) |
1001 | - conf, err := s.State.EnvironConfig() |
1002 | - c.Assert(err, IsNil) |
1003 | - curl, err := charm.InferURL(args.CharmUrl, conf.DefaultSeries()) |
1004 | - c.Assert(err, IsNil) |
1005 | - repo, err := charm.InferRepository(curl, os.Getenv("JUJU_REPOSITORY")) |
1006 | - c.Assert(err, IsNil) |
1007 | - return statecmd.ServiceDeploy(s.State, args, s.Conn, curl, repo) |
1008 | -} |
1009 | - |
1010 | -func (s *DeploySuite) TestCharmDir(c *C) { |
1011 | - coretesting.Charms.ClonedDirPath(s.SeriesPath, "dummy") |
1012 | - args := params.ServiceDeploy{ |
1013 | - CharmUrl: "local:dummy", |
1014 | - NumUnits: 1, |
1015 | - } |
1016 | - err := s.runDeploy(c, args) |
1017 | - c.Assert(err, IsNil) |
1018 | - curl := charm.MustParseURL("local:precise/dummy-1") |
1019 | - // Note that this tests the automatic creation of a service name (dummy) |
1020 | - // from the charm URL. This functionality will be going away soon as |
1021 | - // ServiceName becomes a required argument. |
1022 | - s.AssertService(c, "dummy", curl, 1, 0) |
1023 | -} |
1024 | - |
1025 | -func (s *DeploySuite) TestUpgradeCharmDir(c *C) { |
1026 | - dirPath := coretesting.Charms.ClonedDirPath(s.SeriesPath, "dummy") |
1027 | - args := params.ServiceDeploy{ |
1028 | - CharmUrl: "local:dummy", |
1029 | - BumpRevision: true, |
1030 | - NumUnits: 1, |
1031 | - } |
1032 | - err := s.runDeploy(c, args) |
1033 | - c.Assert(err, IsNil) |
1034 | - curl := charm.MustParseURL("local:precise/dummy-2") |
1035 | - s.AssertService(c, "dummy", curl, 1, 0) |
1036 | - // Check the charm really was upgraded. |
1037 | - ch, err := charm.ReadDir(dirPath) |
1038 | - c.Assert(err, IsNil) |
1039 | - c.Assert(ch.Revision(), Equals, 2) |
1040 | -} |
1041 | - |
1042 | -func (s *DeploySuite) TestCharmBundle(c *C) { |
1043 | - coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
1044 | - args := params.ServiceDeploy{ |
1045 | - CharmUrl: "local:dummy", |
1046 | - ServiceName: "some-service-name", |
1047 | - NumUnits: 1, |
1048 | - } |
1049 | - err := s.runDeploy(c, args) |
1050 | - c.Assert(err, IsNil) |
1051 | - curl := charm.MustParseURL("local:precise/dummy-1") |
1052 | - s.AssertService(c, "some-service-name", curl, 1, 0) |
1053 | -} |
1054 | - |
1055 | -func (s *DeploySuite) TestForceMachine(c *C) { |
1056 | - coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
1057 | - machine, err := s.State.AddMachine("precise", state.JobHostUnits) |
1058 | - c.Assert(err, IsNil) |
1059 | - args := params.ServiceDeploy{ |
1060 | - CharmUrl: "local:dummy", |
1061 | - ServiceName: "portlandia", |
1062 | - ForceMachineId: machine.Id(), |
1063 | - NumUnits: 1, |
1064 | - } |
1065 | - err = s.runDeploy(c, args) |
1066 | - c.Assert(err, IsNil) |
1067 | - svc, err := s.State.Service("portlandia") |
1068 | - c.Assert(err, IsNil) |
1069 | - units, err := svc.AllUnits() |
1070 | - c.Assert(err, IsNil) |
1071 | - c.Assert(units, HasLen, 1) |
1072 | - mid, err := units[0].AssignedMachineId() |
1073 | - c.Assert(err, IsNil) |
1074 | - c.Assert(mid, Equals, machine.Id()) |
1075 | -} |
1076 | - |
1077 | -func (s *DeploySuite) TestForceMachineInvalid(c *C) { |
1078 | - coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
1079 | - args := params.ServiceDeploy{ |
1080 | - CharmUrl: "local:dummy", |
1081 | - ServiceName: "portlandia", |
1082 | - ForceMachineId: "42", |
1083 | - NumUnits: 1, |
1084 | - } |
1085 | - err := s.runDeploy(c, args) |
1086 | - c.Assert(err, ErrorMatches, `cannot assign unit "portlandia/0" to machine: machine 42 not found`) |
1087 | - |
1088 | - args = params.ServiceDeploy{ |
1089 | - CharmUrl: "local:dummy", |
1090 | - ServiceName: "portlandia", |
1091 | - ForceMachineId: "abc", |
1092 | - NumUnits: 1, |
1093 | - } |
1094 | - |
1095 | - err = s.runDeploy(c, args) |
1096 | - c.Assert(err, ErrorMatches, `invalid machine id "abc"`) |
1097 | - |
1098 | - machine, err := s.State.AddMachine("precise", state.JobHostUnits) |
1099 | - args = params.ServiceDeploy{ |
1100 | - CharmUrl: "local:dummy", |
1101 | - ServiceName: "portlandia", |
1102 | - ForceMachineId: machine.Id(), |
1103 | - NumUnits: 5, |
1104 | - } |
1105 | - err = s.runDeploy(c, args) |
1106 | - c.Assert(err, ErrorMatches, `force-machine cannot be used for multiple units`) |
1107 | - |
1108 | - coretesting.Charms.BundlePath(s.SeriesPath, "logging") |
1109 | - args = params.ServiceDeploy{ |
1110 | - CharmUrl: "local:logging", |
1111 | - ForceMachineId: machine.Id(), |
1112 | - } |
1113 | - err = s.runDeploy(c, args) |
1114 | - c.Assert(err, ErrorMatches, `subordinate service cannot specify force-machine`) |
1115 | -} |
1116 | - |
1117 | -func (s *DeploySuite) TestCannotUpgradeCharmBundle(c *C) { |
1118 | - coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
1119 | - args := params.ServiceDeploy{ |
1120 | - CharmUrl: "local:dummy", |
1121 | - BumpRevision: true, |
1122 | - } |
1123 | - err := s.runDeploy(c, args) |
1124 | - c.Assert(err, ErrorMatches, `cannot increment revision of charm "local:precise/dummy-1": not a directory`) |
1125 | - // Verify state not touched... |
1126 | - curl := charm.MustParseURL("local:precise/dummy-1") |
1127 | - _, err = s.State.Charm(curl) |
1128 | - c.Assert(err, ErrorMatches, `charm "local:precise/dummy-1" not found`) |
1129 | - _, err = s.State.Service("dummy") |
1130 | - c.Assert(err, ErrorMatches, `service "dummy" not found`) |
1131 | -} |
1132 | - |
1133 | -func (s *DeploySuite) TestAddsPeerRelations(c *C) { |
1134 | - coretesting.Charms.BundlePath(s.SeriesPath, "riak") |
1135 | - args := params.ServiceDeploy{ |
1136 | - CharmUrl: "local:riak", |
1137 | - NumUnits: 1, |
1138 | - } |
1139 | - err := s.runDeploy(c, args) |
1140 | - c.Assert(err, IsNil) |
1141 | - curl := charm.MustParseURL("local:precise/riak-7") |
1142 | - _, rels := s.AssertService(c, "riak", curl, 1, 1) |
1143 | - rel := rels[0] |
1144 | - ep, err := rel.Endpoint("riak") |
1145 | - c.Assert(err, IsNil) |
1146 | - c.Assert(ep.Name, Equals, "ring") |
1147 | - c.Assert(ep.Role, Equals, charm.RolePeer) |
1148 | - c.Assert(ep.Scope, Equals, charm.ScopeGlobal) |
1149 | -} |
1150 | - |
1151 | -func (s *DeploySuite) TestNumUnits(c *C) { |
1152 | - coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
1153 | - args := params.ServiceDeploy{ |
1154 | - CharmUrl: "local:dummy", |
1155 | - NumUnits: 13, |
1156 | - } |
1157 | - err := s.runDeploy(c, args) |
1158 | - c.Assert(err, IsNil) |
1159 | - curl := charm.MustParseURL("local:precise/dummy-1") |
1160 | - s.AssertService(c, "dummy", curl, 13, 0) |
1161 | -} |
1162 | - |
1163 | -func (s *DeploySuite) TestNumUnitsZero(c *C) { |
1164 | - coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
1165 | - args := params.ServiceDeploy{ |
1166 | - CharmUrl: "local:dummy", |
1167 | - NumUnits: 0, |
1168 | - } |
1169 | - err := s.runDeploy(c, args) |
1170 | - c.Assert(err, ErrorMatches, "must deploy at least one unit") |
1171 | -} |
1172 | - |
1173 | -func (s *DeploySuite) TestSubordinateCharm(c *C) { |
1174 | - coretesting.Charms.BundlePath(s.SeriesPath, "logging") |
1175 | - args := params.ServiceDeploy{ |
1176 | - CharmUrl: "local:logging", |
1177 | - } |
1178 | - err := s.runDeploy(c, args) |
1179 | - c.Assert(err, IsNil) |
1180 | - curl := charm.MustParseURL("local:precise/logging-1") |
1181 | - s.AssertService(c, "logging", curl, 0, 0) |
1182 | -} |
1183 | - |
1184 | -func (s *DeploySuite) TestConfigMap(c *C) { |
1185 | - coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
1186 | - args := params.ServiceDeploy{ |
1187 | - CharmUrl: "local:dummy", |
1188 | - Config: map[string]string{ |
1189 | - "skill-level": "1", |
1190 | - "username": "", |
1191 | - }, |
1192 | - NumUnits: 1, |
1193 | - } |
1194 | - err := s.runDeploy(c, args) |
1195 | - c.Assert(err, IsNil) |
1196 | - curl := charm.MustParseURL("local:precise/dummy-1") |
1197 | - s.AssertService(c, "dummy", curl, 1, 0) |
1198 | - svc, err := s.State.Service("dummy") |
1199 | - c.Assert(err, IsNil) |
1200 | - settings, err := svc.ConfigSettings() |
1201 | - c.Assert(err, IsNil) |
1202 | - c.Assert(settings, DeepEquals, charm.Settings{ |
1203 | - "skill-level": int64(1), |
1204 | - }) |
1205 | -} |
1206 | - |
1207 | -func (s *DeploySuite) TestConfigYAML(c *C) { |
1208 | - coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
1209 | - args := params.ServiceDeploy{ |
1210 | - CharmUrl: "local:dummy", |
1211 | - ConfigYAML: "dummy: {skill-level: 9001, username: null}", |
1212 | - NumUnits: 1, |
1213 | - } |
1214 | - err := s.runDeploy(c, args) |
1215 | - c.Assert(err, IsNil) |
1216 | - curl := charm.MustParseURL("local:precise/dummy-1") |
1217 | - s.AssertService(c, "dummy", curl, 1, 0) |
1218 | - svc, err := s.State.Service("dummy") |
1219 | - c.Assert(err, IsNil) |
1220 | - settings, err := svc.ConfigSettings() |
1221 | - c.Assert(err, IsNil) |
1222 | - c.Assert(settings, DeepEquals, charm.Settings{ |
1223 | - "skill-level": int64(9001), |
1224 | - }) |
1225 | -} |
1226 | - |
1227 | -func (s *DeploySuite) TestConstraints(c *C) { |
1228 | - coretesting.Charms.BundlePath(s.SeriesPath, "dummy") |
1229 | - args := params.ServiceDeploy{ |
1230 | - CharmUrl: "local:dummy", |
1231 | - Constraints: constraints.MustParse("mem=2G cpu-cores=2"), |
1232 | - NumUnits: 1, |
1233 | - } |
1234 | - err := s.runDeploy(c, args) |
1235 | - c.Assert(err, IsNil) |
1236 | - curl := charm.MustParseURL("local:precise/dummy-1") |
1237 | - service, _ := s.AssertService(c, "dummy", curl, 1, 0) |
1238 | - cons, err := service.Constraints() |
1239 | - c.Assert(err, IsNil) |
1240 | - c.Assert(cons, DeepEquals, constraints.MustParse("mem=2G cpu-cores=2")) |
1241 | -} |
1242 | - |
1243 | -func (s *DeploySuite) TestSubordinateConstraints(c *C) { |
1244 | - coretesting.Charms.BundlePath(s.SeriesPath, "logging") |
1245 | - args := params.ServiceDeploy{ |
1246 | - CharmUrl: "local:logging", |
1247 | - Constraints: constraints.MustParse("mem=1G"), |
1248 | - } |
1249 | - err := s.runDeploy(c, args) |
1250 | - c.Assert(err, Equals, state.ErrSubordinateConstraints) |
1251 | -} |
1252 | |
1253 | === modified file 'testing/charm.go' |
1254 | --- testing/charm.go 2013-05-02 15:55:42 +0000 |
1255 | +++ testing/charm.go 2013-06-11 02:14:27 +0000 |
1256 | @@ -111,3 +111,62 @@ |
1257 | check(err) |
1258 | return ch |
1259 | } |
1260 | + |
1261 | +type MockCharmStore struct { |
1262 | + charms map[string]map[int]*charm.Bundle |
1263 | +} |
1264 | + |
1265 | +func NewMockCharmStore() *MockCharmStore { |
1266 | + return &MockCharmStore{map[string]map[int]*charm.Bundle{}} |
1267 | +} |
1268 | + |
1269 | +func (s *MockCharmStore) SetCharm(curl *charm.URL, bundle *charm.Bundle) error { |
1270 | + base := curl.WithRevision(-1).String() |
1271 | + if curl.Revision < 0 { |
1272 | + return fmt.Errorf("bad charm url revision") |
1273 | + } |
1274 | + if bundle == nil { |
1275 | + delete(s.charms[base], curl.Revision) |
1276 | + return nil |
1277 | + } |
1278 | + bundleRev := bundle.Revision() |
1279 | + bundleName := bundle.Meta().Name |
1280 | + if bundleName != curl.Name || bundleRev != curl.Revision { |
1281 | + return fmt.Errorf("charm url %s mismatch with bundle %s-%d", curl, bundleName, bundleRev) |
1282 | + } |
1283 | + if _, found := s.charms[base]; !found { |
1284 | + s.charms[base] = map[int]*charm.Bundle{} |
1285 | + } |
1286 | + s.charms[base][curl.Revision] = bundle |
1287 | + return nil |
1288 | +} |
1289 | + |
1290 | +func (s *MockCharmStore) interpret(curl *charm.URL) (base string, rev int) { |
1291 | + base, rev = curl.WithRevision(-1).String(), curl.Revision |
1292 | + if rev == -1 { |
1293 | + for candidate := range s.charms[base] { |
1294 | + if candidate > rev { |
1295 | + rev = candidate |
1296 | + } |
1297 | + } |
1298 | + } |
1299 | + return |
1300 | +} |
1301 | + |
1302 | +func (s *MockCharmStore) Get(curl *charm.URL) (charm.Charm, error) { |
1303 | + base, rev := s.interpret(curl) |
1304 | + charm, found := s.charms[base][rev] |
1305 | + if !found { |
1306 | + return nil, fmt.Errorf("charm not found in mock store: %s", curl) |
1307 | + } |
1308 | + return charm, nil |
1309 | +} |
1310 | + |
1311 | +func (s *MockCharmStore) Latest(curl *charm.URL) (int, error) { |
1312 | + curl = curl.WithRevision(-1) |
1313 | + base, rev := s.interpret(curl) |
1314 | + if _, found := s.charms[base][rev]; !found { |
1315 | + return 0, fmt.Errorf("charm not found in mock store: %s", curl) |
1316 | + } |
1317 | + return rev, nil |
1318 | +} |
Reviewers: mp+168581_ code.launchpad. net,
Message:
Please take a look.
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: /code.launchpad .net/~fwereade/ juju-core/ config- 6-state- service- sane-methods/ +merge/ 168580
https:/
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/10166044/
Affected files: deploy_ test.go params/ params. go /client. go /client_ test.go /perm_test. go deploy. go deploy_ test.go
A [revision details]
M cmd/juju/deploy.go
M cmd/juju/
M juju/conn.go
M juju/conn_test.go
M state/api/
M state/apiserver
M state/apiserver
M state/apiserver
D state/statecmd/
D state/statecmd/
M testing/charm.go