Merge lp:~thumper/juju-core/use-errgo into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Add errgo as a dep, and start using it.

errgo adds the ability to capture the location of
the error creation or annotation. Annotated errors
don't lose the original error.

https://codereview.appspot.com/59930043/

Add errgo as a dep, and start using it.

errgo adds the ability to capture the location of
the error creation or annotation. Annotated errors
don't lose the original error.

https://codereview.appspot.com/59930043/

[Tim Penhey (thumper)]

Reviewers: mp+204604_code.launchpad.net,

Message:
Please take a look.

Description:
Add errgo as a dep, and start using it.

errgo adds the ability to capture the location of
the error creation or annotation. Annotated errors
don't lose the original error.

https://code.launchpad.net/~thumper/juju-core/use-errgo/+merge/204604

(do not edit description out of merge proposal)

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

Affected files (+141, -77 lines):
   A [revision details]
   M agent/agent.go
   M agent/tools/toolsdir.go
   M cert/cert.go
   M cmd/juju/bootstrap.go
   M constraints/constraints.go
   M dependencies.tsv
   A dependencies_test.go
   M environs/cloudinit.go
   M environs/cloudinit/cloudinit.go
   M environs/config/config.go
   M environs/configstore/disk.go
   M environs/open.go
   M state/apiserver/charms.go
   M state/apiserver/client/client.go
   M worker/firewaller/firewaller.go

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/59930043/

[Roger Peppe (rogpeppe)]

https://codereview.appspot.com/59930043/diff/1/dependencies_test.go
File dependencies_test.go (right):

https://codereview.appspot.com/59930043/diff/1/dependencies_test.go#newcode30
dependencies_test.go:30: func (*dependenciesTest)
TestDependenciesTsvFormat(c *gc.C) {
I think this would work work better as a test in .lbox.check.
There is no package in the root directory and this doesn't really seem
like a great reason to create one.

awk -F "\t" 'NF != 4 {exit 1}' || {
     echo dependencies.tsv is in an invalid format >&2
     exit 1
}

would do the job.

https://codereview.appspot.com/59930043/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/04/2014 03:57 PM, Roger Peppe wrote:
>
> https://codereview.appspot.com/59930043/diff/1/dependencies_test.go
>
>
File dependencies_test.go (right):
>
> https://codereview.appspot.com/59930043/diff/1/dependencies_test.go#newcode30
>
>
dependencies_test.go:30: func (*dependenciesTest)
> TestDependenciesTsvFormat(c *gc.C) { I think this would work work
> better as a test in .lbox.check. There is no package in the root
> directory and this doesn't really seem like a great reason to
> create one.
>
> awk -F "\t" 'NF != 4 {exit 1}' || { echo dependencies.tsv is in an
> invalid format >&2 exit 1 }
>
> would do the job.
>
> https://codereview.appspot.com/59930043/
>

It would, but in a couple of weeks we won't be using lbox submit
anymore because we'll be switching bots, etc.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLw8kgACgkQJdeBCYSNAAPFowCePA3p9JoEFfj5697wqoStX9Mx
LzwAoLuDnRcPqSWcCUkpQSZzorSI7oQM
=3BxQ
-----END PGP SIGNATURE-----

[Roger Peppe (rogpeppe)]

On 4 February 2014 14:00, John A Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 02/04/2014 03:57 PM, Roger Peppe wrote:
>>
>> https://codereview.appspot.com/59930043/diff/1/dependencies_test.go
>>
>>
> File dependencies_test.go (right):
>>
>> https://codereview.appspot.com/59930043/diff/1/dependencies_test.go#newcode30
>>
>>
> dependencies_test.go:30: func (*dependenciesTest)
>> TestDependenciesTsvFormat(c *gc.C) { I think this would work work
>> better as a test in .lbox.check. There is no package in the root
>> directory and this doesn't really seem like a great reason to
>> create one.
>>
>> awk -F "\t" 'NF != 4 {exit 1}' || { echo dependencies.tsv is in an
>> invalid format >&2 exit 1 }
>>
>> would do the job.
>>
>> https://codereview.appspot.com/59930043/
>>
>
> It would, but in a couple of weeks we won't be using lbox submit
> anymore because we'll be switching bots, etc.

Presumably we'll still have some replacement that does similar prechecks
before letting the 'bot at it, won't we?

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

...

>
> Presumably we'll still have some replacement that does similar
> prechecks before letting the 'bot at it, won't we?
>

I'm not sure why we would need that, rather than just having the bot
do whatever checks we actually care about. (And perhaps a script so
you can easily run them yourself?)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLw9HUACgkQJdeBCYSNAANhjACgrbEUX2tmk+9r1HZJwTK5xA/s
ZPEAnijeFvfnHhubPtUHf7bcsV0k4Vm/
=8tSa
-----END PGP SIGNATURE-----

[Roger Peppe (rogpeppe)]

On 4 February 2014 14:09, John A Meinel <email address hidden> wrote:
>> Presumably we'll still have some replacement that does similar
>> prechecks before letting the 'bot at it, won't we?
>
> I'm not sure why we would need that, rather than just having the bot
> do whatever checks we actually care about. (And perhaps a script so
> you can easily run them yourself?)

I find it very useful because it provides a quick sanity check that
things are OK rather than checking my email in 30 minutes
only to find I'd forgotten to gofmt a file.

If we *don't* need a pre-check for all the things we currently
pre-check (gofmt, govet, go build, checktesting), then
why would we need a test specifically for dependencies.tsv
and not those?

It seems to me as if the dependencies.tsv format test
falls in the same category as the other .lbox.check
tests, and should be grouped accordingly.

One might argue that all the .lbox.check tests should
be Go tests, and I would probably agree, but only if our tests
took an order of magnitude less time to run.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/04/2014 04:33 PM, Roger Peppe wrote:
> On 4 February 2014 14:09, John A Meinel <email address hidden>
> wrote:
>>> Presumably we'll still have some replacement that does similar
>>> prechecks before letting the 'bot at it, won't we?
>>
>> I'm not sure why we would need that, rather than just having the
>> bot do whatever checks we actually care about. (And perhaps a
>> script so you can easily run them yourself?)
>
> I find it very useful because it provides a quick sanity check
> that things are OK rather than checking my email in 30 minutes only
> to find I'd forgotten to gofmt a file.

The bot already does go fmt automatically before running the test
suite, so you don't actually have to go fmt for the bot.

>
> If we *don't* need a pre-check for all the things we currently
> pre-check (gofmt, govet, go build, checktesting), then why would we
> need a test specifically for dependencies.tsv and not those?

Because we've repeatedly broken dependencies.tsv for CI but not noticed?

>
> It seems to me as if the dependencies.tsv format test falls in the
> same category as the other .lbox.check tests, and should be grouped
> accordingly.
>
> One might argue that all the .lbox.check tests should be Go tests,
> and I would probably agree, but only if our tests took an order of
> magnitude less time to run.
>

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLxABAACgkQJdeBCYSNAAMowwCfTCqU4gVWdRbdctezwkH7O4zH
BTgAoJn7zDnpOha3d/eSn0mvjAT0jPo/
=jyiJ
-----END PGP SIGNATURE-----

[Roger Peppe (rogpeppe)]

On 4 February 2014 15:00, John A Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 02/04/2014 04:33 PM, Roger Peppe wrote:
>> On 4 February 2014 14:09, John A Meinel <email address hidden>
>> wrote:
>>>> Presumably we'll still have some replacement that does similar
>>>> prechecks before letting the 'bot at it, won't we?
>>>
>>> I'm not sure why we would need that, rather than just having the
>>> bot do whatever checks we actually care about. (And perhaps a
>>> script so you can easily run them yourself?)
>>
>> I find it very useful because it provides a quick sanity check
>> that things are OK rather than checking my email in 30 minutes only
>> to find I'd forgotten to gofmt a file.
>
> The bot already does go fmt automatically before running the test
> suite, so you don't actually have to go fmt for the bot.
>
>>
>> If we *don't* need a pre-check for all the things we currently
>> pre-check (gofmt, govet, go build, checktesting), then why would we
>> need a test specifically for dependencies.tsv and not those?
>
> Because we've repeatedly broken dependencies.tsv for CI but not noticed?

We have also repeatedly had tests with broken printf format
strings and that have failed to run tests by omitting a call to
gocheck.TestingT.

Can we not lose any of those checks, please?

And then the dependencies.tsv test can go alongside them, whereever
we decide they should live.

=== modified file 'agent/agent.go'
--- agent/agent.go	2014-01-28 04:58:43 +0000
+++ agent/agent.go	2014-02-04 06:44:26 +0000
@@ -9,6 +9,7 @@
 	"regexp"
 	"sync"
 
+	"github.com/errgo/errgo"
 	"github.com/loggo/loggo"
 
 	"launchpad.net/juju-core/errors"
@@ -150,16 +151,16 @@
 // machine or unit agent.
 func NewAgentConfig(params AgentConfigParams) (Config, error) {
 	if params.DataDir == "" {
-		return nil, requiredError("data directory")
+		return nil, errgo.Trace(requiredError("data directory"))
 	}
 	if params.Tag == "" {
-		return nil, requiredError("entity tag")
+		return nil, errgo.Trace(requiredError("entity tag"))
 	}
 	if params.Password == "" {
-		return nil, requiredError("password")
+		return nil, errgo.Trace(requiredError("password"))
 	}
 	if params.CACert == nil {
-		return nil, requiredError("CA certificate")
+		return nil, errgo.Trace(requiredError("CA certificate"))
 	}
 	// Note that the password parts of the state and api information are
 	// blank.  This is by design.
@@ -202,10 +203,10 @@
 // a machine running the state server.
 func NewStateMachineConfig(params StateMachineConfigParams) (Config, error) {
 	if params.StateServerCert == nil {
-		return nil, requiredError("state server cert")
+		return nil, errgo.Trace(requiredError("state server cert"))
 	}
 	if params.StateServerKey == nil {
-		return nil, requiredError("state server key")
+		return nil, errgo.Trace(requiredError("state server key"))
 	}
 	config0, err := NewAgentConfig(params.AgentConfigParams)
 	if err != nil {
@@ -308,7 +309,7 @@
 
 func (c *configInternal) APIAddresses() ([]string, error) {
 	if c.apiDetails == nil {
-		return []string{}, fmt.Errorf("No apidetails in config")
+		return []string{}, errgo.New("No apidetails in config")
 	}
 	return append([]string{}, c.apiDetails.addresses...), nil
 }
@@ -323,7 +324,7 @@
 
 func (c *configInternal) check() error {
 	if c.stateDetails == nil && c.apiDetails == nil {
-		return requiredError("state or API addresses")
+		return errgo.Trace(requiredError("state or API addresses"))
 	}
 	if c.stateDetails != nil {
 		if err := checkAddrs(c.stateDetails.addresses, "state server address"); err != nil {
@@ -342,11 +343,11 @@
 
 func checkAddrs(addrs []string, what string) error {
 	if len(addrs) == 0 {
-		return requiredError(what)
+		return errgo.Trace(requiredError(what))
 	}
 	for _, a := range addrs {
 		if !validAddr.MatchString(a) {
-			return fmt.Errorf("invalid %s %q", what, a)
+			return errgo.New("invalid %s %q", what, a)
 		}
 	}
 	return nil

=== modified file 'agent/tools/toolsdir.go'
--- agent/tools/toolsdir.go	2013-09-25 00:18:35 +0000
+++ agent/tools/toolsdir.go	2014-02-04 06:44:26 +0000
@@ -15,6 +15,8 @@
 	"path"
 	"strings"
 
+	"github.com/errgo/errgo"
+
 	coretools "launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/version"
 )
@@ -99,7 +101,7 @@
 		}
 		name := path.Join(dir, hdr.Name)
 		if err := writeFile(name, os.FileMode(hdr.Mode&0777), tr); err != nil {
-			return fmt.Errorf("tar extract %q failed: %v", name, err)
+			return errgo.Annotatef(err, "tar extract %q failed", name)
 		}
 	}
 	toolsMetadataData, err := json.Marshal(tools)

=== modified file 'cert/cert.go'
--- cert/cert.go	2013-09-24 05:42:43 +0000
+++ cert/cert.go	2014-02-04 06:44:26 +0000
@@ -16,6 +16,8 @@
 	"math/big"
 	"net"
 	"time"
+
+	"github.com/errgo/errgo"
 )
 
 var KeyBits = 1024
@@ -61,11 +63,11 @@
 func Verify(srvCertPEM, caCertPEM []byte, when time.Time) error {
 	caCert, err := ParseCert(caCertPEM)
 	if err != nil {
-		return fmt.Errorf("cannot parse CA certificate: %v", err)
+		return errgo.Annotate(err, "cannot parse CA certificate")
 	}
 	srvCert, err := ParseCert(srvCertPEM)
 	if err != nil {
-		return fmt.Errorf("cannot parse server certificate: %v", err)
+		return errgo.Annotate(err, "cannot parse server certificate")
 	}
 	pool := x509.NewCertPool()
 	pool.AddCert(caCert)

=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go	2014-01-31 00:14:11 +0000
+++ cmd/juju/bootstrap.go	2014-02-04 06:44:26 +0000
@@ -9,6 +9,7 @@
 	"os"
 	"strings"
 
+	"github.com/errgo/errgo"
 	"launchpad.net/gnuflag"
 
 	"launchpad.net/juju-core/charm"
@@ -128,7 +129,7 @@
 	// we'd otherwise use environ.Storage.
 	if bs, ok := environ.(environs.BootstrapStorager); ok {
 		if err := bs.EnableBootstrapStorage(bootstrapContext); err != nil {
-			return fmt.Errorf("failed to enable bootstrap storage: %v", err)
+			return errgo.Annotate(err, "failed to enable bootstrap storage")
 		}
 	}
 	if err := bootstrap.EnsureNotBootstrapped(environ); err != nil {

=== modified file 'constraints/constraints.go'
--- constraints/constraints.go	2013-11-12 01:59:52 +0000
+++ constraints/constraints.go	2014-02-04 06:44:26 +0000
@@ -9,6 +9,8 @@
 	"strconv"
 	"strings"
 
+	"github.com/errgo/errgo"
+
 	"launchpad.net/juju-core/instance"
 )
 
@@ -207,7 +209,7 @@
 		return fmt.Errorf("unknown constraint %q", name)
 	}
 	if err != nil {
-		return fmt.Errorf("bad %q constraint: %v", name, err)
+		return errgo.Annotatef(err, "bad %q constraint", name)
 	}
 	return nil
 }

=== modified file 'dependencies.tsv'
--- dependencies.tsv	2014-01-29 14:14:20 +0000
+++ dependencies.tsv	2014-02-04 06:44:26 +0000
@@ -1,6 +1,7 @@
 code.google.com/p/go.crypto	hg	6478cc9340cbbe6c04511280c5007722269108e9	184
 code.google.com/p/go.net	hg	3591c18acabc99439c783463ef00e6dc277eee39	77
 labix.org/v2/mgo	bzr	[e-mail]-20131118213720-aralgr4ienh0gdyq	248
+github.com/errgo/errgo	git	93d72bf813883d1054cae1c001d3a46603f7f559	
 github.com/loggo/loggo	git	89458b4dc99692bc24efe9c2252d7587f8dc247b	
 launchpad.net/gnuflag	bzr	[e-mail]-20121003093437-zcyyw0lpvj2nifpk	12
 launchpad.net/goamz	bzr	[e-mail]-20131218155244-hbnkvlkkzy3vmlh9	44

=== added file 'dependencies_test.go'
--- dependencies_test.go	1970-01-01 00:00:00 +0000
+++ dependencies_test.go	2014-02-04 06:44:26 +0000
@@ -0,0 +1,42 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package juju_test
+
+import (
+	"go/build"
+	"io/ioutil"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	gc "launchpad.net/gocheck"
+)
+
+func Test(t *testing.T) {
+	gc.TestingT(t)
+}
+
+type dependenciesTest struct{}
+
+var _ = gc.Suite(&dependenciesTest{})
+
+func projectRoot(c *gc.C) string {
+	p, err := build.Import("launchpad.net/juju-core", "", build.FindOnly)
+	c.Assert(err, gc.IsNil)
+	return p.Dir
+}
+
+func (*dependenciesTest) TestDependenciesTsvFormat(c *gc.C) {
+	filename := filepath.Join(projectRoot(c), "dependencies.tsv")
+	content, err := ioutil.ReadFile(filename)
+	c.Assert(err, gc.IsNil)
+
+	for _, line := range strings.Split(string(content), "\n") {
+		if line == "" {
+			continue
+		}
+		segments := strings.Split(line, "\t")
+		c.Assert(segments, gc.HasLen, 4)
+	}
+}

=== modified file 'environs/cloudinit.go'
--- environs/cloudinit.go	2014-01-31 03:36:58 +0000
+++ environs/cloudinit.go	2014-02-04 06:44:26 +0000
@@ -6,6 +6,8 @@
 import (
 	"fmt"
 
+	"github.com/errgo/errgo"
+
 	"launchpad.net/juju-core/agent"
 	coreCloudinit "launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/constraints"
@@ -154,7 +156,7 @@
 	// These really are directly relevant to running a state server.
 	cert, key, err := cfg.GenerateStateServerCertAndKey()
 	if err != nil {
-		return fmt.Errorf("cannot generate state server certificate: %v", err)
+		return errgo.Annotate(err, "cannot generate state server certificate")
 	}
 	mcfg.StateServerCert = cert
 	mcfg.StateServerKey = key

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2014-01-30 19:54:48 +0000
+++ environs/cloudinit/cloudinit.go	2014-02-04 06:44:26 +0000
@@ -10,6 +10,7 @@
 	"path"
 	"strings"
 
+	"github.com/errgo/errgo"
 	"launchpad.net/goyaml"
 
 	"launchpad.net/juju-core/agent"
@@ -456,7 +457,7 @@
 	}
 	cmds, err := acfg.WriteCommands()
 	if err != nil {
-		return nil, err
+		return nil, errgo.Annotate(err, "failed to write commands")
 	}
 	c.AddScripts(cmds...)
 	return acfg, nil
@@ -474,7 +475,7 @@
 	conf := upstart.MachineAgentUpstartService(name, toolsDir, cfg.DataDir, cfg.LogDir, tag, machineId, nil)
 	cmds, err := conf.InstallCommands()
 	if err != nil {
-		return fmt.Errorf("cannot make cloud-init upstart script for the %s agent: %v", tag, err)
+		return errgo.Annotatef(err, "cannot make cloud-init upstart script for the %s agent", tag)
 	}
 	c.AddRunCmd(cloudinit.LogProgressCmd("Starting Juju machine agent (%s)", name))
 	c.AddScripts(cmds...)
@@ -496,7 +497,7 @@
 	conf := upstart.MongoUpstartService(name, cfg.DataDir, dbDir, cfg.StatePort)
 	cmds, err := conf.InstallCommands()
 	if err != nil {
-		return fmt.Errorf("cannot make cloud-init upstart script for the state database: %v", err)
+		return errgo.Annotate(err, "cannot make cloud-init upstart script for the state database")
 	}
 	c.AddRunCmd(cloudinit.LogProgressCmd("Starting MongoDB server (%s)", name))
 	c.AddScripts(cmds...)

=== modified file 'environs/config/config.go'
--- environs/config/config.go	2014-02-04 01:15:09 +0000
+++ environs/config/config.go	2014-02-04 06:44:26 +0000
@@ -12,6 +12,7 @@
 	"strings"
 	"time"
 
+	"github.com/errgo/errgo"
 	"github.com/loggo/loggo"
 
 	"launchpad.net/juju-core/cert"
@@ -277,7 +278,7 @@
 	caKey, caKeyOK := cfg.CAPrivateKey()
 	if caCertOK || caKeyOK {
 		if err := verifyKeyPair(caCert, caKey); err != nil {
-			return fmt.Errorf("bad CA certificate/key in configuration: %v", err)
+			return errgo.Annotate(err, "bad CA certificate/key in configuration")
 		}
 	}
 

=== modified file 'environs/configstore/disk.go'
--- environs/configstore/disk.go	2014-01-28 04:58:43 +0000
+++ environs/configstore/disk.go	2014-02-04 06:44:26 +0000
@@ -9,6 +9,7 @@
 	"os"
 	"path/filepath"
 
+	"github.com/errgo/errgo"
 	"github.com/loggo/loggo"
 	"launchpad.net/goyaml"
 
@@ -162,14 +163,14 @@
 func (info *environInfo) Write() error {
 	data, err := goyaml.Marshal(info)
 	if err != nil {
-		return fmt.Errorf("cannot marshal environment info: %v", err)
+		return errgo.Annotate(err, "cannot marshal environment info")
 	}
 	// Create a temporary file and rename it, so that the data
 	// changes atomically.
 	parent, _ := filepath.Split(info.path)
 	tmpFile, err := ioutil.TempFile(parent, "")
 	if err != nil {
-		return fmt.Errorf("cannot create temporary file: %v", err)
+		return errgo.Annotate(err, "cannot create temporary file")
 	}
 	_, err = tmpFile.Write(data)
 	// N.B. We need to close the file before renaming it
@@ -177,11 +178,11 @@
 	// error.
 	tmpFile.Close()
 	if err != nil {
-		return fmt.Errorf("cannot write temporary file: %v", err)
+		return errgo.Annotate(err, "cannot write temporary file")
 	}
 	if err := utils.ReplaceFile(tmpFile.Name(), info.path); err != nil {
 		os.Remove(tmpFile.Name())
-		return fmt.Errorf("cannot rename new environment info file: %v", err)
+		return errgo.Annotate(err, "cannot rename new environment info file")
 	}
 	info.initialized = true
 	return nil

=== modified file 'environs/open.go'
--- environs/open.go	2013-10-04 12:18:17 +0000
+++ environs/open.go	2014-02-04 06:44:26 +0000
@@ -9,6 +9,8 @@
 	"strings"
 	"time"
 
+	"github.com/errgo/errgo"
+
 	"launchpad.net/juju-core/cert"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/environs/configstore"
@@ -243,7 +245,7 @@
 		return err
 	}
 	if err := info.Destroy(); err != nil {
-		return fmt.Errorf("cannot destroy environment configuration information: %v", err)
+		return errgo.Annotate(err, "cannot destroy environment configuration information")
 	}
 	return nil
 }

=== modified file 'state/apiserver/charms.go'
--- state/apiserver/charms.go	2014-01-31 15:14:34 +0000
+++ state/apiserver/charms.go	2014-02-04 06:44:26 +0000
@@ -18,6 +18,8 @@
 	"path/filepath"
 	"strings"
 
+	"github.com/errgo/errgo"
+
 	"launchpad.net/juju-core/charm"
 	envtesting "launchpad.net/juju-core/environs/testing"
 	"launchpad.net/juju-core/names"
@@ -174,13 +176,13 @@
 	}
 	zipr, err := zip.NewReader(f, fi.Size())
 	if err != nil {
-		return fmt.Errorf("cannot open charm archive: %v", err)
+		return errgo.Annotate(err, "cannot open charm archive")
 	}
 
 	// Find out the root dir prefix from the archive.
 	rootDir, err := h.findArchiveRootDir(zipr)
 	if err != nil {
-		return fmt.Errorf("cannot read charm archive: %v", err)
+		return errgo.Annotate(err, "cannot read charm archive")
 	}
 	if rootDir == "" {
 		// Normal charm, just use charm.ReadBundle().
@@ -190,16 +192,16 @@
 	// dir and then read is as a charm dir.
 	tempDir, err := ioutil.TempDir("", "charm-extract")
 	if err != nil {
-		return fmt.Errorf("cannot create temp directory: %v", err)
+		return errgo.Annotate(err, "cannot create temp directory")
 	}
 	defer os.RemoveAll(tempDir)
 	err = h.extractArchiveTo(zipr, rootDir, tempDir)
 	if err != nil {
-		return fmt.Errorf("cannot extract charm archive: %v", err)
+		return errgo.Annotate(err, "cannot extract charm archive")
 	}
 	dir, err := charm.ReadDir(tempDir)
 	if err != nil {
-		return fmt.Errorf("cannot read extracted archive: %v", err)
+		return errgo.Annotate(err, "cannot read extracted archive")
 	}
 	// Now repackage the dir as a bundle at the original path.
 	if err := f.Truncate(0); err != nil {
@@ -354,66 +356,66 @@
 	// dir and the repackaged archive.
 	tempDir, err := ioutil.TempDir("", "charm-download")
 	if err != nil {
-		return fmt.Errorf("cannot create temp directory: %v", err)
+		return errgo.Annotate(err, "cannot create temp directory")
 	}
 	defer os.RemoveAll(tempDir)
 	extractPath := filepath.Join(tempDir, "extracted")
 	repackagedPath := filepath.Join(tempDir, "repackaged.zip")
 	repackagedArchive, err := os.Create(repackagedPath)
 	if err != nil {
-		return fmt.Errorf("cannot repackage uploaded charm: %v", err)
+		return errgo.Annotate(err, "cannot repackage uploaded charm")
 	}
 	defer repackagedArchive.Close()
 
 	// Expand and repack it with the revision specified by curl.
 	archive.SetRevision(curl.Revision)
 	if err := archive.ExpandTo(extractPath); err != nil {
-		return fmt.Errorf("cannot extract uploaded charm: %v", err)
+		return errgo.Annotate(err, "cannot extract uploaded charm")
 	}
 	charmDir, err := charm.ReadDir(extractPath)
 	if err != nil {
-		return fmt.Errorf("cannot read extracted charm: %v", err)
+		return errgo.Annotate(err, "cannot read extracted charm")
 	}
 	// Bundle the charm and calculate its sha256 hash at the
 	// same time.
 	hash := sha256.New()
 	err = charmDir.BundleTo(io.MultiWriter(hash, repackagedArchive))
 	if err != nil {
-		return fmt.Errorf("cannot repackage uploaded charm: %v", err)
+		return errgo.Annotate(err, "cannot repackage uploaded charm")
 	}
 	bundleSHA256 := hex.EncodeToString(hash.Sum(nil))
 	size, err := repackagedArchive.Seek(0, 2)
 	if err != nil {
-		return fmt.Errorf("cannot get charm file size: %v", err)
+		return errgo.Annotate(err, "cannot get charm file size")
 	}
 	// Seek to the beginning so the subsequent Put will read
 	// the whole file again.
 	if _, err := repackagedArchive.Seek(0, 0); err != nil {
-		return fmt.Errorf("cannot rewind the charm file reader: %v", err)
+		return errgo.Annotate(err, "cannot rewind the charm file reader")
 	}
 
 	// Now upload to provider storage.
 	storage, err := envtesting.GetEnvironStorage(h.state)
 	if err != nil {
-		return fmt.Errorf("cannot access provider storage: %v", err)
+		return errgo.Annotate(err, "cannot access provider storage")
 	}
 	name := charm.Quote(curl.String())
 	if err := storage.Put(name, repackagedArchive, size); err != nil {
-		return fmt.Errorf("cannot upload charm to provider storage: %v", err)
+		return errgo.Annotate(err, "cannot upload charm to provider storage")
 	}
 	storageURL, err := storage.URL(name)
 	if err != nil {
-		return fmt.Errorf("cannot get storage URL for charm: %v", err)
+		return errgo.Annotate(err, "cannot get storage URL for charm")
 	}
 	bundleURL, err := url.Parse(storageURL)
 	if err != nil {
-		return fmt.Errorf("cannot parse storage URL: %v", err)
+		return errgo.Annotate(err, "cannot parse storage URL")
 	}
 
 	// And finally, update state.
 	_, err = h.state.UpdateUploadedCharm(archive, curl, bundleURL, bundleSHA256)
 	if err != nil {
-		return fmt.Errorf("cannot update uploaded charm in state: %v", err)
+		return errgo.Annotate(err, "cannot update uploaded charm in state")
 	}
 	return nil
 }

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-01-30 14:22:46 +0000
+++ state/apiserver/client/client.go	2014-02-04 06:44:26 +0000
@@ -9,6 +9,7 @@
 	"os"
 	"strings"
 
+	"github.com/errgo/errgo"
 	"github.com/loggo/loggo"
 
 	"launchpad.net/juju-core/charm"
@@ -851,47 +852,47 @@
 	store := config.AuthorizeCharmRepo(CharmStore, envConfig)
 	downloadedCharm, err := store.Get(charmURL)
 	if err != nil {
-		return fmt.Errorf("cannot download charm %q: %v", charmURL.String(), err)
+		return errgo.Annotatef(err, "cannot download charm %q", charmURL.String())
 	}
 
 	// Open it and calculate the SHA256 hash.
 	downloadedBundle, ok := downloadedCharm.(*charm.Bundle)
 	if !ok {
-		return fmt.Errorf("expected a charm archive, got %T", downloadedCharm)
+		return errgo.New("expected a charm archive, got %T", downloadedCharm)
 	}
 	archive, err := os.Open(downloadedBundle.Path)
 	if err != nil {
-		return fmt.Errorf("cannot read downloaded charm: %v", err)
+		return errgo.Annotate(err, "cannot read downloaded charm")
 	}
 	defer archive.Close()
 	bundleSHA256, size, err := utils.ReadSHA256(archive)
 	if err != nil {
-		return fmt.Errorf("cannot calculate SHA256 hash of charm: %v", err)
+		return errgo.Annotate(err, "cannot calculate SHA256 hash of charm")
 	}
 	if _, err := archive.Seek(0, 0); err != nil {
-		return fmt.Errorf("cannot rewind charm archive: %v", err)
+		return errgo.Annotate(err, "cannot rewind charm archive")
 	}
 
 	// Get the environment storage and upload the charm.
 	env, err := environs.New(envConfig)
 	if err != nil {
-		return fmt.Errorf("cannot access environment: %v", err)
+		return errgo.Annotate(err, "cannot access environment")
 	}
 	storage := env.Storage()
 	archiveName, err := CharmArchiveName(charmURL.Name, charmURL.Revision)
 	if err != nil {
-		return fmt.Errorf("cannot generate charm archive name: %v", err)
+		return errgo.Annotate(err, "cannot generate charm archive name")
 	}
 	if err := storage.Put(archiveName, archive, size); err != nil {
-		return fmt.Errorf("cannot upload charm to provider storage: %v", err)
+		return errgo.Annotate(err, "cannot upload charm to provider storage")
 	}
 	storageURL, err := storage.URL(archiveName)
 	if err != nil {
-		return fmt.Errorf("cannot get storage URL for charm: %v", err)
+		return errgo.Annotate(err, "cannot get storage URL for charm")
 	}
 	bundleURL, err := url.Parse(storageURL)
 	if err != nil {
-		return fmt.Errorf("cannot parse storage URL: %v", err)
+		return errgo.Annotate(err, "cannot parse storage URL")
 	}
 
 	// Finally, update the charm data in state and mark it as no longer pending.
@@ -903,7 +904,7 @@
 		// us. This means we have to delete what we just uploaded
 		// to storage.
 		if err := storage.Remove(archiveName); err != nil {
-			logger.Errorf("cannot remove duplicated charm from storage: %v", err)
+			errgo.Annotate(err, "cannot remove duplicated charm from storage")
 		}
 		return nil
 	}

=== modified file 'worker/firewaller/firewaller.go'
--- worker/firewaller/firewaller.go	2014-01-23 17:08:36 +0000
+++ worker/firewaller/firewaller.go	2014-02-04 06:44:26 +0000
@@ -4,14 +4,13 @@
 package firewaller
 
 import (
-	"fmt"
-
+	"github.com/errgo/errgo"
+	"github.com/loggo/loggo"
 	"launchpad.net/tomb"
 
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
-	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/names"
 	apifirewaller "launchpad.net/juju-core/state/api/firewaller"
 	"launchpad.net/juju-core/state/api/params"
@@ -20,6 +19,8 @@
 	"launchpad.net/juju-core/worker"
 )
 
+var logger = loggo.GetLogger("juju.worker.firewaller")
+
 // Firewaller watches the state for ports opened or closed
 // and reflects those changes onto the backing environment.
 type Firewaller struct {
@@ -93,7 +94,7 @@
 				return err
 			}
 			if err := fw.environ.SetConfig(config); err != nil {
-				log.Errorf("worker/firewaller: loaded invalid environment configuration: %v", err)
+				logger.Errorf("loaded invalid environment configuration: %v", err)
 			}
 		case change, ok := <-fw.machinesWatcher.Changes():
 			if !ok {
@@ -121,7 +122,7 @@
 		case change := <-fw.portsChange:
 			change.unitd.ports = change.ports
 			if err := fw.flushUnits([]*unitData{change.unitd}); err != nil {
-				return fmt.Errorf("cannot change firewall ports: %v", err)
+				return errgo.Annotate(err, "cannot change firewall ports")
 			}
 		case change := <-fw.exposedChange:
 			change.serviced.exposed = change.exposed
@@ -130,7 +131,7 @@
 				unitds = append(unitds, unitd)
 			}
 			if err := fw.flushUnits(unitds); err != nil {
-				return fmt.Errorf("cannot change firewall ports: %v", err)
+				return errgo.Annotate(err, "cannot change firewall ports")
 			}
 		}
 	}
@@ -139,7 +140,7 @@
 // stop a watcher with logging of a possible error.
 func stop(what string, stopper watcher.Stopper) {
 	if err := stopper.Stop(); err != nil {
-		log.Errorf("worker/firewaller: error stopping %s: %v", what, err)
+		logger.Errorf("error stopping %s: %v", what, err)
 	}
 }
 
@@ -156,7 +157,7 @@
 	if params.IsCodeNotFound(err) {
 		return nil
 	} else if err != nil {
-		return fmt.Errorf("worker/firewaller: cannot watch machine units: %v", err)
+		return errgo.Annotate(err, "cannot watch machine units")
 	}
 	unitw, err := m.WatchUnits()
 	if err != nil {
@@ -176,7 +177,7 @@
 		if err != nil {
 			stop("units watcher", unitw)
 			delete(fw.machineds, tag)
-			return fmt.Errorf("worker/firewaller: cannot respond to units changes for %q: %v", tag, err)
+			return errgo.Annotatef(err, "cannot respond to units changes for %q", tag)
 		}
 	}
 	go machined.watchLoop(unitw)
@@ -266,14 +267,14 @@
 	toOpen := Diff(wantedPorts, initialPorts)
 	toClose := Diff(initialPorts, wantedPorts)
 	if len(toOpen) > 0 {
-		log.Infof("worker/firewaller: opening global ports %v", toOpen)
+		logger.Infof("opening global ports %v", toOpen)
 		if err := fw.environ.OpenPorts(toOpen); err != nil {
 			return err
 		}
 		instance.SortPorts(toOpen)
 	}
 	if len(toClose) > 0 {
-		log.Infof("worker/firewaller: closing global ports %v", toClose)
+		logger.Infof("closing global ports %v", toClose)
 		if err := fw.environ.ClosePorts(toClose); err != nil {
 			return err
 		}
@@ -318,7 +319,7 @@
 		toOpen := Diff(machined.ports, initialPorts)
 		toClose := Diff(initialPorts, machined.ports)
 		if len(toOpen) > 0 {
-			log.Infof("worker/firewaller: opening instance ports %v for %q",
+			logger.Infof("opening instance ports %v for %q",
 				toOpen, machined.tag)
 			if err := instances[0].OpenPorts(machineId, toOpen); err != nil {
 				// TODO(mue) Add local retry logic.
@@ -327,7 +328,7 @@
 			instance.SortPorts(toOpen)
 		}
 		if len(toClose) > 0 {
-			log.Infof("worker/firewaller: closing instance ports %v for %q",
+			logger.Infof("closing instance ports %v for %q",
 				toClose, machined.tag)
 			if err := instances[0].ClosePorts(machineId, toClose); err != nil {
 				// TODO(mue) Add local retry logic.
@@ -361,7 +362,7 @@
 			if unit == nil || unit.Life() == params.Dead || machineTag != knownMachineTag {
 				fw.forgetUnit(unitd)
 				changed = append(changed, unitd)
-				log.Debugf("worker/firewaller: stopped watching unit %s", name)
+				logger.Debugf("stopped watching unit %s", name)
 			}
 		} else if unit != nil && unit.Life() != params.Dead && fw.machineds[machineTag] != nil {
 			err = fw.startUnit(unit, machineTag)
@@ -369,11 +370,11 @@
 				return err
 			}
 			changed = append(changed, fw.unitds[name])
-			log.Debugf("worker/firewaller: started watching unit %s", name)
+			logger.Debugf("started watching unit %s", name)
 		}
 	}
 	if err := fw.flushUnits(changed); err != nil {
-		return fmt.Errorf("cannot change firewall ports: %v", err)
+		return errgo.Annotate(err, "cannot change firewall ports")
 	}
 	return nil
 }
@@ -442,7 +443,7 @@
 			return err
 		}
 		instance.SortPorts(toOpen)
-		log.Infof("worker/firewaller: opened ports %v in environment", toOpen)
+		logger.Infof("opened ports %v in environment", toOpen)
 	}
 	if len(toClose) > 0 {
 		if err := fw.environ.ClosePorts(toClose); err != nil {
@@ -450,7 +451,7 @@
 			return err
 		}
 		instance.SortPorts(toClose)
-		log.Infof("worker/firewaller: closed ports %v in environment", toClose)
+		logger.Infof("closed ports %v in environment", toClose)
 	}
 	return nil
 }
@@ -490,7 +491,7 @@
 			return err
 		}
 		instance.SortPorts(toOpen)
-		log.Infof("worker/firewaller: opened ports %v on %q", toOpen, machined.tag)
+		logger.Infof("opened ports %v on %q", toOpen, machined.tag)
 	}
 	if len(toClose) > 0 {
 		if err := instances[0].ClosePorts(machineId, toClose); err != nil {
@@ -498,7 +499,7 @@
 			return err
 		}
 		instance.SortPorts(toClose)
-		log.Infof("worker/firewaller: closed ports %v on %q", toClose, machined.tag)
+		logger.Infof("closed ports %v on %q", toClose, machined.tag)
 	}
 	return nil
 }
@@ -522,7 +523,7 @@
 		if err != nil {
 			return err
 		}
-		log.Debugf("worker/firewaller: started watching %q", tag)
+		logger.Debugf("started watching %q", tag)
 	}
 	return nil
 }
@@ -539,7 +540,7 @@
 	if err := machined.Stop(); err != nil {
 		return err
 	}
-	log.Debugf("worker/firewaller: stopped watching %q", machined.tag)
+	logger.Debugf("stopped watching %q", machined.tag)
 	return nil
 }
 
@@ -549,7 +550,7 @@
 	serviced := unitd.serviced
 	machined := unitd.machined
 	if err := unitd.Stop(); err != nil {
-		log.Errorf("worker/firewaller: unit watcher %q returned error when stopping: %v", name, err)
+		logger.Errorf("unit watcher %q returned error when stopping: %v", name, err)
 	}
 	// Clean up after stopping.
 	delete(fw.unitds, name)
@@ -558,7 +559,7 @@
 	if len(serviced.unitds) == 0 {
 		// Stop service data after all units are removed.
 		if err := serviced.Stop(); err != nil {
-			log.Errorf("worker/firewaller: service watcher %q returned error when stopping: %v", serviced.service.Name(), err)
+			logger.Errorf("service watcher %q returned error when stopping: %v", serviced.service.Name(), err)
 		}
 		delete(fw.serviceds, serviced.service.Name())
 	}