From ab6e2e802119a08a0e5b6208de0f06e7cf98ee36 Mon Sep 17 00:00:00 2001 From: Toby Crawley Date: Thu, 16 Feb 2017 16:58:59 -0500 Subject: [PATCH] Improve command-line client error output (#122) Motivation: The output when an error occurs shows an integer error code, which isn't very helpful. And in the case of an error when creating a resource, you also get a redundant log message from the controller client. Modifications: * the controller client no longer logs errors * the generated String() allows for the enum name to be displayed in the error output * each errorCode enum member has a hand-generated description * the format of the error message from the CLI client was modified to display the error description with the error message Related to #112 --- controller/client/client.go | 14 ----------- controller/resourceStore.go | 47 ++++++++++++++++++++++--------------- error.go | 12 ++++++++-- types.go | 12 ++++++++++ 4 files changed, 50 insertions(+), 35 deletions(-) diff --git a/controller/client/client.go b/controller/client/client.go index 8f6fe077..c6568e03 100644 --- a/controller/client/client.go +++ b/controller/client/client.go @@ -26,8 +26,6 @@ import ( "net/http" "strings" - log "github.com/Sirupsen/logrus" - "github.com/fission/fission" ) @@ -104,10 +102,6 @@ func (c *Client) FunctionCreate(f *fission.Function) (*fission.Metadata, error) body, err := c.handleResponse(resp) if err != nil { - log.WithFields(log.Fields{ - "name": f.Metadata.Name, - "err": err, - }).Error("Failed to create function") return nil, err } @@ -240,10 +234,6 @@ func (c *Client) HTTPTriggerCreate(t *fission.HTTPTrigger) (*fission.Metadata, e body, err := c.handleResponse(resp) if err != nil { - log.WithFields(log.Fields{ - "name": t.Metadata.Name, - "err": err, - }).Error("Failed to create http trigger") return nil, err } @@ -351,10 +341,6 @@ func (c *Client) EnvironmentCreate(env *fission.Environment) (*fission.Metadata, body, err := c.handleResponse(resp) if err != nil { - log.WithFields(log.Fields{ - "name": env.Metadata.Name, - "err": err, - }).Error("Failed to create environment") return nil, err } diff --git a/controller/resourceStore.go b/controller/resourceStore.go index 8553461d..93fc9f5d 100644 --- a/controller/resourceStore.go +++ b/controller/resourceStore.go @@ -18,7 +18,9 @@ package controller import ( "errors" + "fmt" "reflect" + "strings" "time" log "github.com/Sirupsen/logrus" @@ -26,7 +28,6 @@ import ( "github.com/satori/go.uuid" "golang.org/x/net/context" - "fmt" "github.com/fission/fission" ) @@ -96,7 +97,7 @@ func (rs *ResourceStore) create(r resource) error { _, err = rs.KeysAPI.Set(context.Background(), key, string(serialized), &client.SetOptions{PrevExist: client.PrevNoExist}) - return handleEtcdError(err) + return handleEtcdErrorForResource(err, r) } func (rs *ResourceStore) read(rkey string, res resource) error { @@ -108,7 +109,7 @@ func (rs *ResourceStore) read(rkey string, res resource) error { resp, err := rs.KeysAPI.Get(context.Background(), key, nil) if err != nil { - return handleEtcdError(err) + return handleEtcdError(err, typName, rkey) } return rs.serializer.deserialize([]byte(resp.Node.Value), res) } @@ -126,13 +127,13 @@ func (rs *ResourceStore) update(r resource) error { _, err = rs.KeysAPI.Set(context.Background(), key, string(serialized), &client.SetOptions{PrevExist: client.PrevExist}) - return handleEtcdError(err) + return handleEtcdErrorForResource(err, r) } func (rs *ResourceStore) delete(typename, rkey string) error { key := typename + "/" + rkey _, err := rs.KeysAPI.Delete(context.Background(), key, nil) // ignore response - return handleEtcdError(err) + return handleEtcdError(err, typename, rkey) } // getAll finds all entries under key. If none or found or key @@ -143,7 +144,7 @@ func (rs *ResourceStore) getAll(key string) ([]string, error) { if client.IsKeyNotFound(err) { return []string{}, nil } - return nil, handleEtcdError(err) + return nil, handleEtcdError(err, "", key) } res := make([]string, 0, len(resp.Node.Nodes)) @@ -165,7 +166,7 @@ func (rs *ResourceStore) writeFile(parentKey string, contents []byte) (string, s resp, err := rs.KeysAPI.CreateInOrder(context.Background(), parentKey, uid, nil) if err != nil { _ = rs.FileStore.delete(uid) - return "", "", handleEtcdError(err) + return "", "", handleEtcdError(err, "file", parentKey) } return resp.Node.Key, uid, nil @@ -175,7 +176,7 @@ func (rs *ResourceStore) readFile(key string, uid *string) ([]byte, error) { key = "file/" + key resp, err := rs.KeysAPI.Get(context.Background(), key, &client.GetOptions{Sort: true}) if err != nil { - return nil, handleEtcdError(err) + return nil, handleEtcdError(err, "file", key) } if uid == nil { @@ -197,14 +198,14 @@ func (rs *ResourceStore) readFile(key string, uid *string) ([]byte, error) { } contents, err := rs.FileStore.read(*uid) - return contents, handleEtcdError(err) + return contents, err } func (rs *ResourceStore) deleteFile(key string, uid string) error { key = "file/" + key resp, err := rs.KeysAPI.Get(context.Background(), key, &client.GetOptions{Sort: true}) if err != nil { - return handleEtcdError(err) + return handleEtcdError(err, "file", key) } var node *client.Node @@ -225,12 +226,12 @@ func (rs *ResourceStore) deleteFile(key string, uid string) error { _, err = rs.KeysAPI.Delete(context.Background(), node.Key, nil) if err != nil { - return handleEtcdError(err) + return handleEtcdError(err, "", node.Key) } if len(resp.Node.Nodes) == 1 { _, err = rs.KeysAPI.Delete(context.Background(), key, &client.DeleteOptions{Dir: true}) - return handleEtcdError(err) + return handleEtcdError(err, "file", key) } return nil } @@ -239,7 +240,7 @@ func (rs *ResourceStore) deleteAllFiles(key string) error { key = "file/" + key resp, err := rs.KeysAPI.Get(context.Background(), key, &client.GetOptions{Sort: true}) if err != nil { - return handleEtcdError(err) + return handleEtcdError(err, "file", key) } for _, u := range resp.Node.Nodes { err = rs.FileStore.delete(u.Value) @@ -249,30 +250,38 @@ func (rs *ResourceStore) deleteAllFiles(key string) error { _, err = rs.KeysAPI.Delete(context.Background(), u.Key, nil) if err != nil { - return handleEtcdError(err) + return handleEtcdError(err, "", u.Key) } } _, err = rs.KeysAPI.Delete(context.Background(), key, &client.DeleteOptions{Dir: true}) - return handleEtcdError(err) + return handleEtcdError(err, "file", key) } -func handleEtcdError(e error) error { +func handleEtcdErrorForResource(e error, r resource) error { + resourceType, _ := getTypeName(r) + return handleEtcdError(e, resourceType, r.Key()) +} + +func handleEtcdError(e error, resourceType string, resourceKey string) error { ee, ok := e.(client.Error) if !ok { return e } code := fission.ErrorInternal msg := ee.Error() - simpleMsg := fmt.Sprintf("%v (%v)", ee.Message, ee.Cause) + + if len(resourceType) > 0 { + resourceType = strings.ToLower(resourceType) + " " + } //TODO: handle any other etcd error codes we care about switch ee.Code { case client.ErrorCodeNodeExist: code = fission.ErrorNameExists - msg = simpleMsg + msg = fmt.Sprintf("%s'%s' already exists", resourceType, resourceKey) case client.ErrorCodeKeyNotFound: code = fission.ErrorNotFound - msg = simpleMsg + msg = fmt.Sprintf("%s'%s' does not exist", resourceType, resourceKey) } return fission.MakeError(code, msg) } diff --git a/error.go b/error.go index dfba53f3..ad40090e 100644 --- a/error.go +++ b/error.go @@ -23,8 +23,8 @@ import ( "strings" ) -func (e Error) Error() string { - return fmt.Sprintf("(Error %v) %v", e.Code, e.Message) +func (err Error) Error() string { + return fmt.Sprintf("%v - %v", err.Description(), err.Message) } func MakeError(code int, msg string) Error { @@ -90,3 +90,11 @@ func GetHTTPError(err error) (int, string) { } return code, msg } + +func (err Error) Description() string { + idx := int(err.Code) + if idx < 0 || idx > len(errorDescriptions)-1 { + return "" + } + return errorDescriptions[idx] +} diff --git a/types.go b/types.go index bfa67eab..3f7954f5 100644 --- a/types.go +++ b/types.go @@ -72,6 +72,7 @@ type ( Code errorCode `json:"code"` Message string `json:"message"` } + errorCode int ) @@ -85,3 +86,14 @@ const ( ErrorNoSpace ErrorNotImplmented ) + +// must match order and len of the above const +var errorDescriptions = []string{ + "Internal error", + "Not authorized", + "Resource not found", + "Resource exists", + "Invalid argument", + "No space", + "Not implemented", +}