Code review comment for lp:~fwereade/juju-core/errors-cleanup

Revision history for this message
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://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/

« Back to merge proposal