Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve error handling #416

Closed
kadel opened this issue Feb 8, 2017 · 6 comments
Closed

Improve error handling #416

kadel opened this issue Feb 8, 2017 · 6 comments
Labels
priority/P1

Comments

@kadel
Copy link
Member

kadel commented Feb 8, 2017

In some places like for example here we are printing errors using logrus.Fatalf deep inside code.

It would be better if function returned error that can be processed outside of this function (higher in call stack). Code will be cleaner and it will be easier to write unit tests.

I think that we should invest some time in cleaning all error handling in Kompose codebase.
@kadel kadel mentioned this issue Feb 8, 2017
Merged
@procrypt
Copy link

procrypt commented Feb 9, 2017

Printing errors using logrus.Fatalf, will need unit test to run a seperate subprocess to test the functionality of the code.

@surajssd
Copy link
Member

surajssd commented Feb 9, 2017

@procrypt I think what @kadel is trying to say here is that instead of any function doing random logrus.Fatalf we just change that function to return errors and the function calling it should gracefully do logrus.Fatalf with all the resources giving back to the pool if there are any.

@procrypt
Copy link

procrypt commented Feb 9, 2017 •
edited
Loading

@surajssd Yeah I got the point, what I am trying to say is that otherwise we need to do it in this way, which might be a bit convoluted.

@surajssd
Copy link
Member

surajssd commented Feb 9, 2017 •
edited
Loading

@surajssd Yeah I got the point, what I am trying to say is that otherwise we need to do it in this way, which might be a bit convoluted.

@procrypt this is self pointing here.

@concaf
Copy link
Contributor

concaf commented Feb 14, 2017

@kadel taking this up!

@concaf
Copy link
Contributor

concaf commented Mar 1, 2017 •
edited
Loading

FYI: I have done some work at #462

Also, I will be using the github.com/pkg/errors package to wrap and push the errors up the call stack.

Just keeping everyone in the loop.

concaf added a commit to concaf/kompose that referenced this issue Mar 7, 2017
@concaf
return errors instead of logrus.Fatal calls
24a3ecf 
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 added a commit to concaf/kompose that referenced this issue Mar 15, 2017
@concaf
return errors instead of logrus.Fatal calls
7fd2c4a 
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
@cdrage cdrage closed this as completed in 5cb598f Mar 15, 2017
cdrage added a commit that referenced this issue Mar 15, 2017
@cdrage
Merge pull request #462 from containscafeine/improve_error_handling
54f1339 
Improve error handling, fix #416
@cdrage cdrage mentioned this issue Mar 15, 2017
Merged
procrypt pushed a commit to procrypt/kompose that referenced this issue Mar 20, 2017
@concaf
return errors instead of logrus.Fatal calls
7b96805 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
priority/P1
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@kadel @surajssd @concaf @procrypt