Merge lp:~fwereade/juju-core/errors-cleanup into lp:~juju/juju-core/trunk

[William Reade]

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.NotFoundError instead of charm.NotFoundError.

I'm also more than somewhat freaked out by the "auth fails" magic string.
Can anyone explain that?

https://codereview.appspot.com/9830049/

[William Reade (fwereade)]

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
in
play now produce errors.NotFoundError instead of charm.NotFoundError.

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:
   A [revision details]
   M charm/repo.go
   M charm/repo_test.go
   M cmd/juju/deploy_test.go
   M cmd/juju/publish.go
   M cmd/juju/upgradecharm_test.go
   M cmd/jujud/agent_test.go
   M cmd/jujud/bootstrap_test.go
   M environs/dummy/storage.go
   M environs/ec2/storage.go
   M environs/imagemetadata/simplestreams.go
   M environs/local/storage.go
   M environs/maas/storage.go
   M environs/openstack/storage.go
   M environs/tools.go
   M environs/tools_test.go
   M errors/errors.go
   A errors/errors_test.go
   M state/apiserver/client_test.go
   M state/apiserver/errors_test.go
   M state/open.go
   M state/state_test.go
   M testing/charm.go

[Roger Peppe (rogpeppe)]

LGTM with some suggestions. i'd like to lose the stuttering and this
seems like a reasonable CL to do it in.

https://codereview.appspot.com/9830049/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/9830049/diff/1/charm/repo.go#newcode288
charm/repo.go:288: return errors.RawNotFoundf("charm %q not found in
repository %q", curl, repoPath)
NotFoundf("charm %q in repository %q", curl, repoPath) ?

https://codereview.appspot.com/9830049/diff/1/environs/openstack/storage.go
File environs/openstack/storage.go (right):

https://codereview.appspot.com/9830049/diff/1/environs/openstack/storage.go#newcode11
environs/openstack/storage.go:11: coreerrors
"launchpad.net/juju-core/errors"
looks like this can lose the import alias.

https://codereview.appspot.com/9830049/diff/1/environs/tools_test.go
File environs/tools_test.go (right):

https://codereview.appspot.com/9830049/diff/1/environs/tools_test.go#newcode171
environs/tools_test.go:171: c.Check(err, DeepEquals,
&errors.NotFoundError{test.err.Error()})
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://codereview.appspot.com/9830049/diff/1/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/9830049/diff/1/errors/errors.go#newcode11
errors/errors.go:11: type NotFoundError struct {
s/NotFoundError/NotFound/ ?
(as discussed)

and ditto for Unauthorized and the Is* functions.

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

https://codereview.appspot.com/9830049/diff/1/state/apiserver/errors_test.go#newcode26
state/apiserver/errors_test.go:26: err: errors.RawNotFoundf("hello"),
no real need to use Raw here, is there?

https://codereview.appspot.com/9830049/diff/1/state/open.go
File state/open.go (right):

https://codereview.appspot.com/9830049/diff/1/state/open.go#newcode188
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
SetAdminMongoPassword BTW), but it's also a similar issue
re: lack of mongo error code.

https://codereview.appspot.com/9830049/diff/1/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/9830049/diff/1/testing/charm.go#newcode174
testing/charm.go:174: return nil, errors.RawNotFoundf("charm %q not
found in mock store", charmURL)
NotFoundf("charm %q in mock store", charmURL)
(and below)?

https://codereview.appspot.com/9830049/

[Tim Penhey (thumper)]

https://codereview.appspot.com/9830049/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/9830049/diff/1/charm/repo.go#newcode288
charm/repo.go:288: return errors.RawNotFoundf("charm %q not found in
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://codereview.appspot.com/9830049/diff/1/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/9830049/diff/1/charm/repo_test.go#newcode188
charm/repo_test.go:188: c.Assert(errors.IsNotFoundError(err), Equals,
true)
Nice. Although how about a matcher?

   c.Assert(err, IsNotFoundError)
or
   c.Assert(err, errors.IsNotFound)

https://codereview.appspot.com/9830049/diff/1/environs/imagemetadata/simplestreams.go
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9830049/diff/1/environs/imagemetadata/simplestreams.go#newcode248
environs/imagemetadata/simplestreams.go:248: return nil, dataURL,
errors.RawNotFoundf("invalid URL %q", dataURL)
Is there a good reason why we have two functions? RawNotFoundf and
NotFoundf? Why have two same but different functions/types?

https://codereview.appspot.com/9830049/diff/1/errors/errors.go
File errors/errors.go (right):

https://codereview.appspot.com/9830049/diff/1/errors/errors.go#newcode11
errors/errors.go:11: type NotFoundError struct {
On 2013/06/12 13:51:05, rog wrote:
> s/NotFoundError/NotFound/ ?
> (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.IsNotFoundError

Hopefully the package name makes it obvious.

(Message brought to you by the department of redundancy department)

https://codereview.appspot.com/9830049/diff/1/errors/errors.go#newcode35
errors/errors.go:35: // "foo bar".
The name of the method and be behaviour seem to be at odds to me.

https://codereview.appspot.com/9830049/

[Tim Penhey (thumper)]

On 2013/06/18 23:33:17, thumper wrote:
> https://codereview.appspot.com/9830049/diff/1/charm/repo.go
> File charm/repo.go (right):

> https://codereview.appspot.com/9830049/diff/1/charm/repo.go#newcode288
> charm/repo.go:288: return errors.RawNotFoundf("charm %q not found in
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://codereview.appspot.com/9830049/diff/1/charm/repo_test.go
> File charm/repo_test.go (right):

https://codereview.appspot.com/9830049/diff/1/charm/repo_test.go#newcode188
> charm/repo_test.go:188: c.Assert(errors.IsNotFoundError(err), Equals,
true)
> Nice. Although how about a matcher?

> c.Assert(err, IsNotFoundError)
> or
> c.Assert(err, errors.IsNotFound)

https://codereview.appspot.com/9830049/diff/1/environs/imagemetadata/simplestreams.go
> File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9830049/diff/1/environs/imagemetadata/simplestreams.go#newcode248
> environs/imagemetadata/simplestreams.go:248: return nil, dataURL,
> errors.RawNotFoundf("invalid URL %q", dataURL)
> Is there a good reason why we have two functions? RawNotFoundf and
NotFoundf?
> Why have two same but different functions/types?

> https://codereview.appspot.com/9830049/diff/1/errors/errors.go
> File errors/errors.go (right):

https://codereview.appspot.com/9830049/diff/1/errors/errors.go#newcode11
> errors/errors.go:11: type NotFoundError struct {
> On 2013/06/12 13:51:05, rog wrote:
> > s/NotFoundError/NotFound/ ?
> > (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.IsNotFoundError

> Hopefully the package name makes it obvious.

> (Message brought to you by the department of redundancy department)

https://codereview.appspot.com/9830049/diff/1/errors/errors.go#newcode35
> errors/errors.go:35: // "foo bar".
> 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

https://codereview.appspot.com/9830049/

[Tim Penhey (thumper)]

review: Approve

[John A Meinel (jameinel)]

Note this is still targeted at the old trunk.

Unmerged revisions

1268. By William Reade on 2013-06-12

mock charm store now uses errors.NotFoundError

1267. By William Reade on 2013-06-12

merge parent

1266. By William Reade on 2013-06-12

make errors somewhat consistent

1265. By William Reade on 2013-06-11

merge parent

1264. By William Reade on 2013-06-10

trivial error message fix (but notice how little it's tested for...)

1263. By William Reade on 2013-06-10

rewrite charm.Config for clarity, adding currently-unused ParseSettingsYAML method and new Settings type

1262. By Ian Booth on 2013-06-10

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 on 2013-06-10

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 on 2013-06-10

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 on 2013-06-07

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

=== modified file 'charm/repo.go'
--- charm/repo.go	2013-06-10 20:50:42 +0000
+++ charm/repo.go	2013-06-12 12:32:25 +0000
@@ -10,12 +10,14 @@
 	"fmt"
 	"io"
 	"io/ioutil"
-	"launchpad.net/juju-core/log"
 	"net/http"
 	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
+
+	"launchpad.net/juju-core/errors"
+	"launchpad.net/juju-core/log"
 )
 
 // CacheDir stores the charm cache directory path.
@@ -46,15 +48,6 @@
 	Latest(curl *URL) (int, error)
 }
 
-// NotFoundError represents an error indicating that the requested data wasn't found.
-type NotFoundError struct {
-	msg string
-}
-
-func (e *NotFoundError) Error() string {
-	return e.msg
-}
-
 // CharmStore is a Repository that provides access to the public juju charm store.
 type CharmStore struct {
 	BaseURL string
@@ -83,7 +76,7 @@
 		return nil, fmt.Errorf("charm: charm store returned response without charm %q", key)
 	}
 	if len(info.Errors) == 1 && info.Errors[0] == "entry not found" {
-		return nil, &NotFoundError{fmt.Sprintf("charm not found: %s", curl)}
+		return nil, errors.NotFoundf("charm %q", curl)
 	}
 	return info, nil
 }
@@ -116,9 +109,9 @@
 	}
 	if len(event.Errors) == 1 && event.Errors[0] == "entry not found" {
 		if digest == "" {
-			return nil, &NotFoundError{fmt.Sprintf("charm event not found for %q", curl)}
+			return nil, errors.NotFoundf("charm event for %q", curl)
 		} else {
-			return nil, &NotFoundError{fmt.Sprintf("charm event not found for %q with digest %q", curl, digest)}
+			return nil, errors.NotFoundf("charm event for %q with digest %q", curl, digest)
 		}
 	}
 	return event, nil
@@ -288,11 +281,11 @@
 }
 
 func repoNotFound(path string) error {
-	return &NotFoundError{fmt.Sprintf("no repository found at %q", path)}
+	return errors.NotFoundf("repository %q", path)
 }
 
 func charmNotFound(curl *URL, repoPath string) error {
-	return &NotFoundError{fmt.Sprintf("charm not found in %q: %s", repoPath, curl)}
+	return errors.RawNotFoundf("charm %q not found in repository %q", curl, repoPath)
 }
 
 func mightBeCharm(info os.FileInfo) bool {

=== modified file 'charm/repo_test.go'
--- charm/repo_test.go	2013-06-11 23:32:05 +0000
+++ charm/repo_test.go	2013-06-12 12:32:25 +0000
@@ -9,15 +9,17 @@
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
-	. "launchpad.net/gocheck"
-	"launchpad.net/juju-core/charm"
-	"launchpad.net/juju-core/testing"
 	"net"
 	"net/http"
 	"os"
 	"path/filepath"
 	"strconv"
 	"strings"
+
+	. "launchpad.net/gocheck"
+	"launchpad.net/juju-core/charm"
+	"launchpad.net/juju-core/errors"
+	"launchpad.net/juju-core/testing"
 )
 
 type MockStore struct {
@@ -180,11 +182,13 @@
 
 func (s *StoreSuite) TestMissing(c *C) {
 	charmURL := charm.MustParseURL("cs:series/missing")
-	expect := `charm not found: cs:series/missing`
+	expect := `charm "cs:series/missing" not found`
 	_, err := s.store.Latest(charmURL)
 	c.Assert(err, ErrorMatches, expect)
+	c.Assert(errors.IsNotFoundError(err), Equals, true)
 	_, err = s.store.Get(charmURL)
 	c.Assert(err, ErrorMatches, expect)
+	c.Assert(errors.IsNotFoundError(err), Equals, true)
 }
 
 func (s *StoreSuite) TestError(c *C) {
@@ -280,7 +284,8 @@
 func (s *StoreSuite) TestInfoNotFound(c *C) {
 	charmURL := charm.MustParseURL("cs:series/missing")
 	info, err := s.store.Info(charmURL)
-	c.Assert(err, ErrorMatches, `charm not found: cs:series/missing`)
+	c.Assert(err, ErrorMatches, `charm "cs:series/missing" not found`)
+	c.Assert(errors.IsNotFoundError(err), Equals, true)
 	c.Assert(info, IsNil)
 }
 
@@ -319,14 +324,16 @@
 func (s *StoreSuite) TestEventNotFound(c *C) {
 	charmURL := charm.MustParseURL("cs:series/missing")
 	event, err := s.store.Event(charmURL, "")
-	c.Assert(err, ErrorMatches, `charm event not found for "cs:series/missing"`)
+	c.Assert(err, ErrorMatches, `charm event for "cs:series/missing" not found`)
+	c.Assert(errors.IsNotFoundError(err), Equals, true)
 	c.Assert(event, IsNil)
 }
 
 func (s *StoreSuite) TestEventNotFoundDigest(c *C) {
 	charmURL := charm.MustParseURL("cs:series/good")
 	event, err := s.store.Event(charmURL, "missing-digest")
-	c.Assert(err, ErrorMatches, `charm event not found for "cs:series/good" with digest "missing-digest"`)
+	c.Assert(err, ErrorMatches, `charm event for "cs:series/good" with digest "missing-digest" not found`)
+	c.Assert(errors.IsNotFoundError(err), Equals, true)
 	c.Assert(event, IsNil)
 }
 
@@ -414,7 +421,7 @@
 }
 
 func (s *LocalRepoSuite) checkNotFoundErr(c *C, err error, charmURL *charm.URL) {
-	expect := `charm not found in "` + s.repo.Path + `": ` + charmURL.String()
+	expect := fmt.Sprintf("charm %q not found in repository %q", charmURL, s.repo.Path)
 	c.Check(err, ErrorMatches, expect)
 }
 
@@ -432,16 +439,18 @@
 }
 
 func (s *LocalRepoSuite) TestMissingRepo(c *C) {
+	curl := charm.MustParseURL("local:series/zebra")
+	expect := fmt.Sprintf("repository %q not found", s.repo.Path)
+	checkNotFound := func() {
+		_, err := s.repo.Latest(curl)
+		c.Check(err, ErrorMatches, expect)
+		_, err = s.repo.Get(curl)
+		c.Check(err, ErrorMatches, expect)
+	}
 	c.Assert(os.RemoveAll(s.repo.Path), IsNil)
-	_, err := s.repo.Latest(charm.MustParseURL("local:series/zebra"))
-	c.Assert(err, ErrorMatches, `no repository found at ".*"`)
-	_, err = s.repo.Get(charm.MustParseURL("local:series/zebra"))
-	c.Assert(err, ErrorMatches, `no repository found at ".*"`)
+	checkNotFound()
 	c.Assert(ioutil.WriteFile(s.repo.Path, nil, 0666), IsNil)
-	_, err = s.repo.Latest(charm.MustParseURL("local:series/zebra"))
-	c.Assert(err, ErrorMatches, `no repository found at ".*"`)
-	_, err = s.repo.Get(charm.MustParseURL("local:series/zebra"))
-	c.Assert(err, ErrorMatches, `no repository found at ".*"`)
+	checkNotFound()
 }
 
 func (s *LocalRepoSuite) TestMultipleVersions(c *C) {

=== modified file 'cmd/juju/deploy_test.go'
--- cmd/juju/deploy_test.go	2013-06-12 11:25:32 +0000
+++ cmd/juju/deploy_test.go	2013-06-12 12:32:25 +0000
@@ -62,7 +62,7 @@
 
 func (s *DeploySuite) TestNoCharm(c *C) {
 	err := runDeploy(c, "local:unknown-123")
-	c.Assert(err, ErrorMatches, `cannot get charm: charm not found in ".*": local:precise/unknown-123`)
+	c.Assert(err, ErrorMatches, `cannot get charm: charm "local:precise/unknown-123" not found in repository ".*"`)
 }
 
 func (s *DeploySuite) TestCharmDir(c *C) {

=== modified file 'cmd/juju/publish.go'
--- cmd/juju/publish.go	2013-05-02 15:55:42 +0000
+++ cmd/juju/publish.go	2013-06-12 12:32:25 +0000
@@ -5,14 +5,16 @@
 
 import (
 	"fmt"
+	"os"
+	"strings"
+	"time"
+
 	"launchpad.net/gnuflag"
 	"launchpad.net/juju-core/bzr"
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/cmd"
+	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/log"
-	"os"
-	"strings"
-	"time"
 )
 
 type PublishCommand struct {
@@ -131,9 +133,9 @@
 	}
 
 	oldEvent, err := charm.Store.Event(curl, localDigest)
-	if _, ok := err.(*charm.NotFoundError); ok {
+	if errors.IsNotFoundError(err) {
 		oldEvent, err = charm.Store.Event(curl, "")
-		if _, ok := err.(*charm.NotFoundError); ok {
+		if errors.IsNotFoundError(err) {
 			log.Infof("charm %s is not yet in the store", curl)
 			err = nil
 		}
@@ -156,7 +158,7 @@
 	for {
 		time.Sleep(c.pollDelay)
 		newEvent, err := charm.Store.Event(curl, "")
-		if _, ok := err.(*charm.NotFoundError); ok {
+		if errors.IsNotFoundError(err) {
 			continue
 		}
 		if err != nil {

=== modified file 'cmd/juju/upgradecharm_test.go'
--- cmd/juju/upgradecharm_test.go	2013-06-10 20:50:42 +0000
+++ cmd/juju/upgradecharm_test.go	2013-06-12 12:32:25 +0000
@@ -41,12 +41,12 @@
 	c.Assert(err, IsNil)
 
 	err = runUpgradeCharm(c, "riak", "--repository=blah")
-	c.Assert(err, ErrorMatches, `no repository found at ".*blah"`)
+	c.Assert(err, ErrorMatches, `repository ".*blah" not found`)
 	// Reset JUJU_REPOSITORY explicitly, because repoSuite.SetUpTest
 	// overwrites it (TearDownTest will revert it again).
 	os.Setenv("JUJU_REPOSITORY", "")
 	err = runUpgradeCharm(c, "riak", "--repository=")
-	c.Assert(err, ErrorMatches, `charm not found in ".*": local:precise/riak`)
+	c.Assert(err, ErrorMatches, `charm "local:precise/riak" not found in repository ".*"`)
 }
 
 func (s *UpgradeCharmErrorsSuite) TestInvalidService(c *C) {
@@ -70,11 +70,12 @@
 
 func (s *UpgradeCharmErrorsSuite) TestInvalidSwitchURL(c *C) {
 	s.deployService(c)
-	err := runUpgradeCharm(c, "riak", "--switch=blah")
-	c.Assert(err, ErrorMatches, "charm not found: cs:precise/blah")
-	err = runUpgradeCharm(c, "riak", "--switch=cs:missing/one")
-	c.Assert(err, ErrorMatches, "charm not found: cs:missing/one")
+	err := runUpgradeCharm(c, "riak", "--switch=local:blah")
+	c.Assert(err, ErrorMatches, `charm "local:precise/blah" not found in repository ".*"`)
+	err = runUpgradeCharm(c, "riak", "--switch=local:missing/one")
+	c.Assert(err, ErrorMatches, `charm "local:missing/one" not found in repository ".*"`)
 	// TODO(dimitern): add tests with incompatible charms
+	// TODO(fwereade): hook up forthcoming testing.MockCharmStore for cs: tests
 }
 
 func (s *UpgradeCharmErrorsSuite) TestSwitchAndRevisionFails(c *C) {

=== modified file 'cmd/jujud/agent_test.go'
--- cmd/jujud/agent_test.go	2013-05-31 08:33:34 +0000
+++ cmd/jujud/agent_test.go	2013-06-12 12:32:25 +0000
@@ -6,21 +6,22 @@
 import (
 	"bytes"
 	"fmt"
+	"net/http"
+	"os"
+	"path/filepath"
+	"time"
+
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/environs/agent"
 	"launchpad.net/juju-core/environs/tools"
-	"launchpad.net/juju-core/errors"
-	"launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
-	coretesting "launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/version"
 	"launchpad.net/juju-core/worker"
 	"launchpad.net/tomb"
-	"net/http"
-	"os"
-	"path/filepath"
-	"time"
+
+	"launchpad.net/juju-core/juju/testing"
+	coretesting "launchpad.net/juju-core/testing"
 )
 
 var _ = Suite(&toolSuite{})
@@ -348,13 +349,13 @@
 	info := s.StateInfo(c)
 	info.Tag = ent.Tag()
 	info.Password = "initial"
-	testOpenState(c, info, errors.Unauthorizedf("unauth"))
+	assertOpenState(c, info, false)
 
 	// Read the configuration and check that we can connect with it.
 	c.Assert(refreshConfig(conf), IsNil)
 	newPassword := conf.StateInfo.Password
 
-	testOpenState(c, conf.StateInfo, nil)
+	assertOpenState(c, conf.StateInfo, true)
 
 	// Check that it starts again ok
 	err = runStop(newAgent())
@@ -376,7 +377,7 @@
 	c.Assert(conf.StateInfo.Password, Not(Equals), newPassword)
 
 	info.Password = conf.StateInfo.Password
-	testOpenState(c, info, nil)
+	assertOpenState(c, info, true)
 }
 
 func (s *agentSuite) testUpgrade(c *C, agent runner, currentTools *state.Tools) {

=== modified file 'cmd/jujud/bootstrap_test.go'
--- cmd/jujud/bootstrap_test.go	2013-05-30 00:47:30 +0000
+++ cmd/jujud/bootstrap_test.go	2013-06-12 12:32:25 +0000
@@ -134,15 +134,15 @@
 	})
 }
 
-func testOpenState(c *C, info *state.Info, expectErr error) {
+func assertOpenState(c *C, info *state.Info, expectSuccess bool) {
 	st, err := state.Open(info, state.DefaultDialOpts())
 	if st != nil {
 		st.Close()
 	}
-	if expectErr != nil {
+	if expectSuccess {
+		c.Assert(err, IsNil)
+	} else {
 		c.Assert(errors.IsUnauthorizedError(err), Equals, true)
-	} else {
-		c.Assert(err, IsNil)
 	}
 }
 
@@ -162,10 +162,10 @@
 		Addrs:  []string{testing.MgoAddr},
 		CACert: []byte(testing.CACert),
 	}
-	testOpenState(c, info, errors.Unauthorizedf("some auth problem"))
+	assertOpenState(c, info, false)
 
 	info.Tag, info.Password = "machine-0", "foo"
-	testOpenState(c, info, nil)
+	assertOpenState(c, info, true)
 
 	info.Tag = ""
 	st, err := state.Open(info, state.DefaultDialOpts())

=== modified file 'environs/dummy/storage.go'
--- environs/dummy/storage.go	2013-05-30 00:47:30 +0000
+++ environs/dummy/storage.go	2013-06-12 12:32:25 +0000
@@ -82,7 +82,7 @@
 	}
 	data, ok := s.files[path]
 	if !ok {
-		return nil, errors.NotFoundf("file %q not found", path)
+		return nil, errors.NotFoundf("file %q", path)
 	}
 	return data, nil
 }

=== modified file 'environs/ec2/storage.go'
--- environs/ec2/storage.go	2013-05-30 00:47:30 +0000
+++ environs/ec2/storage.go	2013-06-12 12:32:25 +0000
@@ -160,7 +160,7 @@
 
 func maybeNotFound(err error) error {
 	if err != nil && s3ErrorStatusCode(err) == 404 {
-		return &errors.NotFoundError{err, ""}
+		return errors.RawNotFoundf("%v", err)
 	}
 	return err
 }

=== modified file 'environs/imagemetadata/simplestreams.go'
--- environs/imagemetadata/simplestreams.go	2013-05-30 05:39:33 +0000
+++ environs/imagemetadata/simplestreams.go	2013-06-12 12:32:25 +0000
@@ -245,14 +245,14 @@
 	dataURL += path
 	resp, err := http.Get(dataURL)
 	if err != nil {
-		return nil, dataURL, errors.NotFoundf("invalid URL %q", dataURL)
+		return nil, dataURL, errors.RawNotFoundf("invalid URL %q", dataURL)
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode == http.StatusNotFound {
-		return nil, dataURL, errors.NotFoundf("cannot find URL %q", dataURL)
+		return nil, dataURL, errors.NotFoundf("URL %q", dataURL)
 	}
 	if resp.StatusCode == http.StatusUnauthorized {
-		return nil, dataURL, errors.Unauthorizedf("unauthorised access to URL %q", dataURL)
+		return nil, dataURL, errors.Unauthorizedf("access to URL %q", dataURL)
 	}
 	if resp.StatusCode != http.StatusOK {
 		return nil, dataURL, fmt.Errorf("cannot access URL %q, %q", dataURL, resp.Status)
@@ -322,9 +322,9 @@
 		}
 	}
 	if !containsImageIds {
-		return "", errors.NotFoundf("index file missing %q data", imageIds)
+		return "", errors.RawNotFoundf("index file missing %q data", imageIds)
 	}
-	return "", errors.NotFoundf("index file missing data for cloud %v and product name(s) %q", ic.CloudSpec, prodIds)
+	return "", errors.RawNotFoundf("index file missing data for cloud %v and product name(s) %q", ic.CloudSpec, prodIds)
 }
 
 // utility function to see if element exists in values slice.

=== modified file 'environs/local/storage.go'
--- environs/local/storage.go	2013-05-30 00:47:30 +0000
+++ environs/local/storage.go	2013-06-12 12:32:25 +0000
@@ -42,7 +42,7 @@
 		return nil, err
 	}
 	if resp.StatusCode != 200 {
-		return nil, errors.NotFoundf("file %q not found", name)
+		return nil, errors.NotFoundf("file %q", name)
 	}
 	return resp.Body, nil
 }

=== modified file 'environs/maas/storage.go'
--- environs/maas/storage.go	2013-05-30 00:47:30 +0000
+++ environs/maas/storage.go	2013-06-12 12:32:25 +0000
@@ -77,9 +77,9 @@
 		noObj := gomaasapi.MAASObject{}
 		serverErr, ok := err.(gomaasapi.ServerError)
 		if ok && serverErr.StatusCode == 404 {
-			return noObj, errors.NotFoundf("file '%s' not found", name)
+			return noObj, errors.NotFoundf("file %q", name)
 		}
-		msg := fmt.Errorf("could not access file '%s': %v", name, err)
+		msg := fmt.Errorf("could not access file %q: %v", name, err)
 		return noObj, msg
 	}
 	return obj, nil

=== modified file 'environs/openstack/storage.go'
--- environs/openstack/storage.go	2013-05-31 01:02:53 +0000
+++ environs/openstack/storage.go	2013-06-12 12:32:25 +0000
@@ -160,7 +160,7 @@
 // container not being found.
 func maybeNotFound(err error) (error, bool) {
 	if err != nil && gooseerrors.IsNotFound(err) {
-		return &coreerrors.NotFoundError{err, ""}, true
+		return coreerrors.RawNotFoundf("%v", err), true
 	}
 	return err, false
 }

=== modified file 'environs/tools.go'
--- environs/tools.go	2013-05-30 00:47:30 +0000
+++ environs/tools.go	2013-06-12 12:32:25 +0000
@@ -147,6 +147,6 @@
 
 func convertToolsError(err *error) {
 	if isToolsError(*err) {
-		*err = &errors.NotFoundError{*err, ""}
+		*err = errors.RawNotFoundf("%v", *err)
 	}
 }

=== modified file 'environs/tools_test.go'
--- environs/tools_test.go	2013-05-30 00:47:30 +0000
+++ environs/tools_test.go	2013-06-12 12:32:25 +0000
@@ -168,7 +168,7 @@
 			if len(actual) > 0 {
 				c.Logf(actual.String())
 			}
-			c.Check(err, DeepEquals, &errors.NotFoundError{test.err, ""})
+			c.Check(err, DeepEquals, &errors.NotFoundError{test.err.Error()})
 			continue
 		}
 		source := private
@@ -389,7 +389,7 @@
 			if len(actual) > 0 {
 				c.Logf(actual.String())
 			}
-			c.Check(err, DeepEquals, &errors.NotFoundError{test.err, ""})
+			c.Check(err, DeepEquals, &errors.NotFoundError{test.err.Error()})
 			continue
 		}
 		expect := map[version.Binary]string{}
@@ -484,7 +484,7 @@
 			if len(actual) > 0 {
 				c.Logf(actual.String())
 			}
-			c.Check(err, DeepEquals, &errors.NotFoundError{test.err, ""})
+			c.Check(err, DeepEquals, &errors.NotFoundError{test.err.Error()})
 			continue
 		}
 		expect := map[version.Binary]string{}
@@ -548,7 +548,7 @@
 			}
 			c.Check(actual.URL, DeepEquals, source[actual.Binary])
 		} else {
-			c.Check(err, DeepEquals, &errors.NotFoundError{test.err, ""})
+			c.Check(err, DeepEquals, &errors.NotFoundError{test.err.Error()})
 		}
 	}
 }

=== modified file 'errors/errors.go'
--- errors/errors.go	2013-05-30 00:47:30 +0000
+++ errors/errors.go	2013-06-12 12:32:25 +0000
@@ -7,60 +7,62 @@
 	"fmt"
 )
 
-// NotFoundError records an error when something has not been found.
+// NotFoundError indicates that something has not been found.
 type NotFoundError struct {
-	// error is the underlying error.
-	error
 	Msg string
 }
 
-// IsNotFoundError returns true if err is a NotFoundError.
+func (e *NotFoundError) Error() string {
+	if e.Msg == "" {
+		return "<something> not found"
+	}
+	return e.Msg
+}
+
+// IsNotFoundError returns true if err is a *NotFoundError.
 func IsNotFoundError(err error) bool {
-	if _, ok := err.(*NotFoundError); ok {
-		return true
-	}
-	return false
-}
-
-func (e *NotFoundError) Error() string {
-	if e.Msg != "" || e.error == nil {
-		if e.error != nil {
-			return fmt.Sprintf("%s: %v", e.Msg, e.error.Error())
-		}
-		return e.Msg
-	}
-	return e.error.Error()
-}
-
-// NotFoundf returns a error for which IsNotFound returns
-// true. The message for the error is made up from the given
-// arguments formatted as with fmt.Sprintf, with the
-// string " not found" appended.
+	_, ok := err.(*NotFoundError)
+	return ok
+}
+
+// NotFoundf("foo %s", "bar") returns a *NotFoundError with the message
+// "foo bar not found".
 func NotFoundf(format string, args ...interface{}) error {
-	return &NotFoundError{nil, fmt.Sprintf(format+" not found", args...)}
-}
-
-// UnauthorizedError represents the error that an operation is unauthorized.
-// Use IsUnauthorized() to determine if the error was related to authorization failure.
+	return RawNotFoundf(format+" not found", args...)
+}
+
+// RawNotFoundf("foo %s", "bar") returns a *NotFoundError with the message
+// "foo bar".
+func RawNotFoundf(format string, args ...interface{}) error {
+	return &NotFoundError{fmt.Sprintf(format, args...)}
+}
+
+// UnauthorizedError indicates that some operation is unauthorized.
 type UnauthorizedError struct {
-	error
 	Msg string
 }
 
+func (e *UnauthorizedError) Error() string {
+	if e.Msg == "" {
+		return "unauthorized <something>"
+	}
+	return e.Msg
+}
+
+// IsUnauthorizedError returns true if err is a *UnauthorizedError.
 func IsUnauthorizedError(err error) bool {
 	_, ok := err.(*UnauthorizedError)
 	return ok
 }
 
-func (e *UnauthorizedError) Error() string {
-	if e.error != nil {
-		return fmt.Sprintf("%s: %v", e.Msg, e.error.Error())
-	}
-	return e.Msg
-}
-
-// Unauthorizedf returns an error for which IsUnauthorizedError returns true.
-// It is mainly used for testing.
+// Unauthorizedf("foo %s", "bar") returns a *UnauthorizedError with the
+// message "unauthorized foo bar".
 func Unauthorizedf(format string, args ...interface{}) error {
-	return &UnauthorizedError{nil, fmt.Sprintf(format, args...)}
+	return RawUnauthorizedf("unauthorized "+format, args...)
+}
+
+// RawUnauthorizedf("foo %s", "bar") returns a *UnauthorizedError with the
+// message "foo bar".
+func RawUnauthorizedf(format string, args ...interface{}) error {
+	return &UnauthorizedError{fmt.Sprintf(format, args...)}
 }

=== added file 'errors/errors_test.go'
--- errors/errors_test.go	1970-01-01 00:00:00 +0000
+++ errors/errors_test.go	2013-06-12 12:32:25 +0000
@@ -0,0 +1,74 @@
+// Copyright 2011, 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package errors_test
+
+import (
+	"fmt"
+	"testing"
+
+	. "launchpad.net/gocheck"
+	"launchpad.net/juju-core/errors"
+)
+
+func TestPackage(t *testing.T) {
+	TestingT(t)
+}
+
+type NotFoundSuite struct{}
+
+var _ = Suite(&NotFoundSuite{})
+
+func (s *NotFoundSuite) TestNotFound(c *C) {
+	emptyErr := &errors.NotFoundError{}
+	c.Check(emptyErr, ErrorMatches, `<something> not found`)
+	c.Check(errors.IsNotFoundError(emptyErr), Equals, true)
+
+	rawErr := &errors.NotFoundError{"widget cannot be located"}
+	c.Check(rawErr, ErrorMatches, `widget cannot be located`)
+	c.Check(errors.IsNotFoundError(rawErr), Equals, true)
+
+	badErr := fmt.Errorf("thingy not found")
+	c.Check(errors.IsNotFoundError(badErr), Equals, false)
+
+	c.Check(errors.IsNotFoundError(nil), Equals, false)
+}
+
+func (s *NotFoundSuite) TestNotFoundf(c *C) {
+	err := errors.NotFoundf("plop %q", "wibble")
+	c.Check(err, ErrorMatches, `plop "wibble" not found`)
+	c.Check(errors.IsNotFoundError(err), Equals, true)
+
+	err = errors.RawNotFoundf("plop %q", "wibble")
+	c.Check(err, ErrorMatches, `plop "wibble"`)
+	c.Check(errors.IsNotFoundError(err), Equals, true)
+}
+
+type UnauthorizedSuite struct{}
+
+var _ = Suite(&UnauthorizedSuite{})
+
+func (s *UnauthorizedSuite) TestUnauthorized(c *C) {
+	emptyErr := &errors.UnauthorizedError{}
+	c.Check(emptyErr, ErrorMatches, `unauthorized <something>`)
+	c.Check(errors.IsUnauthorizedError(emptyErr), Equals, true)
+
+	rawErr := &errors.UnauthorizedError{"whatchamadoodle access is forbidden"}
+	c.Check(rawErr, ErrorMatches, `whatchamadoodle access is forbidden`)
+	c.Check(errors.IsUnauthorizedError(rawErr), Equals, true)
+
+	badErr := fmt.Errorf("unauthorized thingy")
+	c.Check(errors.IsUnauthorizedError(badErr), Equals, false)
+
+	c.Check(errors.IsUnauthorizedError(nil), Equals, false)
+}
+
+func (s *UnauthorizedSuite) TestUnauthorizedf(c *C) {
+	err := errors.Unauthorizedf("plop %q", "wibble")
+	c.Check(err, ErrorMatches, `unauthorized plop "wibble"`)
+	c.Check(errors.IsUnauthorizedError(err), Equals, true)
+
+	err = errors.RawUnauthorizedf("plop %q", "wibble")
+	c.Check(err, ErrorMatches, `plop "wibble"`)
+	c.Check(errors.IsUnauthorizedError(err), Equals, true)
+}

=== modified file 'state/apiserver/client_test.go'
--- state/apiserver/client_test.go	2013-06-12 11:25:32 +0000
+++ state/apiserver/client_test.go	2013-06-12 12:32:25 +0000
@@ -309,7 +309,7 @@
 		"wordpress":                      `charm URL has invalid schema: "wordpress"`,
 		"cs:wordpress":                   `charm URL without series: "cs:wordpress"`,
 		"cs:precise/wordpress":           "charm url must include revision",
-		"cs:precise/wordpress-999999":    `cannot get charm: charm not found in mock store: cs:precise/wordpress-999999`,
+		"cs:precise/wordpress-999999":    `cannot get charm: charm "cs:precise/wordpress-999999" not found in mock store`,
 		"local:precise/wordpress-999999": `charm url has unsupported schema "local"`,
 	} {
 		c.Logf("test %s", url)

=== modified file 'state/apiserver/errors_test.go'
--- state/apiserver/errors_test.go	2013-06-06 17:09:49 +0000
+++ state/apiserver/errors_test.go	2013-06-12 12:32:25 +0000
@@ -23,10 +23,10 @@
 	err  error
 	code string
 }{{
-	err:  errors.NotFoundf("hello"),
+	err:  errors.RawNotFoundf("hello"),
 	code: api.CodeNotFound,
 }, {
-	err:  errors.Unauthorizedf("hello"),
+	err:  errors.RawUnauthorizedf("hello"),
 	code: api.CodeUnauthorized,
 }, {
 	err:  state.ErrCannotEnterScopeYet,

=== modified file 'state/open.go'
--- state/open.go	2013-06-10 11:16:30 +0000
+++ state/open.go	2013-06-12 12:32:25 +0000
@@ -178,19 +178,23 @@
 	logSizeTests = 1000000
 )
 
-func maybeUnauthorized(err error, msg string) error {
+func maybeUnauthorized(err error, message string) error {
 	if err == nil {
 		return nil
 	}
 	// Unauthorized access errors have no error code,
 	// just a simple error string.
+	// TODO(fwereade): does this comment imply that "auth fails" is a magic
+	// error value produced my mongo, or mgo, or something? And what about
+	// the magic "need to login" that's checked in testing/mgo.go? Should
+	// we be checking for that as well?
 	if err.Error() == "auth fails" {
-		return &errors.UnauthorizedError{err, msg}
+		return errors.RawUnauthorizedf(message)
 	}
 	if err, ok := err.(*mgo.QueryError); ok && err.Code == 10057 {
-		return &errors.UnauthorizedError{err, msg}
+		return errors.RawUnauthorizedf(message)
 	}
-	return fmt.Errorf("%s: %v", msg, err)
+	return fmt.Errorf("%s: %v", message, err)
 }
 
 func newState(session *mgo.Session, info *Info) (*State, error) {

=== modified file 'state/state_test.go'
--- state/state_test.go	2013-06-11 20:56:00 +0000
+++ state/state_test.go	2013-06-12 12:32:25 +0000
@@ -79,13 +79,6 @@
 	c.Assert(apiAddrs, DeepEquals, expectedAddrs)
 }
 
-func (s *StateSuite) TestIsNotFound(c *C) {
-	err1 := fmt.Errorf("unrelated error")
-	err2 := errors.NotFoundf("foo")
-	c.Assert(errors.IsNotFoundError(err1), Equals, false)
-	c.Assert(errors.IsNotFoundError(err2), Equals, true)
-}
-
 func (s *StateSuite) TestAddCharm(c *C) {
 	// Check that adding charms from scratch works correctly.
 	ch := testing.Charms.Dir("dummy")

=== modified file 'testing/charm.go'
--- testing/charm.go	2013-06-12 11:25:32 +0000
+++ testing/charm.go	2013-06-12 12:32:25 +0000
@@ -6,10 +6,12 @@
 import (
 	"fmt"
 	"go/build"
-	"launchpad.net/juju-core/charm"
 	"os"
 	"os/exec"
 	"path/filepath"
+
+	"launchpad.net/juju-core/charm"
+	"launchpad.net/juju-core/errors"
 )
 
 func init() {
@@ -169,7 +171,7 @@
 	base, rev := s.interpret(charmURL)
 	charm, found := s.charms[base][rev]
 	if !found {
-		return nil, fmt.Errorf("charm not found in mock store: %s", charmURL)
+		return nil, errors.RawNotFoundf("charm %q not found in mock store", charmURL)
 	}
 	return charm, nil
 }
@@ -179,7 +181,7 @@
 	charmURL = charmURL.WithRevision(-1)
 	base, rev := s.interpret(charmURL)
 	if _, found := s.charms[base][rev]; !found {
-		return 0, fmt.Errorf("charm not found in mock store: %s", charmURL)
+		return 0, errors.RawNotFoundf("charm %q not found in mock store", charmURL)
 	}
 	return rev, nil
 }