Code review comment for lp:~hduran-8/goose/testservice_errors

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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

https://codereview.appspot.com/99960043/diff/1/testservices/errors.go#newcode5
testservices/errors.go:5: var nameReference = map[int]string{
Where do these values come from? I assume these are meant to match what
OpenStack returns exactly? If so, can you refer to some official
documentation?

If they're *not* meant to be identical to OpenStack, then this is
duplication of code in net/http.

https://codereview.appspot.com/99960043/diff/1/testservices/errors.go#newcode19
testservices/errors.go:19: type ServerError struct {
Does it perhaps make sense to expose more of goose/errors and use the
existing error types that would be returned by the real OpenStack?

https://codereview.appspot.com/99960043/diff/1/testservices/errors.go#newcode60
testservices/errors.go:60: // XXX: hduran-8 I infered this from the
python nova code, might be wrong
XXX is not typical to Juju or related projects. Use something like
"TODO(hduran-8) this is inferred from the Python nova code and might be
wrong; validate".

For most cases, we have a common format:
     TODO(nick) YYYY-MM-DD #bugid
     Summary.

and file a bug.

https://codereview.appspot.com/99960043/diff/1/testservices/errors.go#newcode66
testservices/errors.go:66: return &ServerError{
I would suggest a minor refactoring to simplify all of these functions:

func serverErrorf(code int, message string, args ...interface{})
*ServerError {
     return &ServerError{code: code, message: fmt.Sprintf(message,
args...)}
}

func NewAddFlavorError(id string) *ServerError {
     return serverErrorf(409, "A flavor with id %q already exists", id)
}

...

https://codereview.appspot.com/99960043/diff/1/testservices/errors.go#newcode67
testservices/errors.go:67: message: fmt.Sprintf("A flavor with id %q
already exists", id),
Do these error messages come from OpenStack, or did you make them up? We
typically start all error messages lower-case.

https://codereview.appspot.com/99960043/diff/1/testservices/errors_test.go
File testservices/errors_test.go (right):

https://codereview.appspot.com/99960043/diff/1/testservices/errors_test.go#newcode20
testservices/errors_test.go:20: // _, ok := err.(*error)
delete me

https://codereview.appspot.com/99960043/diff/1/testservices/errors_test.go#newcode30
testservices/errors_test.go:30: // _, ok := err.(*error)
delete me

https://codereview.appspot.com/99960043/

« Back to merge proposal