-
Notifications
You must be signed in to change notification settings - Fork 25
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.
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
[VC-35738] Bubble up the errors from sub-commands and handle errors centrally instead of using log.Fatal at multiple sites #599
Conversation
// Usage information is still available on stdout with the `-h` and `--help` | ||
// flags. | ||
SilenceErrors: true, | ||
SilenceUsage: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested by @maelvls in #593 (comment)
4438c98
to
817f07c
Compare
54dc42e
to
864edd3
Compare
35d6638
to
23579e7
Compare
23579e7
to
708b1d7
Compare
b4ee1ac
to
704847d
Compare
Signed-off-by: Richard Wall <richard.wall@venafi.com>
…up the stderr output Signed-off-by: Richard Wall <richard.wall@venafi.com>
Signed-off-by: Richard Wall <richard.wall@venafi.com>
704847d
to
045142c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems all good to me. I haven't tested this manually, but I trust the output that you gave.
err = multierror.Append( | ||
err, | ||
fmt.Errorf("failed to wait for controller-runtime component to stop: %v", groupErr), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused a bug.
The group error is discarded, because I was assigning to the err
variable assuming that it was the returned error, but it's not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #606
This is a small part of a larger effort to allow the agent to use structured logging.
This will make it easier for platform administrators to parse the logs and to distinguish errors from info messages.
It also is a small part of an effort to use contextual logging instead of the legacy
log
module.This will make it easier to test the log output of our code in unit tests.
The existing code uses
log.Fatal
at the site of various errors to both print the error and to exit the process with code 1.Instead the errors should be returned and bubbled back to the root command, which makes it easier to handle the errors in a consistent fashion and makes it easier to test the code that had previously caused the process to exit.
$ git grep -i '\.fatal' | grep -v 't\.Fatal'
Example output: