juju-core

Merge lp:~niemeyer/juju-core/resource-not-found into lp:~juju/juju-core/trunk

  1. resource-not-found
  2. Merge into trunk
Proposed by Gustavo Niemeyer
Status: Merged
Merged at revision: 562
Proposed branch: lp:~niemeyer/juju-core/resource-not-found
Merge into: lp:~juju/juju-core/trunk
Diff against target: 427 lines (+94/-47)
14 files modified
cmd/juju/deploy_test.go (+2/-2)
cmd/juju/expose_test.go (+1/-1)
cmd/juju/unexpose_test.go (+1/-1)
state/assign_test.go (+2/-2)
state/charm_test.go (+4/-4)
state/confignode.go (+0/-9)
state/machine.go (+1/-2)
state/machine_test.go (+5/-4)
state/relation_test.go (+9/-4)
state/service_test.go (+2/-2)
state/state.go (+37/-5)
state/state_test.go (+20/-7)
state/unit.go (+4/-4)
state/unit_test.go (+6/-0)
To merge this branch: bzr merge lp:~niemeyer/juju-core/resource-not-found
Reviewer Review Type Date Requested Status
The Go Language Gophers Pending
Review via email: mp+125917@code.launchpad.net

Description of the change

state: getter methods return NotFoundError

Due to the in-progress lifecycle changes, client code
must be able to distinguish not found errors.

https://codereview.appspot.com/6549056/

To post a comment you must log in.
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Reviewers: mp+125917_code.launchpad.net,

Message:
Please take a look.

Description:
state: getter methods return NotFoundError

Due to the in-progress lifecycle changes, client code
must be able to distinguish not found errors from other
kinds of errors in client code.

https://code.launchpad.net/~niemeyer/juju-core/resource-not-found/+merge/125917

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/juju/deploy_test.go
   M cmd/juju/expose_test.go
   M cmd/juju/unexpose_test.go
   M state/assign_test.go
   M state/charm_test.go
   M state/confignode.go
   M state/machine.go
   M state/machine_test.go
   M state/relation_test.go
   M state/service_test.go
   M state/state.go
   M state/state_test.go
   M state/unit.go
   M state/unit_test.go

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Please take a look.

https://codereview.appspot.com/6549056/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

looks reasonable, but one thought below.

https://codereview.appspot.com/6549056/diff/2002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode46
state/state.go:46: format string
i'm not sure that i see the benefit in storing the format and args here.
are we really concerned that the work involved in formatting the error
message is going to slow us down?

it's also potentially dangerous - if we pass in a mutable value whose
String value can change, the error may change value after creation.

https://codereview.appspot.com/6549056/

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

*** Submitted:

state: getter methods return NotFoundError

Due to the in-progress lifecycle changes, client code
must be able to distinguish not found errors.

R=dfc, TheMue, rog
CC=
https://codereview.appspot.com/6549056

https://codereview.appspot.com/6549056/diff/2002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode46
state/state.go:46: format string
On 2012/09/24 14:12:04, rog wrote:
> i'm not sure that i see the benefit in storing the format and args
 here. are we
> really concerned that the work involved in formatting the error
 message is going
> to slow us down?

I certainly don't care enough to argue. I'll change it.

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode53
state/state.go:53:
On 2012/09/24 05:20:35, dfc wrote:
> comment please.

I honestly think it's not worth it in this case. Reading the body of the
function is more clear and takes less time than reading any comments we
add here, and since it's a private function it won't show up in the
docs.

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode55
state/state.go:55: return &NotFoundError{format, args}
On 2012/09/24 09:37:11, TheMue wrote:
> If no later access to the args is planned the error message could
 already be

Changed so NotFoundError has a dumb msg argument.

https://codereview.appspot.com/6549056/diff/2002/state/state.go#newcode384
state/state.go:384: func (s *State) Relation(endpoints
...RelationEndpoint) (r *Relation, err error) {
On 2012/09/24 05:20:35, dfc wrote:
> suggestion: drop the named return values as we're not using defer
 ErrorContextf
> anymore in this method.

Done.

https://codereview.appspot.com/6549056/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cmd/juju/deploy_test.go'
2--- cmd/juju/deploy_test.go 2012-09-21 19:11:50 +0000
3+++ cmd/juju/deploy_test.go 2012-09-24 03:51:21 +0000
4@@ -149,9 +149,9 @@
5 // Verify state not touched...
6 curl := charm.MustParseURL("local:precise/dummy-1")
7 _, err = s.State.Charm(curl)
8- c.Assert(err, ErrorMatches, `cannot get charm "local:precise/dummy-1": not found`)
9+ c.Assert(err, ErrorMatches, `charm "local:precise/dummy-1" not found`)
10 _, err = s.State.Service("dummy")
11- c.Assert(err, ErrorMatches, `cannot get service "dummy": not found`)
12+ c.Assert(err, ErrorMatches, `service "dummy" not found`)
13 }
14
15 func (s *DeploySuite) TestAddsPeerRelations(c *C) {
16
17=== modified file 'cmd/juju/expose_test.go'
18--- cmd/juju/expose_test.go 2012-09-21 19:14:11 +0000
19+++ cmd/juju/expose_test.go 2012-09-24 03:51:21 +0000
20@@ -40,5 +40,5 @@
21 s.assertExposed(c, "some-service-name")
22
23 err = runExpose(c, "nonexistent-service")
24- c.Assert(err, ErrorMatches, `cannot get service "nonexistent-service": not found`)
25+ c.Assert(err, ErrorMatches, `service "nonexistent-service" not found`)
26 }
27
28=== modified file 'cmd/juju/unexpose_test.go'
29--- cmd/juju/unexpose_test.go 2012-09-21 19:14:11 +0000
30+++ cmd/juju/unexpose_test.go 2012-09-24 03:51:21 +0000
31@@ -44,5 +44,5 @@
32 s.assertExposed(c, "some-service-name", false)
33
34 err = runUnexpose(c, "nonexistent-service")
35- c.Assert(err, ErrorMatches, `cannot get service "nonexistent-service": not found`)
36+ c.Assert(err, ErrorMatches, `service "nonexistent-service" not found`)
37 }
38
39=== modified file 'state/assign_test.go'
40--- state/assign_test.go 2012-09-21 14:54:59 +0000
41+++ state/assign_test.go 2012-09-24 03:51:21 +0000
42@@ -362,7 +362,7 @@
43 c.Assert(err, IsNil)
44
45 _, err = unit.AssignToUnusedMachine()
46- c.Assert(err, ErrorMatches, `cannot assign unit "wordpress/0" to unused machine.*: cannot get unit "wordpress/0": not found`)
47+ c.Assert(err, ErrorMatches, `cannot assign unit "wordpress/0" to unused machine.*: unit "wordpress/0" not found`)
48 }
49
50 func (s *AssignSuite) TestAssignUnitToUnusedMachineWithRemovedUnit(c *C) {
51@@ -381,7 +381,7 @@
52 c.Assert(err, IsNil)
53
54 _, err = unit.AssignToUnusedMachine()
55- c.Assert(err, ErrorMatches, `cannot assign unit "wordpress/0" to unused machine.*: cannot get unit "wordpress/0": not found`)
56+ c.Assert(err, ErrorMatches, `cannot assign unit "wordpress/0" to unused machine.*: unit "wordpress/0" not found`)
57 }
58
59 func (s *AssignSuite) TestAssignUnitToUnusedMachineOnlyZero(c *C) {
60
61=== modified file 'state/charm_test.go'
62--- state/charm_test.go 2012-09-19 13:40:41 +0000
63+++ state/charm_test.go 2012-09-24 03:51:21 +0000
64@@ -3,6 +3,7 @@
65 import (
66 . "launchpad.net/gocheck"
67 "launchpad.net/juju-core/charm"
68+ "launchpad.net/juju-core/state"
69 "net/url"
70 )
71
72@@ -40,10 +41,9 @@
73 )
74 }
75
76-func (s *CharmSuite) TestGetNonExistentCharm(c *C) {
77- // Check that getting a non-existent charm fails nicely.
78-
79+func (s *CharmSuite) TestCharmNotFound(c *C) {
80 curl := charm.MustParseURL("local:anotherseries/dummy-1")
81 _, err := s.State.Charm(curl)
82- c.Assert(err, ErrorMatches, `cannot get charm "local:anotherseries/dummy-1": .*`)
83+ c.Assert(err, ErrorMatches, `charm "local:anotherseries/dummy-1" not found`)
84+ c.Assert(err, FitsTypeOf, &state.NotFoundError{})
85 }
86
87=== modified file 'state/confignode.go'
88--- state/confignode.go 2012-09-21 13:01:54 +0000
89+++ state/confignode.go 2012-09-24 03:51:21 +0000
90@@ -61,15 +61,6 @@
91 txnRevno int64
92 }
93
94-// NotFoundError represents the error that something is not found.
95-type NotFoundError struct {
96- what string
97-}
98-
99-func (e *NotFoundError) Error() string {
100- return fmt.Sprintf("%s not found", e.what)
101-}
102-
103 // Keys returns the current keys in alphabetical order.
104 func (c *ConfigNode) Keys() []string {
105 keys := []string{}
106
107=== modified file 'state/machine.go'
108--- state/machine.go 2012-09-20 09:15:49 +0000
109+++ state/machine.go 2012-09-24 03:51:21 +0000
110@@ -149,8 +149,7 @@
111 // InstanceId returns the provider specific machine id for this machine.
112 func (m *Machine) InstanceId() (string, error) {
113 if m.doc.InstanceId == "" {
114- msg := fmt.Sprintf("instance id for machine %d is not set", m.Id())
115- return "", &NotFoundError{msg}
116+ return "", notFound("instance id for machine %v", m)
117 }
118 return m.doc.InstanceId, nil
119 }
120
121=== modified file 'state/machine_test.go'
122--- state/machine_test.go 2012-09-21 17:26:00 +0000
123+++ state/machine_test.go 2012-09-24 03:51:21 +0000
124@@ -96,13 +96,14 @@
125 err = machine.Refresh()
126 c.Assert(err, IsNil)
127 iid, err := machine.InstanceId()
128- c.Assert(err, FitsTypeOf, &state.NotFoundError{})
129+ c.Assert(state.IsNotFound(err), Equals, true)
130 c.Assert(iid, Equals, "")
131 }
132
133 func (s *MachineSuite) TestMachineInstanceIdMissing(c *C) {
134 iid, err := s.machine.InstanceId()
135- c.Assert(err, FitsTypeOf, &state.NotFoundError{})
136+ c.Assert(state.IsNotFound(err), Equals, true)
137+ c.Assert(err, ErrorMatches, "instance id for machine 0 not found")
138 c.Assert(iid, Equals, "")
139 }
140
141@@ -118,7 +119,7 @@
142 err = machine.Refresh()
143 c.Assert(err, IsNil)
144 iid, err := machine.InstanceId()
145- c.Assert(err, FitsTypeOf, &state.NotFoundError{})
146+ c.Assert(state.IsNotFound(err), Equals, true)
147 c.Assert(iid, Equals, "")
148 }
149
150@@ -322,7 +323,7 @@
151 //info.tools, err = m.AgentTools()
152 //c.Assert(err, IsNil)
153 info.instanceId, err = m.InstanceId()
154- if _, ok := err.(*state.NotFoundError); !ok {
155+ if !state.IsNotFound(err) {
156 c.Assert(err, IsNil)
157 }
158 c.Assert(info, DeepEquals, test.want)
159
160=== modified file 'state/relation_test.go'
161--- state/relation_test.go 2012-09-20 15:34:42 +0000
162+++ state/relation_test.go 2012-09-24 03:51:21 +0000
163@@ -63,9 +63,14 @@
164 c.Assert(err, ErrorMatches, `cannot add relation "": cannot relate 0 endpoints`)
165 _, err = s.State.AddRelation(proep, reqep, peerep)
166 c.Assert(err, ErrorMatches, `cannot add relation "peer:baz pro:foo req:bar": cannot relate 3 endpoints`)
167+}
168
169- _, err = s.State.Relation(peerep)
170- c.Assert(err, ErrorMatches, `cannot get relation "peer:baz": .*`)
171+func (s *RelationSuite) TestRelationNotFound(c *C) {
172+ subway := state.RelationEndpoint{"subway", "mongodb", "db", state.RoleRequirer, charm.ScopeGlobal}
173+ mongo := state.RelationEndpoint{"mongo", "mongodb", "server", state.RoleProvider, charm.ScopeGlobal}
174+ _, err := s.State.Relation(subway, mongo)
175+ c.Assert(err, ErrorMatches, `relation "mongo:server subway:db" not found`)
176+ c.Assert(state.IsNotFound(err), Equals, true)
177 }
178
179 func (s *RelationSuite) TestProviderRequirerRelation(c *C) {
180@@ -147,9 +152,9 @@
181 err = s.State.RemoveService(peer)
182 c.Assert(err, IsNil)
183 _, err = s.State.Service("peer")
184- c.Assert(err, ErrorMatches, `cannot get service "peer": not found`)
185+ c.Assert(err, ErrorMatches, `service "peer" not found`)
186 _, err = s.State.Relation(peerep)
187- c.Assert(err, ErrorMatches, `cannot get relation "peer:baz": not found`)
188+ c.Assert(err, ErrorMatches, `relation "peer:baz" not found`)
189 }
190
191 func assertNoRelations(c *C, srv *state.Service) {
192
193=== modified file 'state/service_test.go'
194--- state/service_test.go 2012-09-21 18:26:46 +0000
195+++ state/service_test.go 2012-09-24 03:51:21 +0000
196@@ -210,7 +210,7 @@
197 unit, err = s.State.Unit("mysql/0/0")
198 c.Assert(err, ErrorMatches, `"mysql/0/0" is not a valid unit name`)
199 unit, err = s.State.Unit("pressword/0")
200- c.Assert(err, ErrorMatches, `cannot get unit "pressword/0": not found`)
201+ c.Assert(err, ErrorMatches, `unit "pressword/0" not found`)
202
203 // Add another service to check units are not misattributed.
204 mysql, err := s.State.AddService("wordpress", s.charm)
205@@ -284,7 +284,7 @@
206 err = s.State.RemoveService(s.service)
207 c.Assert(err, IsNil)
208 _, err = s.State.Unit("mysql/0")
209- c.Assert(err, ErrorMatches, `cannot get unit "mysql/0": not found`)
210+ c.Assert(err, ErrorMatches, `unit "mysql/0" not found`)
211 }
212
213 func (s *ServiceSuite) TestServiceConfig(c *C) {
214
215=== modified file 'state/state.go'
216--- state/state.go 2012-09-21 17:26:00 +0000
217+++ state/state.go 2012-09-24 03:51:21 +0000
218@@ -41,6 +41,25 @@
219 return validUnit.MatchString(name)
220 }
221
222+// NotFoundError represents the error that something is not found.
223+type NotFoundError struct {
224+ format string
225+ args []interface{}
226+}
227+
228+func (e *NotFoundError) Error() string {
229+ return fmt.Sprintf(e.format+" not found", e.args...)
230+}
231+
232+func notFound(format string, args ...interface{}) error {
233+ return &NotFoundError{format, args}
234+}
235+
236+func IsNotFound(err error) bool {
237+ _, ok := err.(*NotFoundError)
238+ return ok
239+}
240+
241 // State represents the state of an environment
242 // managed by juju.
243 type State struct {
244@@ -155,6 +174,9 @@
245 mdoc := &machineDoc{}
246 sel := D{{"_id", id}}
247 err := s.machines.Find(sel).One(mdoc)
248+ if err == mgo.ErrNotFound {
249+ return nil, notFound("machine %d", id)
250+ }
251 if err != nil {
252 return nil, fmt.Errorf("cannot get machine %d: %v", id, err)
253 }
254@@ -183,10 +205,12 @@
255 func (s *State) Charm(curl *charm.URL) (*Charm, error) {
256 cdoc := &charmDoc{}
257 err := s.charms.Find(D{{"_id", curl}}).One(cdoc)
258+ if err == mgo.ErrNotFound {
259+ return nil, notFound("charm %q", curl)
260+ }
261 if err != nil {
262 return nil, fmt.Errorf("cannot get charm %q: %v", curl, err)
263 }
264-
265 return newCharm(s, cdoc)
266 }
267
268@@ -277,6 +301,9 @@
269 sdoc := &serviceDoc{}
270 sel := D{{"_id", name}}
271 err = s.services.Find(sel).One(sdoc)
272+ if err == mgo.ErrNotFound {
273+ return nil, notFound("service %q", name)
274+ }
275 if err != nil {
276 return nil, fmt.Errorf("cannot get service %q: %v", name, err)
277 }
278@@ -355,12 +382,14 @@
279
280 // Relation returns the existing relation with the given endpoints.
281 func (s *State) Relation(endpoints ...RelationEndpoint) (r *Relation, err error) {
282- defer trivial.ErrorContextf(&err, "cannot get relation %q", relationKey(endpoints))
283-
284 doc := relationDoc{}
285- err = s.relations.Find(D{{"_id", relationKey(endpoints)}}).One(&doc)
286+ key := relationKey(endpoints)
287+ err = s.relations.Find(D{{"_id", key}}).One(&doc)
288+ if err == mgo.ErrNotFound {
289+ return nil, notFound("relation %q", key)
290+ }
291 if err != nil {
292- return nil, err
293+ return nil, fmt.Errorf("cannot get relation %q: %v", key, err)
294 }
295 return newRelation(s, &doc), nil
296 }
297@@ -391,6 +420,9 @@
298 }
299 doc := unitDoc{}
300 err := s.units.FindId(name).One(&doc)
301+ if err == mgo.ErrNotFound {
302+ return nil, notFound("unit %q", name)
303+ }
304 if err != nil {
305 return nil, fmt.Errorf("cannot get unit %q: %v", name, err)
306 }
307
308=== modified file 'state/state_test.go'
309--- state/state_test.go 2012-09-21 18:26:46 +0000
310+++ state/state_test.go 2012-09-24 03:51:21 +0000
311@@ -30,6 +30,13 @@
312 }
313 }
314
315+func (s *StateSuite) TestIsNotFound(c *C) {
316+ err1 := fmt.Errorf("unrelated error")
317+ err2 := &state.NotFoundError{}
318+ c.Assert(state.IsNotFound(err1), Equals, false)
319+ c.Assert(state.IsNotFound(err2), Equals, true)
320+}
321+
322 func (s *StateSuite) TestAddCharm(c *C) {
323 // Check that adding charms from scratch works correctly.
324 ch := testing.Charms.Dir("series", "dummy")
325@@ -97,6 +104,12 @@
326 c.Assert(machine.Id(), Equals, expectedId)
327 }
328
329+func (s *StateSuite) TestMachineNotFound(c *C) {
330+ _, err := s.State.Machine(0)
331+ c.Assert(err, ErrorMatches, "machine 0 not found")
332+ c.Assert(state.IsNotFound(err), Equals, true)
333+}
334+
335 func (s *StateSuite) TestAllMachines(c *C) {
336 c.Skip("Marshalling of agent tools is currently broken")
337 numInserts := 42
338@@ -153,6 +166,12 @@
339 c.Assert(ch.URL(), DeepEquals, charm.URL())
340 }
341
342+func (s *StateSuite) TestServiceNotFound(c *C) {
343+ _, err := s.State.Service("bummer")
344+ c.Assert(err, ErrorMatches, `service "bummer" not found`)
345+ c.Assert(state.IsNotFound(err), Equals, true)
346+}
347+
348 func (s *StateSuite) TestRemoveService(c *C) {
349 charm := s.AddTestingCharm(c, "dummy")
350 service, err := s.State.AddService("wordpress", charm)
351@@ -166,18 +185,12 @@
352 err = s.State.RemoveService(service)
353 c.Assert(err, IsNil)
354 _, err = s.State.Service("wordpress")
355- c.Assert(err, ErrorMatches, `cannot get service "wordpress": .*`)
356+ c.Assert(err, ErrorMatches, `service "wordpress" not found`)
357
358 err = s.State.RemoveService(service)
359 c.Assert(err, IsNil)
360 }
361
362-func (s *StateSuite) TestReadNonExistentService(c *C) {
363- // BUG(aram): use error strings from state.
364- _, err := s.State.Service("pressword")
365- c.Assert(err, ErrorMatches, `cannot get service "pressword": .*`)
366-}
367-
368 func (s *StateSuite) TestAllServices(c *C) {
369 charm := s.AddTestingCharm(c, "dummy")
370 services, err := s.State.AllServices()
371
372=== modified file 'state/unit.go'
373--- state/unit.go 2012-09-21 18:26:46 +0000
374+++ state/unit.go 2012-09-24 03:51:21 +0000
375@@ -526,7 +526,7 @@
376 }
377 err = u.st.runner.Run(ops, "", nil)
378 if err != nil {
379- return fmt.Errorf("cannot unassign unit %q from machine: %v", u, onAbort(err, &NotFoundError{"machine"}))
380+ return fmt.Errorf("cannot unassign unit %q from machine: %v", u, onAbort(err, notFound("machine")))
381 }
382 u.doc.MachineId = nil
383 return nil
384@@ -541,7 +541,7 @@
385 Update: D{{"$set", D{{"publicaddress", address}}}},
386 }}
387 if err := u.st.runner.Run(ops, "", nil); err != nil {
388- return fmt.Errorf("cannot set public address of unit %q: %v", u, onAbort(err, &NotFoundError{"machine"}))
389+ return fmt.Errorf("cannot set public address of unit %q: %v", u, onAbort(err, notFound("machine")))
390 }
391 u.doc.PublicAddress = address
392 return nil
393@@ -557,7 +557,7 @@
394 }}
395 err := u.st.runner.Run(ops, "", nil)
396 if err != nil {
397- return fmt.Errorf("cannot set private address of unit %q: %v", u, &NotFoundError{"unit"})
398+ return fmt.Errorf("cannot set private address of unit %q: %v", u, notFound("unit"))
399 }
400 u.doc.PrivateAddress = address
401 return nil
402@@ -609,7 +609,7 @@
403 }}
404 err := u.st.runner.Run(ops, "", nil)
405 if err != nil {
406- return fmt.Errorf("cannot clear resolved mode for unit %q: %v", u, &NotFoundError{"unit"})
407+ return fmt.Errorf("cannot clear resolved mode for unit %q: %v", u, notFound("unit"))
408 }
409 u.doc.Resolved = ResolvedNone
410 return nil
411
412=== modified file 'state/unit_test.go'
413--- state/unit_test.go 2012-09-21 18:26:46 +0000
414+++ state/unit_test.go 2012-09-24 03:51:21 +0000
415@@ -23,6 +23,12 @@
416 c.Assert(err, IsNil)
417 }
418
419+func (s *UnitSuite) TestUnitNotFound(c *C) {
420+ _, err := s.State.Unit("subway/0")
421+ c.Assert(err, ErrorMatches, `unit "subway/0" not found`)
422+ c.Assert(state.IsNotFound(err), Equals, true)
423+}
424+
425 func (s *UnitSuite) TestGetSetPublicAddress(c *C) {
426 address, err := s.unit.PublicAddress()
427 c.Assert(err, ErrorMatches, `public address of unit "wordpress/0" not found`)

Subscribers

People subscribed via source and target branches