Improve error handling, fix #416 #462
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
concaf
force-pushed
the
improve_error_handling
branch
4 times, most recently
from
7b0d5e8
to
430e6fc
Compare
concaf
changed the title
concaf
force-pushed
the
improve_error_handling
branch
from
0c9071d
to
24a3ecf
Compare
| komposeObject := kobject.KomposeObject{ | ||
| ServiceConfigs: make(map[string]kobject.ServiceConfig), | ||
| LoadedFrom: "bundle", | ||
| } | ||
| file := files[0] | ||
| buf, err := ioutil.ReadFile(file) | ||
| if err != nil { | ||
| log.Fatalf("Failed to read bundles file: %s ", err) | ||
| return kobject.KomposeObject{}, errors.Wrap(err, "ioutil.ReadFile failed, Failed to read bundles file") |
There was a problem hiding this comment.
Just curious: Is it good practice to send the method/function name in error?
There was a problem hiding this comment.
Also follow up question here you are using the error message as ioutil.ReadFile failed which is kinda absolute name but below at line#191 you have just used the name which is loadFile, why is that so?
There was a problem hiding this comment.
@surajssd about the good practice part, it's a very subjective opinion, but we can use it to track down the error when the error messages will get printed in app.go
and about the the error message part, I added the name of the failing functions wrt the function they were failing in, so it gets easier to track down the error.
|
Good job man! Did a review and the code LGTM 👍 Just need to rebase against master (I updated the vendoring in a different PR) |
concaf
force-pushed
the
improve_error_handling
branch
from
80a9140
to
7fd2c4a
Compare
This commit refactors the code to remove more or less
all occurences of logrus.Fatalf() from the code under
pkg/ except for app.go where all the errors are being
handled currently.
This is being done since random logrus.Fatalf() calls
all around the code was making handling the errors,
unit testing and troubleshooting a bit more painful.
logrus.Fatalf() calls are either replaced by
return errors.New("new error")
or
return errors.Wrap(err, "annonate error")
calls, and the function signatures are accordingly
changed to accomodate the new return values.
The unit tests which previously used to check
if logrus.Fatalf() calls worked fine have also
been fixed to only check for errors now.
Fixes kubernetes#416
concaf
force-pushed
the
improve_error_handling
branch
3 times, most recently
from
dc7cf61
to
5eae411
Compare
|
@cdrage fixed! :) |
pkg/app/app.go
Outdated
| @@ -35,6 +35,7 @@ import ( | |||
|
|
|||
| "os" | |||
|
|
|||
| "github.com/fsouza/go-dockerclient/external/github.com/Sirupsen/logrus" | |||
There was a problem hiding this comment.
why not import github.com/Sirupsen/logrus instead of from fsouza
There was a problem hiding this comment.
Whoops, never realized that logrus was aliased to log, so my editor ended up importing this. Fixed! Nice catch, man :)
This adds github.com/pkg/errors to glide.yaml followed by glide and glide-vc commands. The github.com/pkg/errors package is currently required mainly for the errors.Wrap() and errors.New() methods, since this lets us to annotate the errors while passing the error message up the call stack.
concaf
force-pushed
the
improve_error_handling
branch
from
5eae411
to
e9544ca
Compare
|
@cdrage fixed. |
|
LGTM! 👍 |
No description provided.