[VC-35738] Remove os.Exit from the logs module#611
Conversation
| logs.Initialize() | ||
| PersistentPreRunE: func(cmd *cobra.Command, args []string) error { | ||
| return logs.Initialize() | ||
| }, |
There was a problem hiding this comment.
Returning the error here means that the error can be logged in the same format as all the other errors that are returned by the command.
| klog.ErrorS(err, "Exiting due to error", "exit-code", exitCode) | ||
| klog.FlushAndExit(time.Second, exitCode) | ||
| } | ||
| } |
There was a problem hiding this comment.
These changes are intended to make the logs test fixture parse the flags and behave the same way as Cobra.
- --help / -h results in stdout and exit code 0
- Unrecognized flags result in an error which is logged to stderr with exit code 1
There was a problem hiding this comment.
Thanks for the extra context. I think it would be worth adding a comment above to keep that in mind in the future. I'm already imagining myself in a year: why do we do this weird dance with the exit codes in the test code?
There was a problem hiding this comment.
I'll merge this now and add the comment in one of the followup PRS.
3d4876b to
a8f16c7
Compare
…e realistic Signed-off-by: Richard Wall <richard.wall@venafi.com>
a8f16c7 to
c3b5306
Compare
maelvls
left a comment
There was a problem hiding this comment.
This sounds like a good change. I haven't tested the change manually nor have I run the automated tests, I trust you.
Instead of calling os.exit directly after parsing and finding an error in the logging flags, the error is now returned and handled the same way as errors that come from the
Runfunction....they are logged using structured logging and the logs are flushed before the process exits.I've adjusted the logs tests accordingly and modified them so that the log flag parsing and error handling in the tests matches the behaviour of the actual command.
Before:
After: