Merge lp:~fwereade/juju-core/errors-cleanup into lp:~juju/juju-core/trunk
- errors-cleanup
- Merge into trunk
Status: | Rejected |
---|---|
Rejected by: | William Reade |
Proposed branch: | lp:~fwereade/juju-core/errors-cleanup |
Merge into: | lp:~juju/juju-core/trunk |
Diff against target: |
821 lines (+214/-133) 22 files modified
charm/repo.go (+8/-15) charm/repo_test.go (+25/-16) cmd/juju/deploy_test.go (+1/-1) cmd/juju/publish.go (+8/-6) cmd/juju/upgradecharm_test.go (+7/-6) cmd/jujud/agent_test.go (+11/-10) cmd/jujud/bootstrap_test.go (+6/-6) environs/dummy/storage.go (+1/-1) environs/ec2/storage.go (+1/-1) environs/imagemetadata/simplestreams.go (+5/-5) environs/local/storage.go (+1/-1) environs/maas/storage.go (+2/-2) environs/openstack/storage.go (+1/-1) environs/tools.go (+1/-1) environs/tools_test.go (+4/-4) errors/errors.go (+42/-40) errors/errors_test.go (+74/-0) state/apiserver/client_test.go (+1/-1) state/apiserver/errors_test.go (+2/-2) state/open.go (+8/-4) state/state_test.go (+0/-7) testing/charm.go (+5/-3) |
To merge this branch: | bzr merge lp:~fwereade/juju-core/errors-cleanup |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tim Penhey (community) | Approve | ||
Review via email: mp+168928@code.launchpad.net |
Commit message
Description of the change
errors: tests, consistency
We weren't testing or making sensible use of the embedded errors in
NotFoundError and UnauthorizedError; we were doing inappropriate things
with both error types in various situations; the action of NotFoundf was
not consistent with Unauthorizedf.
These conditions no longer apply; and the various charm.Repository types in
play now produce errors.
I'm also more than somewhat freaked out by the "auth fails" magic string.
Can anyone explain that?
William Reade (fwereade) wrote : | # |
Roger Peppe (rogpeppe) wrote : | # |
LGTM with some suggestions. i'd like to lose the stuttering and this
seems like a reasonable CL to do it in.
https:/
File charm/repo.go (right):
https:/
charm/repo.go:288: return errors.
repository %q", curl, repoPath)
NotFoundf("charm %q in repository %q", curl, repoPath) ?
https:/
File environs/
https:/
environs/
"launchpad.
looks like this can lose the import alias.
https:/
File environs/
https:/
environs/
&errors.
in other tests we do check IsNotFound and the error message rather than
using DeepEquals. would that be better here too?
alternatively we could change those other tests to use this idiom,
which seems fine too.
https:/
File errors/errors.go (right):
https:/
errors/
s/NotFoundError
(as discussed)
and ditto for Unauthorized and the Is* functions.
https:/
File state/apiserver
https:/
state/apiserver
no real need to use Raw here, is there?
https:/
File state/open.go (right):
https:/
state/open.go:188: // error value produced my mongo, or mgo, or
something? And what about
s/my mongo/by mongo/
yes, i seem to remember that in some cases, mongo
does not produce an error code when access is unauthorised.
i believe gustavo filed an issue for that, but
i don't recall where or when.
about the "need to login" error, i don't *think*
we need to check that here, as it should happen only
when setting passwords AFAICS (that error is also checked in
SetAdminMongoPa
re: lack of mongo error code.
https:/
File testing/charm.go (right):
https:/
testing/
found in mock store", charmURL)
NotFoundf("charm %q in mock store", charmURL)
(and below)?
Tim Penhey (thumper) wrote : | # |
https:/
File charm/repo.go (right):
https:/
charm/repo.go:288: return errors.
repository %q", curl, repoPath)
On 2013/06/12 13:51:05, rog wrote:
> NotFoundf("charm %q in repository %q", curl, repoPath) ?
What is the difference between NotFoundf and RawNotFoundf?
https:/
File charm/repo_test.go (right):
https:/
charm/repo_
true)
Nice. Although how about a matcher?
c.Assert(err, IsNotFoundError)
or
c.Assert(err, errors.IsNotFound)
https:/
File environs/
https:/
environs/
errors.
Is there a good reason why we have two functions? RawNotFoundf and
NotFoundf? Why have two same but different functions/types?
https:/
File errors/errors.go (right):
https:/
errors/
On 2013/06/12 13:51:05, rog wrote:
> s/NotFoundError
> (as discussed)
> and ditto for Unauthorized and the Is* functions.
+1 as almost always namespaced in the caller code with the package.
errors.
seems nicer to me than
errors.
Hopefully the package name makes it obvious.
(Message brought to you by the department of redundancy department)
https:/
errors/
The name of the method and be behaviour seem to be at odds to me.
Tim Penhey (thumper) wrote : | # |
On 2013/06/18 23:33:17, thumper wrote:
> https:/
> File charm/repo.go (right):
> https:/
> charm/repo.go:288: return errors.
repository
> %q", curl, repoPath)
> On 2013/06/12 13:51:05, rog wrote:
> > NotFoundf("charm %q in repository %q", curl, repoPath) ?
> What is the difference between NotFoundf and RawNotFoundf?
> https:/
> File charm/repo_test.go (right):
https:/
> charm/repo_
true)
> Nice. Although how about a matcher?
> c.Assert(err, IsNotFoundError)
> or
> c.Assert(err, errors.IsNotFound)
https:/
> File environs/
https:/
> environs/
> errors.
> Is there a good reason why we have two functions? RawNotFoundf and
NotFoundf?
> Why have two same but different functions/types?
> https:/
> File errors/errors.go (right):
https:/
> errors/
> On 2013/06/12 13:51:05, rog wrote:
> > s/NotFoundError
> > (as discussed)
> >
> > and ditto for Unauthorized and the Is* functions.
> +1 as almost always namespaced in the caller code with the package.
> errors.IsNotFound
> seems nicer to me than
> errors.
> Hopefully the package name makes it obvious.
> (Message brought to you by the department of redundancy department)
https:/
> errors/
> The name of the method and be behaviour seem to be at odds to me.
LGTM - it is better than what was there before, lets not depend on
perfection
Tim Penhey (thumper) : | # |
John A Meinel (jameinel) wrote : | # |
Note this is still targeted at the old trunk.
Unmerged revisions
- 1268. By William Reade
-
mock charm store now uses errors.
NotFoundError - 1267. By William Reade
-
merge parent
- 1266. By William Reade
-
make errors somewhat consistent
- 1265. By William Reade
-
merge parent
- 1264. By William Reade
-
trivial error message fix (but notice how little it's tested for...)
- 1263. By William Reade
-
rewrite charm.Config for clarity, adding currently-unused ParseSettingsYAML method and new Settings type
- 1262. By Ian Booth
-
Move assignment policy to global env config
Previoysly, each provider defined it's own AssignmentPolicy,
which was hard coded to AssignNew. Now, it's been made an
env setting with default AssignNew. With containers, it will be
possible to add a unit to an existing machine (if not dirty) so
this work is a step in that direction.R=jameinel, fwereade
CC=
https://codereview. appspot. com/9824043 - 1261. By Ian Booth
-
Add dirty flag to machine state
When choosing an unused machine to assign a unit to, a check was being made
to see if there were any principal units for the machine. If machines had units
assigned and then unassigned, the machine would appear clean since number of
principal unit goes to 0 but the machine should still be considered dirty.The machine state has a "clean" flag (boolean). Machines are created with the
clean flag set to true. It is set false when a machine has a unit assigned,
and not reset when units are unassigned. The assign to unused machine logic
is updated to look at the clean flag.R=fwereade, thumper
CC=
https://codereview. appspot. com/9820043 - 1260. By Tim Penhey
-
First part of the provisioner refactoring.
This branch breaks up the current provisioner and defines some interfaces that
we'll be using for containers.The broker interface is what we have starting, stopping instances, and listing
instances and related machines.An environment broker is written that defers the actual calls to the environ.
The provisioner now creates a provisioning task for the environment provider
using a machine watcher and the environment broker. The common provisioning
methods are now in provisioner_task.go. I refactored some of the methods as we were duplicating a lot of calls, like
get all instances, and then getting instances for individual ids. Also,
looking up specific machines, and also getting all machines. Now we just do
the "all" gets, and keep a map around for the duration of the checks.R=wallyworld, mue, fwereade, rog
CC=
https://codereview. appspot. com/9937046 - 1259. By Frank Mueller
-
state: added CleanupWatcher
The CleanupWatcher signals the demand for the running of
state.Cleanup(). It is the first in a row of CLs to add
an according worker.R=fwereade, dimitern
CC=
https://codereview. appspot. com/10078043
Preview Diff
1 | === modified file 'charm/repo.go' | |||
2 | --- charm/repo.go 2013-06-10 20:50:42 +0000 | |||
3 | +++ charm/repo.go 2013-06-12 12:32:25 +0000 | |||
4 | @@ -10,12 +10,14 @@ | |||
5 | 10 | "fmt" | 10 | "fmt" |
6 | 11 | "io" | 11 | "io" |
7 | 12 | "io/ioutil" | 12 | "io/ioutil" |
8 | 13 | "launchpad.net/juju-core/log" | ||
9 | 14 | "net/http" | 13 | "net/http" |
10 | 15 | "net/url" | 14 | "net/url" |
11 | 16 | "os" | 15 | "os" |
12 | 17 | "path/filepath" | 16 | "path/filepath" |
13 | 18 | "strings" | 17 | "strings" |
14 | 18 | |||
15 | 19 | "launchpad.net/juju-core/errors" | ||
16 | 20 | "launchpad.net/juju-core/log" | ||
17 | 19 | ) | 21 | ) |
18 | 20 | 22 | ||
19 | 21 | // CacheDir stores the charm cache directory path. | 23 | // CacheDir stores the charm cache directory path. |
20 | @@ -46,15 +48,6 @@ | |||
21 | 46 | Latest(curl *URL) (int, error) | 48 | Latest(curl *URL) (int, error) |
22 | 47 | } | 49 | } |
23 | 48 | 50 | ||
24 | 49 | // NotFoundError represents an error indicating that the requested data wasn't found. | ||
25 | 50 | type NotFoundError struct { | ||
26 | 51 | msg string | ||
27 | 52 | } | ||
28 | 53 | |||
29 | 54 | func (e *NotFoundError) Error() string { | ||
30 | 55 | return e.msg | ||
31 | 56 | } | ||
32 | 57 | |||
33 | 58 | // CharmStore is a Repository that provides access to the public juju charm store. | 51 | // CharmStore is a Repository that provides access to the public juju charm store. |
34 | 59 | type CharmStore struct { | 52 | type CharmStore struct { |
35 | 60 | BaseURL string | 53 | BaseURL string |
36 | @@ -83,7 +76,7 @@ | |||
37 | 83 | return nil, fmt.Errorf("charm: charm store returned response without charm %q", key) | 76 | return nil, fmt.Errorf("charm: charm store returned response without charm %q", key) |
38 | 84 | } | 77 | } |
39 | 85 | if len(info.Errors) == 1 && info.Errors[0] == "entry not found" { | 78 | if len(info.Errors) == 1 && info.Errors[0] == "entry not found" { |
41 | 86 | return nil, &NotFoundError{fmt.Sprintf("charm not found: %s", curl)} | 79 | return nil, errors.NotFoundf("charm %q", curl) |
42 | 87 | } | 80 | } |
43 | 88 | return info, nil | 81 | return info, nil |
44 | 89 | } | 82 | } |
45 | @@ -116,9 +109,9 @@ | |||
46 | 116 | } | 109 | } |
47 | 117 | if len(event.Errors) == 1 && event.Errors[0] == "entry not found" { | 110 | if len(event.Errors) == 1 && event.Errors[0] == "entry not found" { |
48 | 118 | if digest == "" { | 111 | if digest == "" { |
50 | 119 | return nil, &NotFoundError{fmt.Sprintf("charm event not found for %q", curl)} | 112 | return nil, errors.NotFoundf("charm event for %q", curl) |
51 | 120 | } else { | 113 | } else { |
53 | 121 | return nil, &NotFoundError{fmt.Sprintf("charm event not found for %q with digest %q", curl, digest)} | 114 | return nil, errors.NotFoundf("charm event for %q with digest %q", curl, digest) |
54 | 122 | } | 115 | } |
55 | 123 | } | 116 | } |
56 | 124 | return event, nil | 117 | return event, nil |
57 | @@ -288,11 +281,11 @@ | |||
58 | 288 | } | 281 | } |
59 | 289 | 282 | ||
60 | 290 | func repoNotFound(path string) error { | 283 | func repoNotFound(path string) error { |
62 | 291 | return &NotFoundError{fmt.Sprintf("no repository found at %q", path)} | 284 | return errors.NotFoundf("repository %q", path) |
63 | 292 | } | 285 | } |
64 | 293 | 286 | ||
65 | 294 | func charmNotFound(curl *URL, repoPath string) error { | 287 | func charmNotFound(curl *URL, repoPath string) error { |
67 | 295 | return &NotFoundError{fmt.Sprintf("charm not found in %q: %s", repoPath, curl)} | 288 | return errors.RawNotFoundf("charm %q not found in repository %q", curl, repoPath) |
68 | 296 | } | 289 | } |
69 | 297 | 290 | ||
70 | 298 | func mightBeCharm(info os.FileInfo) bool { | 291 | func mightBeCharm(info os.FileInfo) bool { |
71 | 299 | 292 | ||
72 | === modified file 'charm/repo_test.go' | |||
73 | --- charm/repo_test.go 2013-06-11 23:32:05 +0000 | |||
74 | +++ charm/repo_test.go 2013-06-12 12:32:25 +0000 | |||
75 | @@ -9,15 +9,17 @@ | |||
76 | 9 | "encoding/json" | 9 | "encoding/json" |
77 | 10 | "fmt" | 10 | "fmt" |
78 | 11 | "io/ioutil" | 11 | "io/ioutil" |
79 | 12 | . "launchpad.net/gocheck" | ||
80 | 13 | "launchpad.net/juju-core/charm" | ||
81 | 14 | "launchpad.net/juju-core/testing" | ||
82 | 15 | "net" | 12 | "net" |
83 | 16 | "net/http" | 13 | "net/http" |
84 | 17 | "os" | 14 | "os" |
85 | 18 | "path/filepath" | 15 | "path/filepath" |
86 | 19 | "strconv" | 16 | "strconv" |
87 | 20 | "strings" | 17 | "strings" |
88 | 18 | |||
89 | 19 | . "launchpad.net/gocheck" | ||
90 | 20 | "launchpad.net/juju-core/charm" | ||
91 | 21 | "launchpad.net/juju-core/errors" | ||
92 | 22 | "launchpad.net/juju-core/testing" | ||
93 | 21 | ) | 23 | ) |
94 | 22 | 24 | ||
95 | 23 | type MockStore struct { | 25 | type MockStore struct { |
96 | @@ -180,11 +182,13 @@ | |||
97 | 180 | 182 | ||
98 | 181 | func (s *StoreSuite) TestMissing(c *C) { | 183 | func (s *StoreSuite) TestMissing(c *C) { |
99 | 182 | charmURL := charm.MustParseURL("cs:series/missing") | 184 | charmURL := charm.MustParseURL("cs:series/missing") |
101 | 183 | expect := `charm not found: cs:series/missing` | 185 | expect := `charm "cs:series/missing" not found` |
102 | 184 | _, err := s.store.Latest(charmURL) | 186 | _, err := s.store.Latest(charmURL) |
103 | 185 | c.Assert(err, ErrorMatches, expect) | 187 | c.Assert(err, ErrorMatches, expect) |
104 | 188 | c.Assert(errors.IsNotFoundError(err), Equals, true) | ||
105 | 186 | _, err = s.store.Get(charmURL) | 189 | _, err = s.store.Get(charmURL) |
106 | 187 | c.Assert(err, ErrorMatches, expect) | 190 | c.Assert(err, ErrorMatches, expect) |
107 | 191 | c.Assert(errors.IsNotFoundError(err), Equals, true) | ||
108 | 188 | } | 192 | } |
109 | 189 | 193 | ||
110 | 190 | func (s *StoreSuite) TestError(c *C) { | 194 | func (s *StoreSuite) TestError(c *C) { |
111 | @@ -280,7 +284,8 @@ | |||
112 | 280 | func (s *StoreSuite) TestInfoNotFound(c *C) { | 284 | func (s *StoreSuite) TestInfoNotFound(c *C) { |
113 | 281 | charmURL := charm.MustParseURL("cs:series/missing") | 285 | charmURL := charm.MustParseURL("cs:series/missing") |
114 | 282 | info, err := s.store.Info(charmURL) | 286 | info, err := s.store.Info(charmURL) |
116 | 283 | c.Assert(err, ErrorMatches, `charm not found: cs:series/missing`) | 287 | c.Assert(err, ErrorMatches, `charm "cs:series/missing" not found`) |
117 | 288 | c.Assert(errors.IsNotFoundError(err), Equals, true) | ||
118 | 284 | c.Assert(info, IsNil) | 289 | c.Assert(info, IsNil) |
119 | 285 | } | 290 | } |
120 | 286 | 291 | ||
121 | @@ -319,14 +324,16 @@ | |||
122 | 319 | func (s *StoreSuite) TestEventNotFound(c *C) { | 324 | func (s *StoreSuite) TestEventNotFound(c *C) { |
123 | 320 | charmURL := charm.MustParseURL("cs:series/missing") | 325 | charmURL := charm.MustParseURL("cs:series/missing") |
124 | 321 | event, err := s.store.Event(charmURL, "") | 326 | event, err := s.store.Event(charmURL, "") |
126 | 322 | c.Assert(err, ErrorMatches, `charm event not found for "cs:series/missing"`) | 327 | c.Assert(err, ErrorMatches, `charm event for "cs:series/missing" not found`) |
127 | 328 | c.Assert(errors.IsNotFoundError(err), Equals, true) | ||
128 | 323 | c.Assert(event, IsNil) | 329 | c.Assert(event, IsNil) |
129 | 324 | } | 330 | } |
130 | 325 | 331 | ||
131 | 326 | func (s *StoreSuite) TestEventNotFoundDigest(c *C) { | 332 | func (s *StoreSuite) TestEventNotFoundDigest(c *C) { |
132 | 327 | charmURL := charm.MustParseURL("cs:series/good") | 333 | charmURL := charm.MustParseURL("cs:series/good") |
133 | 328 | event, err := s.store.Event(charmURL, "missing-digest") | 334 | event, err := s.store.Event(charmURL, "missing-digest") |
135 | 329 | c.Assert(err, ErrorMatches, `charm event not found for "cs:series/good" with digest "missing-digest"`) | 335 | c.Assert(err, ErrorMatches, `charm event for "cs:series/good" with digest "missing-digest" not found`) |
136 | 336 | c.Assert(errors.IsNotFoundError(err), Equals, true) | ||
137 | 330 | c.Assert(event, IsNil) | 337 | c.Assert(event, IsNil) |
138 | 331 | } | 338 | } |
139 | 332 | 339 | ||
140 | @@ -414,7 +421,7 @@ | |||
141 | 414 | } | 421 | } |
142 | 415 | 422 | ||
143 | 416 | func (s *LocalRepoSuite) checkNotFoundErr(c *C, err error, charmURL *charm.URL) { | 423 | func (s *LocalRepoSuite) checkNotFoundErr(c *C, err error, charmURL *charm.URL) { |
145 | 417 | expect := `charm not found in "` + s.repo.Path + `": ` + charmURL.String() | 424 | expect := fmt.Sprintf("charm %q not found in repository %q", charmURL, s.repo.Path) |
146 | 418 | c.Check(err, ErrorMatches, expect) | 425 | c.Check(err, ErrorMatches, expect) |
147 | 419 | } | 426 | } |
148 | 420 | 427 | ||
149 | @@ -432,16 +439,18 @@ | |||
150 | 432 | } | 439 | } |
151 | 433 | 440 | ||
152 | 434 | func (s *LocalRepoSuite) TestMissingRepo(c *C) { | 441 | func (s *LocalRepoSuite) TestMissingRepo(c *C) { |
153 | 442 | curl := charm.MustParseURL("local:series/zebra") | ||
154 | 443 | expect := fmt.Sprintf("repository %q not found", s.repo.Path) | ||
155 | 444 | checkNotFound := func() { | ||
156 | 445 | _, err := s.repo.Latest(curl) | ||
157 | 446 | c.Check(err, ErrorMatches, expect) | ||
158 | 447 | _, err = s.repo.Get(curl) | ||
159 | 448 | c.Check(err, ErrorMatches, expect) | ||
160 | 449 | } | ||
161 | 435 | c.Assert(os.RemoveAll(s.repo.Path), IsNil) | 450 | c.Assert(os.RemoveAll(s.repo.Path), IsNil) |
166 | 436 | _, err := s.repo.Latest(charm.MustParseURL("local:series/zebra")) | 451 | checkNotFound() |
163 | 437 | c.Assert(err, ErrorMatches, `no repository found at ".*"`) | ||
164 | 438 | _, err = s.repo.Get(charm.MustParseURL("local:series/zebra")) | ||
165 | 439 | c.Assert(err, ErrorMatches, `no repository found at ".*"`) | ||
167 | 440 | c.Assert(ioutil.WriteFile(s.repo.Path, nil, 0666), IsNil) | 452 | c.Assert(ioutil.WriteFile(s.repo.Path, nil, 0666), IsNil) |
172 | 441 | _, err = s.repo.Latest(charm.MustParseURL("local:series/zebra")) | 453 | checkNotFound() |
169 | 442 | c.Assert(err, ErrorMatches, `no repository found at ".*"`) | ||
170 | 443 | _, err = s.repo.Get(charm.MustParseURL("local:series/zebra")) | ||
171 | 444 | c.Assert(err, ErrorMatches, `no repository found at ".*"`) | ||
173 | 445 | } | 454 | } |
174 | 446 | 455 | ||
175 | 447 | func (s *LocalRepoSuite) TestMultipleVersions(c *C) { | 456 | func (s *LocalRepoSuite) TestMultipleVersions(c *C) { |
176 | 448 | 457 | ||
177 | === modified file 'cmd/juju/deploy_test.go' | |||
178 | --- cmd/juju/deploy_test.go 2013-06-12 11:25:32 +0000 | |||
179 | +++ cmd/juju/deploy_test.go 2013-06-12 12:32:25 +0000 | |||
180 | @@ -62,7 +62,7 @@ | |||
181 | 62 | 62 | ||
182 | 63 | func (s *DeploySuite) TestNoCharm(c *C) { | 63 | func (s *DeploySuite) TestNoCharm(c *C) { |
183 | 64 | err := runDeploy(c, "local:unknown-123") | 64 | err := runDeploy(c, "local:unknown-123") |
185 | 65 | c.Assert(err, ErrorMatches, `cannot get charm: charm not found in ".*": local:precise/unknown-123`) | 65 | c.Assert(err, ErrorMatches, `cannot get charm: charm "local:precise/unknown-123" not found in repository ".*"`) |
186 | 66 | } | 66 | } |
187 | 67 | 67 | ||
188 | 68 | func (s *DeploySuite) TestCharmDir(c *C) { | 68 | func (s *DeploySuite) TestCharmDir(c *C) { |
189 | 69 | 69 | ||
190 | === modified file 'cmd/juju/publish.go' | |||
191 | --- cmd/juju/publish.go 2013-05-02 15:55:42 +0000 | |||
192 | +++ cmd/juju/publish.go 2013-06-12 12:32:25 +0000 | |||
193 | @@ -5,14 +5,16 @@ | |||
194 | 5 | 5 | ||
195 | 6 | import ( | 6 | import ( |
196 | 7 | "fmt" | 7 | "fmt" |
197 | 8 | "os" | ||
198 | 9 | "strings" | ||
199 | 10 | "time" | ||
200 | 11 | |||
201 | 8 | "launchpad.net/gnuflag" | 12 | "launchpad.net/gnuflag" |
202 | 9 | "launchpad.net/juju-core/bzr" | 13 | "launchpad.net/juju-core/bzr" |
203 | 10 | "launchpad.net/juju-core/charm" | 14 | "launchpad.net/juju-core/charm" |
204 | 11 | "launchpad.net/juju-core/cmd" | 15 | "launchpad.net/juju-core/cmd" |
205 | 16 | "launchpad.net/juju-core/errors" | ||
206 | 12 | "launchpad.net/juju-core/log" | 17 | "launchpad.net/juju-core/log" |
207 | 13 | "os" | ||
208 | 14 | "strings" | ||
209 | 15 | "time" | ||
210 | 16 | ) | 18 | ) |
211 | 17 | 19 | ||
212 | 18 | type PublishCommand struct { | 20 | type PublishCommand struct { |
213 | @@ -131,9 +133,9 @@ | |||
214 | 131 | } | 133 | } |
215 | 132 | 134 | ||
216 | 133 | oldEvent, err := charm.Store.Event(curl, localDigest) | 135 | oldEvent, err := charm.Store.Event(curl, localDigest) |
218 | 134 | if _, ok := err.(*charm.NotFoundError); ok { | 136 | if errors.IsNotFoundError(err) { |
219 | 135 | oldEvent, err = charm.Store.Event(curl, "") | 137 | oldEvent, err = charm.Store.Event(curl, "") |
221 | 136 | if _, ok := err.(*charm.NotFoundError); ok { | 138 | if errors.IsNotFoundError(err) { |
222 | 137 | log.Infof("charm %s is not yet in the store", curl) | 139 | log.Infof("charm %s is not yet in the store", curl) |
223 | 138 | err = nil | 140 | err = nil |
224 | 139 | } | 141 | } |
225 | @@ -156,7 +158,7 @@ | |||
226 | 156 | for { | 158 | for { |
227 | 157 | time.Sleep(c.pollDelay) | 159 | time.Sleep(c.pollDelay) |
228 | 158 | newEvent, err := charm.Store.Event(curl, "") | 160 | newEvent, err := charm.Store.Event(curl, "") |
230 | 159 | if _, ok := err.(*charm.NotFoundError); ok { | 161 | if errors.IsNotFoundError(err) { |
231 | 160 | continue | 162 | continue |
232 | 161 | } | 163 | } |
233 | 162 | if err != nil { | 164 | if err != nil { |
234 | 163 | 165 | ||
235 | === modified file 'cmd/juju/upgradecharm_test.go' | |||
236 | --- cmd/juju/upgradecharm_test.go 2013-06-10 20:50:42 +0000 | |||
237 | +++ cmd/juju/upgradecharm_test.go 2013-06-12 12:32:25 +0000 | |||
238 | @@ -41,12 +41,12 @@ | |||
239 | 41 | c.Assert(err, IsNil) | 41 | c.Assert(err, IsNil) |
240 | 42 | 42 | ||
241 | 43 | err = runUpgradeCharm(c, "riak", "--repository=blah") | 43 | err = runUpgradeCharm(c, "riak", "--repository=blah") |
243 | 44 | c.Assert(err, ErrorMatches, `no repository found at ".*blah"`) | 44 | c.Assert(err, ErrorMatches, `repository ".*blah" not found`) |
244 | 45 | // Reset JUJU_REPOSITORY explicitly, because repoSuite.SetUpTest | 45 | // Reset JUJU_REPOSITORY explicitly, because repoSuite.SetUpTest |
245 | 46 | // overwrites it (TearDownTest will revert it again). | 46 | // overwrites it (TearDownTest will revert it again). |
246 | 47 | os.Setenv("JUJU_REPOSITORY", "") | 47 | os.Setenv("JUJU_REPOSITORY", "") |
247 | 48 | err = runUpgradeCharm(c, "riak", "--repository=") | 48 | err = runUpgradeCharm(c, "riak", "--repository=") |
249 | 49 | c.Assert(err, ErrorMatches, `charm not found in ".*": local:precise/riak`) | 49 | c.Assert(err, ErrorMatches, `charm "local:precise/riak" not found in repository ".*"`) |
250 | 50 | } | 50 | } |
251 | 51 | 51 | ||
252 | 52 | func (s *UpgradeCharmErrorsSuite) TestInvalidService(c *C) { | 52 | func (s *UpgradeCharmErrorsSuite) TestInvalidService(c *C) { |
253 | @@ -70,11 +70,12 @@ | |||
254 | 70 | 70 | ||
255 | 71 | func (s *UpgradeCharmErrorsSuite) TestInvalidSwitchURL(c *C) { | 71 | func (s *UpgradeCharmErrorsSuite) TestInvalidSwitchURL(c *C) { |
256 | 72 | s.deployService(c) | 72 | s.deployService(c) |
261 | 73 | err := runUpgradeCharm(c, "riak", "--switch=blah") | 73 | err := runUpgradeCharm(c, "riak", "--switch=local:blah") |
262 | 74 | c.Assert(err, ErrorMatches, "charm not found: cs:precise/blah") | 74 | c.Assert(err, ErrorMatches, `charm "local:precise/blah" not found in repository ".*"`) |
263 | 75 | err = runUpgradeCharm(c, "riak", "--switch=cs:missing/one") | 75 | err = runUpgradeCharm(c, "riak", "--switch=local:missing/one") |
264 | 76 | c.Assert(err, ErrorMatches, "charm not found: cs:missing/one") | 76 | c.Assert(err, ErrorMatches, `charm "local:missing/one" not found in repository ".*"`) |
265 | 77 | // TODO(dimitern): add tests with incompatible charms | 77 | // TODO(dimitern): add tests with incompatible charms |
266 | 78 | // TODO(fwereade): hook up forthcoming testing.MockCharmStore for cs: tests | ||
267 | 78 | } | 79 | } |
268 | 79 | 80 | ||
269 | 80 | func (s *UpgradeCharmErrorsSuite) TestSwitchAndRevisionFails(c *C) { | 81 | func (s *UpgradeCharmErrorsSuite) TestSwitchAndRevisionFails(c *C) { |
270 | 81 | 82 | ||
271 | === modified file 'cmd/jujud/agent_test.go' | |||
272 | --- cmd/jujud/agent_test.go 2013-05-31 08:33:34 +0000 | |||
273 | +++ cmd/jujud/agent_test.go 2013-06-12 12:32:25 +0000 | |||
274 | @@ -6,21 +6,22 @@ | |||
275 | 6 | import ( | 6 | import ( |
276 | 7 | "bytes" | 7 | "bytes" |
277 | 8 | "fmt" | 8 | "fmt" |
278 | 9 | "net/http" | ||
279 | 10 | "os" | ||
280 | 11 | "path/filepath" | ||
281 | 12 | "time" | ||
282 | 13 | |||
283 | 9 | . "launchpad.net/gocheck" | 14 | . "launchpad.net/gocheck" |
284 | 10 | "launchpad.net/juju-core/cmd" | 15 | "launchpad.net/juju-core/cmd" |
285 | 11 | "launchpad.net/juju-core/environs/agent" | 16 | "launchpad.net/juju-core/environs/agent" |
286 | 12 | "launchpad.net/juju-core/environs/tools" | 17 | "launchpad.net/juju-core/environs/tools" |
287 | 13 | "launchpad.net/juju-core/errors" | ||
288 | 14 | "launchpad.net/juju-core/juju/testing" | ||
289 | 15 | "launchpad.net/juju-core/state" | 18 | "launchpad.net/juju-core/state" |
290 | 16 | coretesting "launchpad.net/juju-core/testing" | ||
291 | 17 | "launchpad.net/juju-core/version" | 19 | "launchpad.net/juju-core/version" |
292 | 18 | "launchpad.net/juju-core/worker" | 20 | "launchpad.net/juju-core/worker" |
293 | 19 | "launchpad.net/tomb" | 21 | "launchpad.net/tomb" |
298 | 20 | "net/http" | 22 | |
299 | 21 | "os" | 23 | "launchpad.net/juju-core/juju/testing" |
300 | 22 | "path/filepath" | 24 | coretesting "launchpad.net/juju-core/testing" |
297 | 23 | "time" | ||
301 | 24 | ) | 25 | ) |
302 | 25 | 26 | ||
303 | 26 | var _ = Suite(&toolSuite{}) | 27 | var _ = Suite(&toolSuite{}) |
304 | @@ -348,13 +349,13 @@ | |||
305 | 348 | info := s.StateInfo(c) | 349 | info := s.StateInfo(c) |
306 | 349 | info.Tag = ent.Tag() | 350 | info.Tag = ent.Tag() |
307 | 350 | info.Password = "initial" | 351 | info.Password = "initial" |
309 | 351 | testOpenState(c, info, errors.Unauthorizedf("unauth")) | 352 | assertOpenState(c, info, false) |
310 | 352 | 353 | ||
311 | 353 | // Read the configuration and check that we can connect with it. | 354 | // Read the configuration and check that we can connect with it. |
312 | 354 | c.Assert(refreshConfig(conf), IsNil) | 355 | c.Assert(refreshConfig(conf), IsNil) |
313 | 355 | newPassword := conf.StateInfo.Password | 356 | newPassword := conf.StateInfo.Password |
314 | 356 | 357 | ||
316 | 357 | testOpenState(c, conf.StateInfo, nil) | 358 | assertOpenState(c, conf.StateInfo, true) |
317 | 358 | 359 | ||
318 | 359 | // Check that it starts again ok | 360 | // Check that it starts again ok |
319 | 360 | err = runStop(newAgent()) | 361 | err = runStop(newAgent()) |
320 | @@ -376,7 +377,7 @@ | |||
321 | 376 | c.Assert(conf.StateInfo.Password, Not(Equals), newPassword) | 377 | c.Assert(conf.StateInfo.Password, Not(Equals), newPassword) |
322 | 377 | 378 | ||
323 | 378 | info.Password = conf.StateInfo.Password | 379 | info.Password = conf.StateInfo.Password |
325 | 379 | testOpenState(c, info, nil) | 380 | assertOpenState(c, info, true) |
326 | 380 | } | 381 | } |
327 | 381 | 382 | ||
328 | 382 | func (s *agentSuite) testUpgrade(c *C, agent runner, currentTools *state.Tools) { | 383 | func (s *agentSuite) testUpgrade(c *C, agent runner, currentTools *state.Tools) { |
329 | 383 | 384 | ||
330 | === modified file 'cmd/jujud/bootstrap_test.go' | |||
331 | --- cmd/jujud/bootstrap_test.go 2013-05-30 00:47:30 +0000 | |||
332 | +++ cmd/jujud/bootstrap_test.go 2013-06-12 12:32:25 +0000 | |||
333 | @@ -134,15 +134,15 @@ | |||
334 | 134 | }) | 134 | }) |
335 | 135 | } | 135 | } |
336 | 136 | 136 | ||
338 | 137 | func testOpenState(c *C, info *state.Info, expectErr error) { | 137 | func assertOpenState(c *C, info *state.Info, expectSuccess bool) { |
339 | 138 | st, err := state.Open(info, state.DefaultDialOpts()) | 138 | st, err := state.Open(info, state.DefaultDialOpts()) |
340 | 139 | if st != nil { | 139 | if st != nil { |
341 | 140 | st.Close() | 140 | st.Close() |
342 | 141 | } | 141 | } |
344 | 142 | if expectErr != nil { | 142 | if expectSuccess { |
345 | 143 | c.Assert(err, IsNil) | ||
346 | 144 | } else { | ||
347 | 143 | c.Assert(errors.IsUnauthorizedError(err), Equals, true) | 145 | c.Assert(errors.IsUnauthorizedError(err), Equals, true) |
348 | 144 | } else { | ||
349 | 145 | c.Assert(err, IsNil) | ||
350 | 146 | } | 146 | } |
351 | 147 | } | 147 | } |
352 | 148 | 148 | ||
353 | @@ -162,10 +162,10 @@ | |||
354 | 162 | Addrs: []string{testing.MgoAddr}, | 162 | Addrs: []string{testing.MgoAddr}, |
355 | 163 | CACert: []byte(testing.CACert), | 163 | CACert: []byte(testing.CACert), |
356 | 164 | } | 164 | } |
358 | 165 | testOpenState(c, info, errors.Unauthorizedf("some auth problem")) | 165 | assertOpenState(c, info, false) |
359 | 166 | 166 | ||
360 | 167 | info.Tag, info.Password = "machine-0", "foo" | 167 | info.Tag, info.Password = "machine-0", "foo" |
362 | 168 | testOpenState(c, info, nil) | 168 | assertOpenState(c, info, true) |
363 | 169 | 169 | ||
364 | 170 | info.Tag = "" | 170 | info.Tag = "" |
365 | 171 | st, err := state.Open(info, state.DefaultDialOpts()) | 171 | st, err := state.Open(info, state.DefaultDialOpts()) |
366 | 172 | 172 | ||
367 | === modified file 'environs/dummy/storage.go' | |||
368 | --- environs/dummy/storage.go 2013-05-30 00:47:30 +0000 | |||
369 | +++ environs/dummy/storage.go 2013-06-12 12:32:25 +0000 | |||
370 | @@ -82,7 +82,7 @@ | |||
371 | 82 | } | 82 | } |
372 | 83 | data, ok := s.files[path] | 83 | data, ok := s.files[path] |
373 | 84 | if !ok { | 84 | if !ok { |
375 | 85 | return nil, errors.NotFoundf("file %q not found", path) | 85 | return nil, errors.NotFoundf("file %q", path) |
376 | 86 | } | 86 | } |
377 | 87 | return data, nil | 87 | return data, nil |
378 | 88 | } | 88 | } |
379 | 89 | 89 | ||
380 | === modified file 'environs/ec2/storage.go' | |||
381 | --- environs/ec2/storage.go 2013-05-30 00:47:30 +0000 | |||
382 | +++ environs/ec2/storage.go 2013-06-12 12:32:25 +0000 | |||
383 | @@ -160,7 +160,7 @@ | |||
384 | 160 | 160 | ||
385 | 161 | func maybeNotFound(err error) error { | 161 | func maybeNotFound(err error) error { |
386 | 162 | if err != nil && s3ErrorStatusCode(err) == 404 { | 162 | if err != nil && s3ErrorStatusCode(err) == 404 { |
388 | 163 | return &errors.NotFoundError{err, ""} | 163 | return errors.RawNotFoundf("%v", err) |
389 | 164 | } | 164 | } |
390 | 165 | return err | 165 | return err |
391 | 166 | } | 166 | } |
392 | 167 | 167 | ||
393 | === modified file 'environs/imagemetadata/simplestreams.go' | |||
394 | --- environs/imagemetadata/simplestreams.go 2013-05-30 05:39:33 +0000 | |||
395 | +++ environs/imagemetadata/simplestreams.go 2013-06-12 12:32:25 +0000 | |||
396 | @@ -245,14 +245,14 @@ | |||
397 | 245 | dataURL += path | 245 | dataURL += path |
398 | 246 | resp, err := http.Get(dataURL) | 246 | resp, err := http.Get(dataURL) |
399 | 247 | if err != nil { | 247 | if err != nil { |
401 | 248 | return nil, dataURL, errors.NotFoundf("invalid URL %q", dataURL) | 248 | return nil, dataURL, errors.RawNotFoundf("invalid URL %q", dataURL) |
402 | 249 | } | 249 | } |
403 | 250 | defer resp.Body.Close() | 250 | defer resp.Body.Close() |
404 | 251 | if resp.StatusCode == http.StatusNotFound { | 251 | if resp.StatusCode == http.StatusNotFound { |
406 | 252 | return nil, dataURL, errors.NotFoundf("cannot find URL %q", dataURL) | 252 | return nil, dataURL, errors.NotFoundf("URL %q", dataURL) |
407 | 253 | } | 253 | } |
408 | 254 | if resp.StatusCode == http.StatusUnauthorized { | 254 | if resp.StatusCode == http.StatusUnauthorized { |
410 | 255 | return nil, dataURL, errors.Unauthorizedf("unauthorised access to URL %q", dataURL) | 255 | return nil, dataURL, errors.Unauthorizedf("access to URL %q", dataURL) |
411 | 256 | } | 256 | } |
412 | 257 | if resp.StatusCode != http.StatusOK { | 257 | if resp.StatusCode != http.StatusOK { |
413 | 258 | return nil, dataURL, fmt.Errorf("cannot access URL %q, %q", dataURL, resp.Status) | 258 | return nil, dataURL, fmt.Errorf("cannot access URL %q, %q", dataURL, resp.Status) |
414 | @@ -322,9 +322,9 @@ | |||
415 | 322 | } | 322 | } |
416 | 323 | } | 323 | } |
417 | 324 | if !containsImageIds { | 324 | if !containsImageIds { |
419 | 325 | return "", errors.NotFoundf("index file missing %q data", imageIds) | 325 | return "", errors.RawNotFoundf("index file missing %q data", imageIds) |
420 | 326 | } | 326 | } |
422 | 327 | return "", errors.NotFoundf("index file missing data for cloud %v and product name(s) %q", ic.CloudSpec, prodIds) | 327 | return "", errors.RawNotFoundf("index file missing data for cloud %v and product name(s) %q", ic.CloudSpec, prodIds) |
423 | 328 | } | 328 | } |
424 | 329 | 329 | ||
425 | 330 | // utility function to see if element exists in values slice. | 330 | // utility function to see if element exists in values slice. |
426 | 331 | 331 | ||
427 | === modified file 'environs/local/storage.go' | |||
428 | --- environs/local/storage.go 2013-05-30 00:47:30 +0000 | |||
429 | +++ environs/local/storage.go 2013-06-12 12:32:25 +0000 | |||
430 | @@ -42,7 +42,7 @@ | |||
431 | 42 | return nil, err | 42 | return nil, err |
432 | 43 | } | 43 | } |
433 | 44 | if resp.StatusCode != 200 { | 44 | if resp.StatusCode != 200 { |
435 | 45 | return nil, errors.NotFoundf("file %q not found", name) | 45 | return nil, errors.NotFoundf("file %q", name) |
436 | 46 | } | 46 | } |
437 | 47 | return resp.Body, nil | 47 | return resp.Body, nil |
438 | 48 | } | 48 | } |
439 | 49 | 49 | ||
440 | === modified file 'environs/maas/storage.go' | |||
441 | --- environs/maas/storage.go 2013-05-30 00:47:30 +0000 | |||
442 | +++ environs/maas/storage.go 2013-06-12 12:32:25 +0000 | |||
443 | @@ -77,9 +77,9 @@ | |||
444 | 77 | noObj := gomaasapi.MAASObject{} | 77 | noObj := gomaasapi.MAASObject{} |
445 | 78 | serverErr, ok := err.(gomaasapi.ServerError) | 78 | serverErr, ok := err.(gomaasapi.ServerError) |
446 | 79 | if ok && serverErr.StatusCode == 404 { | 79 | if ok && serverErr.StatusCode == 404 { |
448 | 80 | return noObj, errors.NotFoundf("file '%s' not found", name) | 80 | return noObj, errors.NotFoundf("file %q", name) |
449 | 81 | } | 81 | } |
451 | 82 | msg := fmt.Errorf("could not access file '%s': %v", name, err) | 82 | msg := fmt.Errorf("could not access file %q: %v", name, err) |
452 | 83 | return noObj, msg | 83 | return noObj, msg |
453 | 84 | } | 84 | } |
454 | 85 | return obj, nil | 85 | return obj, nil |
455 | 86 | 86 | ||
456 | === modified file 'environs/openstack/storage.go' | |||
457 | --- environs/openstack/storage.go 2013-05-31 01:02:53 +0000 | |||
458 | +++ environs/openstack/storage.go 2013-06-12 12:32:25 +0000 | |||
459 | @@ -160,7 +160,7 @@ | |||
460 | 160 | // container not being found. | 160 | // container not being found. |
461 | 161 | func maybeNotFound(err error) (error, bool) { | 161 | func maybeNotFound(err error) (error, bool) { |
462 | 162 | if err != nil && gooseerrors.IsNotFound(err) { | 162 | if err != nil && gooseerrors.IsNotFound(err) { |
464 | 163 | return &coreerrors.NotFoundError{err, ""}, true | 163 | return coreerrors.RawNotFoundf("%v", err), true |
465 | 164 | } | 164 | } |
466 | 165 | return err, false | 165 | return err, false |
467 | 166 | } | 166 | } |
468 | 167 | 167 | ||
469 | === modified file 'environs/tools.go' | |||
470 | --- environs/tools.go 2013-05-30 00:47:30 +0000 | |||
471 | +++ environs/tools.go 2013-06-12 12:32:25 +0000 | |||
472 | @@ -147,6 +147,6 @@ | |||
473 | 147 | 147 | ||
474 | 148 | func convertToolsError(err *error) { | 148 | func convertToolsError(err *error) { |
475 | 149 | if isToolsError(*err) { | 149 | if isToolsError(*err) { |
477 | 150 | *err = &errors.NotFoundError{*err, ""} | 150 | *err = errors.RawNotFoundf("%v", *err) |
478 | 151 | } | 151 | } |
479 | 152 | } | 152 | } |
480 | 153 | 153 | ||
481 | === modified file 'environs/tools_test.go' | |||
482 | --- environs/tools_test.go 2013-05-30 00:47:30 +0000 | |||
483 | +++ environs/tools_test.go 2013-06-12 12:32:25 +0000 | |||
484 | @@ -168,7 +168,7 @@ | |||
485 | 168 | if len(actual) > 0 { | 168 | if len(actual) > 0 { |
486 | 169 | c.Logf(actual.String()) | 169 | c.Logf(actual.String()) |
487 | 170 | } | 170 | } |
489 | 171 | c.Check(err, DeepEquals, &errors.NotFoundError{test.err, ""}) | 171 | c.Check(err, DeepEquals, &errors.NotFoundError{test.err.Error()}) |
490 | 172 | continue | 172 | continue |
491 | 173 | } | 173 | } |
492 | 174 | source := private | 174 | source := private |
493 | @@ -389,7 +389,7 @@ | |||
494 | 389 | if len(actual) > 0 { | 389 | if len(actual) > 0 { |
495 | 390 | c.Logf(actual.String()) | 390 | c.Logf(actual.String()) |
496 | 391 | } | 391 | } |
498 | 392 | c.Check(err, DeepEquals, &errors.NotFoundError{test.err, ""}) | 392 | c.Check(err, DeepEquals, &errors.NotFoundError{test.err.Error()}) |
499 | 393 | continue | 393 | continue |
500 | 394 | } | 394 | } |
501 | 395 | expect := map[version.Binary]string{} | 395 | expect := map[version.Binary]string{} |
502 | @@ -484,7 +484,7 @@ | |||
503 | 484 | if len(actual) > 0 { | 484 | if len(actual) > 0 { |
504 | 485 | c.Logf(actual.String()) | 485 | c.Logf(actual.String()) |
505 | 486 | } | 486 | } |
507 | 487 | c.Check(err, DeepEquals, &errors.NotFoundError{test.err, ""}) | 487 | c.Check(err, DeepEquals, &errors.NotFoundError{test.err.Error()}) |
508 | 488 | continue | 488 | continue |
509 | 489 | } | 489 | } |
510 | 490 | expect := map[version.Binary]string{} | 490 | expect := map[version.Binary]string{} |
511 | @@ -548,7 +548,7 @@ | |||
512 | 548 | } | 548 | } |
513 | 549 | c.Check(actual.URL, DeepEquals, source[actual.Binary]) | 549 | c.Check(actual.URL, DeepEquals, source[actual.Binary]) |
514 | 550 | } else { | 550 | } else { |
516 | 551 | c.Check(err, DeepEquals, &errors.NotFoundError{test.err, ""}) | 551 | c.Check(err, DeepEquals, &errors.NotFoundError{test.err.Error()}) |
517 | 552 | } | 552 | } |
518 | 553 | } | 553 | } |
519 | 554 | } | 554 | } |
520 | 555 | 555 | ||
521 | === modified file 'errors/errors.go' | |||
522 | --- errors/errors.go 2013-05-30 00:47:30 +0000 | |||
523 | +++ errors/errors.go 2013-06-12 12:32:25 +0000 | |||
524 | @@ -7,60 +7,62 @@ | |||
525 | 7 | "fmt" | 7 | "fmt" |
526 | 8 | ) | 8 | ) |
527 | 9 | 9 | ||
529 | 10 | // NotFoundError records an error when something has not been found. | 10 | // NotFoundError indicates that something has not been found. |
530 | 11 | type NotFoundError struct { | 11 | type NotFoundError struct { |
531 | 12 | // error is the underlying error. | ||
532 | 13 | error | ||
533 | 14 | Msg string | 12 | Msg string |
534 | 15 | } | 13 | } |
535 | 16 | 14 | ||
537 | 17 | // IsNotFoundError returns true if err is a NotFoundError. | 15 | func (e *NotFoundError) Error() string { |
538 | 16 | if e.Msg == "" { | ||
539 | 17 | return "<something> not found" | ||
540 | 18 | } | ||
541 | 19 | return e.Msg | ||
542 | 20 | } | ||
543 | 21 | |||
544 | 22 | // IsNotFoundError returns true if err is a *NotFoundError. | ||
545 | 18 | func IsNotFoundError(err error) bool { | 23 | func IsNotFoundError(err error) bool { |
566 | 19 | if _, ok := err.(*NotFoundError); ok { | 24 | _, ok := err.(*NotFoundError) |
567 | 20 | return true | 25 | return ok |
568 | 21 | } | 26 | } |
569 | 22 | return false | 27 | |
570 | 23 | } | 28 | // NotFoundf("foo %s", "bar") returns a *NotFoundError with the message |
571 | 24 | 29 | // "foo bar not found". | |
552 | 25 | func (e *NotFoundError) Error() string { | ||
553 | 26 | if e.Msg != "" || e.error == nil { | ||
554 | 27 | if e.error != nil { | ||
555 | 28 | return fmt.Sprintf("%s: %v", e.Msg, e.error.Error()) | ||
556 | 29 | } | ||
557 | 30 | return e.Msg | ||
558 | 31 | } | ||
559 | 32 | return e.error.Error() | ||
560 | 33 | } | ||
561 | 34 | |||
562 | 35 | // NotFoundf returns a error for which IsNotFound returns | ||
563 | 36 | // true. The message for the error is made up from the given | ||
564 | 37 | // arguments formatted as with fmt.Sprintf, with the | ||
565 | 38 | // string " not found" appended. | ||
572 | 39 | func NotFoundf(format string, args ...interface{}) error { | 30 | func NotFoundf(format string, args ...interface{}) error { |
578 | 40 | return &NotFoundError{nil, fmt.Sprintf(format+" not found", args...)} | 31 | return RawNotFoundf(format+" not found", args...) |
579 | 41 | } | 32 | } |
580 | 42 | 33 | ||
581 | 43 | // UnauthorizedError represents the error that an operation is unauthorized. | 34 | // RawNotFoundf("foo %s", "bar") returns a *NotFoundError with the message |
582 | 44 | // Use IsUnauthorized() to determine if the error was related to authorization failure. | 35 | // "foo bar". |
583 | 36 | func RawNotFoundf(format string, args ...interface{}) error { | ||
584 | 37 | return &NotFoundError{fmt.Sprintf(format, args...)} | ||
585 | 38 | } | ||
586 | 39 | |||
587 | 40 | // UnauthorizedError indicates that some operation is unauthorized. | ||
588 | 45 | type UnauthorizedError struct { | 41 | type UnauthorizedError struct { |
589 | 46 | error | ||
590 | 47 | Msg string | 42 | Msg string |
591 | 48 | } | 43 | } |
592 | 49 | 44 | ||
593 | 45 | func (e *UnauthorizedError) Error() string { | ||
594 | 46 | if e.Msg == "" { | ||
595 | 47 | return "unauthorized <something>" | ||
596 | 48 | } | ||
597 | 49 | return e.Msg | ||
598 | 50 | } | ||
599 | 51 | |||
600 | 52 | // IsUnauthorizedError returns true if err is a *UnauthorizedError. | ||
601 | 50 | func IsUnauthorizedError(err error) bool { | 53 | func IsUnauthorizedError(err error) bool { |
602 | 51 | _, ok := err.(*UnauthorizedError) | 54 | _, ok := err.(*UnauthorizedError) |
603 | 52 | return ok | 55 | return ok |
604 | 53 | } | 56 | } |
605 | 54 | 57 | ||
615 | 55 | func (e *UnauthorizedError) Error() string { | 58 | // Unauthorizedf("foo %s", "bar") returns a *UnauthorizedError with the |
616 | 56 | if e.error != nil { | 59 | // message "unauthorized foo bar". |
608 | 57 | return fmt.Sprintf("%s: %v", e.Msg, e.error.Error()) | ||
609 | 58 | } | ||
610 | 59 | return e.Msg | ||
611 | 60 | } | ||
612 | 61 | |||
613 | 62 | // Unauthorizedf returns an error for which IsUnauthorizedError returns true. | ||
614 | 63 | // It is mainly used for testing. | ||
617 | 64 | func Unauthorizedf(format string, args ...interface{}) error { | 60 | func Unauthorizedf(format string, args ...interface{}) error { |
619 | 65 | return &UnauthorizedError{nil, fmt.Sprintf(format, args...)} | 61 | return RawUnauthorizedf("unauthorized "+format, args...) |
620 | 62 | } | ||
621 | 63 | |||
622 | 64 | // RawUnauthorizedf("foo %s", "bar") returns a *UnauthorizedError with the | ||
623 | 65 | // message "foo bar". | ||
624 | 66 | func RawUnauthorizedf(format string, args ...interface{}) error { | ||
625 | 67 | return &UnauthorizedError{fmt.Sprintf(format, args...)} | ||
626 | 66 | } | 68 | } |
627 | 67 | 69 | ||
628 | === added file 'errors/errors_test.go' | |||
629 | --- errors/errors_test.go 1970-01-01 00:00:00 +0000 | |||
630 | +++ errors/errors_test.go 2013-06-12 12:32:25 +0000 | |||
631 | @@ -0,0 +1,74 @@ | |||
632 | 1 | // Copyright 2011, 2012, 2013 Canonical Ltd. | ||
633 | 2 | // Licensed under the AGPLv3, see LICENCE file for details. | ||
634 | 3 | |||
635 | 4 | package errors_test | ||
636 | 5 | |||
637 | 6 | import ( | ||
638 | 7 | "fmt" | ||
639 | 8 | "testing" | ||
640 | 9 | |||
641 | 10 | . "launchpad.net/gocheck" | ||
642 | 11 | "launchpad.net/juju-core/errors" | ||
643 | 12 | ) | ||
644 | 13 | |||
645 | 14 | func TestPackage(t *testing.T) { | ||
646 | 15 | TestingT(t) | ||
647 | 16 | } | ||
648 | 17 | |||
649 | 18 | type NotFoundSuite struct{} | ||
650 | 19 | |||
651 | 20 | var _ = Suite(&NotFoundSuite{}) | ||
652 | 21 | |||
653 | 22 | func (s *NotFoundSuite) TestNotFound(c *C) { | ||
654 | 23 | emptyErr := &errors.NotFoundError{} | ||
655 | 24 | c.Check(emptyErr, ErrorMatches, `<something> not found`) | ||
656 | 25 | c.Check(errors.IsNotFoundError(emptyErr), Equals, true) | ||
657 | 26 | |||
658 | 27 | rawErr := &errors.NotFoundError{"widget cannot be located"} | ||
659 | 28 | c.Check(rawErr, ErrorMatches, `widget cannot be located`) | ||
660 | 29 | c.Check(errors.IsNotFoundError(rawErr), Equals, true) | ||
661 | 30 | |||
662 | 31 | badErr := fmt.Errorf("thingy not found") | ||
663 | 32 | c.Check(errors.IsNotFoundError(badErr), Equals, false) | ||
664 | 33 | |||
665 | 34 | c.Check(errors.IsNotFoundError(nil), Equals, false) | ||
666 | 35 | } | ||
667 | 36 | |||
668 | 37 | func (s *NotFoundSuite) TestNotFoundf(c *C) { | ||
669 | 38 | err := errors.NotFoundf("plop %q", "wibble") | ||
670 | 39 | c.Check(err, ErrorMatches, `plop "wibble" not found`) | ||
671 | 40 | c.Check(errors.IsNotFoundError(err), Equals, true) | ||
672 | 41 | |||
673 | 42 | err = errors.RawNotFoundf("plop %q", "wibble") | ||
674 | 43 | c.Check(err, ErrorMatches, `plop "wibble"`) | ||
675 | 44 | c.Check(errors.IsNotFoundError(err), Equals, true) | ||
676 | 45 | } | ||
677 | 46 | |||
678 | 47 | type UnauthorizedSuite struct{} | ||
679 | 48 | |||
680 | 49 | var _ = Suite(&UnauthorizedSuite{}) | ||
681 | 50 | |||
682 | 51 | func (s *UnauthorizedSuite) TestUnauthorized(c *C) { | ||
683 | 52 | emptyErr := &errors.UnauthorizedError{} | ||
684 | 53 | c.Check(emptyErr, ErrorMatches, `unauthorized <something>`) | ||
685 | 54 | c.Check(errors.IsUnauthorizedError(emptyErr), Equals, true) | ||
686 | 55 | |||
687 | 56 | rawErr := &errors.UnauthorizedError{"whatchamadoodle access is forbidden"} | ||
688 | 57 | c.Check(rawErr, ErrorMatches, `whatchamadoodle access is forbidden`) | ||
689 | 58 | c.Check(errors.IsUnauthorizedError(rawErr), Equals, true) | ||
690 | 59 | |||
691 | 60 | badErr := fmt.Errorf("unauthorized thingy") | ||
692 | 61 | c.Check(errors.IsUnauthorizedError(badErr), Equals, false) | ||
693 | 62 | |||
694 | 63 | c.Check(errors.IsUnauthorizedError(nil), Equals, false) | ||
695 | 64 | } | ||
696 | 65 | |||
697 | 66 | func (s *UnauthorizedSuite) TestUnauthorizedf(c *C) { | ||
698 | 67 | err := errors.Unauthorizedf("plop %q", "wibble") | ||
699 | 68 | c.Check(err, ErrorMatches, `unauthorized plop "wibble"`) | ||
700 | 69 | c.Check(errors.IsUnauthorizedError(err), Equals, true) | ||
701 | 70 | |||
702 | 71 | err = errors.RawUnauthorizedf("plop %q", "wibble") | ||
703 | 72 | c.Check(err, ErrorMatches, `plop "wibble"`) | ||
704 | 73 | c.Check(errors.IsUnauthorizedError(err), Equals, true) | ||
705 | 74 | } | ||
706 | 0 | 75 | ||
707 | === modified file 'state/apiserver/client_test.go' | |||
708 | --- state/apiserver/client_test.go 2013-06-12 11:25:32 +0000 | |||
709 | +++ state/apiserver/client_test.go 2013-06-12 12:32:25 +0000 | |||
710 | @@ -309,7 +309,7 @@ | |||
711 | 309 | "wordpress": `charm URL has invalid schema: "wordpress"`, | 309 | "wordpress": `charm URL has invalid schema: "wordpress"`, |
712 | 310 | "cs:wordpress": `charm URL without series: "cs:wordpress"`, | 310 | "cs:wordpress": `charm URL without series: "cs:wordpress"`, |
713 | 311 | "cs:precise/wordpress": "charm url must include revision", | 311 | "cs:precise/wordpress": "charm url must include revision", |
715 | 312 | "cs:precise/wordpress-999999": `cannot get charm: charm not found in mock store: cs:precise/wordpress-999999`, | 312 | "cs:precise/wordpress-999999": `cannot get charm: charm "cs:precise/wordpress-999999" not found in mock store`, |
716 | 313 | "local:precise/wordpress-999999": `charm url has unsupported schema "local"`, | 313 | "local:precise/wordpress-999999": `charm url has unsupported schema "local"`, |
717 | 314 | } { | 314 | } { |
718 | 315 | c.Logf("test %s", url) | 315 | c.Logf("test %s", url) |
719 | 316 | 316 | ||
720 | === modified file 'state/apiserver/errors_test.go' | |||
721 | --- state/apiserver/errors_test.go 2013-06-06 17:09:49 +0000 | |||
722 | +++ state/apiserver/errors_test.go 2013-06-12 12:32:25 +0000 | |||
723 | @@ -23,10 +23,10 @@ | |||
724 | 23 | err error | 23 | err error |
725 | 24 | code string | 24 | code string |
726 | 25 | }{{ | 25 | }{{ |
728 | 26 | err: errors.NotFoundf("hello"), | 26 | err: errors.RawNotFoundf("hello"), |
729 | 27 | code: api.CodeNotFound, | 27 | code: api.CodeNotFound, |
730 | 28 | }, { | 28 | }, { |
732 | 29 | err: errors.Unauthorizedf("hello"), | 29 | err: errors.RawUnauthorizedf("hello"), |
733 | 30 | code: api.CodeUnauthorized, | 30 | code: api.CodeUnauthorized, |
734 | 31 | }, { | 31 | }, { |
735 | 32 | err: state.ErrCannotEnterScopeYet, | 32 | err: state.ErrCannotEnterScopeYet, |
736 | 33 | 33 | ||
737 | === modified file 'state/open.go' | |||
738 | --- state/open.go 2013-06-10 11:16:30 +0000 | |||
739 | +++ state/open.go 2013-06-12 12:32:25 +0000 | |||
740 | @@ -178,19 +178,23 @@ | |||
741 | 178 | logSizeTests = 1000000 | 178 | logSizeTests = 1000000 |
742 | 179 | ) | 179 | ) |
743 | 180 | 180 | ||
745 | 181 | func maybeUnauthorized(err error, msg string) error { | 181 | func maybeUnauthorized(err error, message string) error { |
746 | 182 | if err == nil { | 182 | if err == nil { |
747 | 183 | return nil | 183 | return nil |
748 | 184 | } | 184 | } |
749 | 185 | // Unauthorized access errors have no error code, | 185 | // Unauthorized access errors have no error code, |
750 | 186 | // just a simple error string. | 186 | // just a simple error string. |
751 | 187 | // TODO(fwereade): does this comment imply that "auth fails" is a magic | ||
752 | 188 | // error value produced my mongo, or mgo, or something? And what about | ||
753 | 189 | // the magic "need to login" that's checked in testing/mgo.go? Should | ||
754 | 190 | // we be checking for that as well? | ||
755 | 187 | if err.Error() == "auth fails" { | 191 | if err.Error() == "auth fails" { |
757 | 188 | return &errors.UnauthorizedError{err, msg} | 192 | return errors.RawUnauthorizedf(message) |
758 | 189 | } | 193 | } |
759 | 190 | if err, ok := err.(*mgo.QueryError); ok && err.Code == 10057 { | 194 | if err, ok := err.(*mgo.QueryError); ok && err.Code == 10057 { |
761 | 191 | return &errors.UnauthorizedError{err, msg} | 195 | return errors.RawUnauthorizedf(message) |
762 | 192 | } | 196 | } |
764 | 193 | return fmt.Errorf("%s: %v", msg, err) | 197 | return fmt.Errorf("%s: %v", message, err) |
765 | 194 | } | 198 | } |
766 | 195 | 199 | ||
767 | 196 | func newState(session *mgo.Session, info *Info) (*State, error) { | 200 | func newState(session *mgo.Session, info *Info) (*State, error) { |
768 | 197 | 201 | ||
769 | === modified file 'state/state_test.go' | |||
770 | --- state/state_test.go 2013-06-11 20:56:00 +0000 | |||
771 | +++ state/state_test.go 2013-06-12 12:32:25 +0000 | |||
772 | @@ -79,13 +79,6 @@ | |||
773 | 79 | c.Assert(apiAddrs, DeepEquals, expectedAddrs) | 79 | c.Assert(apiAddrs, DeepEquals, expectedAddrs) |
774 | 80 | } | 80 | } |
775 | 81 | 81 | ||
776 | 82 | func (s *StateSuite) TestIsNotFound(c *C) { | ||
777 | 83 | err1 := fmt.Errorf("unrelated error") | ||
778 | 84 | err2 := errors.NotFoundf("foo") | ||
779 | 85 | c.Assert(errors.IsNotFoundError(err1), Equals, false) | ||
780 | 86 | c.Assert(errors.IsNotFoundError(err2), Equals, true) | ||
781 | 87 | } | ||
782 | 88 | |||
783 | 89 | func (s *StateSuite) TestAddCharm(c *C) { | 82 | func (s *StateSuite) TestAddCharm(c *C) { |
784 | 90 | // Check that adding charms from scratch works correctly. | 83 | // Check that adding charms from scratch works correctly. |
785 | 91 | ch := testing.Charms.Dir("dummy") | 84 | ch := testing.Charms.Dir("dummy") |
786 | 92 | 85 | ||
787 | === modified file 'testing/charm.go' | |||
788 | --- testing/charm.go 2013-06-12 11:25:32 +0000 | |||
789 | +++ testing/charm.go 2013-06-12 12:32:25 +0000 | |||
790 | @@ -6,10 +6,12 @@ | |||
791 | 6 | import ( | 6 | import ( |
792 | 7 | "fmt" | 7 | "fmt" |
793 | 8 | "go/build" | 8 | "go/build" |
794 | 9 | "launchpad.net/juju-core/charm" | ||
795 | 10 | "os" | 9 | "os" |
796 | 11 | "os/exec" | 10 | "os/exec" |
797 | 12 | "path/filepath" | 11 | "path/filepath" |
798 | 12 | |||
799 | 13 | "launchpad.net/juju-core/charm" | ||
800 | 14 | "launchpad.net/juju-core/errors" | ||
801 | 13 | ) | 15 | ) |
802 | 14 | 16 | ||
803 | 15 | func init() { | 17 | func init() { |
804 | @@ -169,7 +171,7 @@ | |||
805 | 169 | base, rev := s.interpret(charmURL) | 171 | base, rev := s.interpret(charmURL) |
806 | 170 | charm, found := s.charms[base][rev] | 172 | charm, found := s.charms[base][rev] |
807 | 171 | if !found { | 173 | if !found { |
809 | 172 | return nil, fmt.Errorf("charm not found in mock store: %s", charmURL) | 174 | return nil, errors.RawNotFoundf("charm %q not found in mock store", charmURL) |
810 | 173 | } | 175 | } |
811 | 174 | return charm, nil | 176 | return charm, nil |
812 | 175 | } | 177 | } |
813 | @@ -179,7 +181,7 @@ | |||
814 | 179 | charmURL = charmURL.WithRevision(-1) | 181 | charmURL = charmURL.WithRevision(-1) |
815 | 180 | base, rev := s.interpret(charmURL) | 182 | base, rev := s.interpret(charmURL) |
816 | 181 | if _, found := s.charms[base][rev]; !found { | 183 | if _, found := s.charms[base][rev]; !found { |
818 | 182 | return 0, fmt.Errorf("charm not found in mock store: %s", charmURL) | 184 | return 0, errors.RawNotFoundf("charm %q not found in mock store", charmURL) |
819 | 183 | } | 185 | } |
820 | 184 | return rev, nil | 186 | return rev, nil |
821 | 185 | } | 187 | } |
Reviewers: mp+168928_ code.launchpad. net,
Message:
Please take a look.
Description:
errors: tests, consistency
We weren't testing or making sensible use of the embedded errors in
NotFoundError and UnauthorizedError; we were doing inappropriate things
with both error types in various situations; the action of NotFoundf was
not consistent with Unauthorizedf.
These conditions no longer apply; and the various charm.Repository types NotFoundError instead of charm.NotFoundE rror.
in
play now produce errors.
I'm also more than somewhat freaked out by the "auth fails" magic
string.
Can anyone explain that?
https:/ /code.launchpad .net/~fwereade/ juju-core/ errors- cleanup/ +merge/ 168928
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/9830049/
Affected files: deploy_ test.go upgradecharm_ test.go agent_test. go bootstrap_ test.go dummy/storage. go ec2/storage. go imagemetadata/ simplestreams. go local/storage. go maas/storage. go openstack/ storage. go tools_test. go errors_ test.go /client_ test.go /errors_ test.go
A [revision details]
M charm/repo.go
M charm/repo_test.go
M cmd/juju/
M cmd/juju/publish.go
M cmd/juju/
M cmd/jujud/
M cmd/jujud/
M environs/
M environs/
M environs/
M environs/
M environs/
M environs/
M environs/tools.go
M environs/
M errors/errors.go
A errors/
M state/apiserver
M state/apiserver
M state/open.go
M state/state_test.go
M testing/charm.go