-
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] Remove os.Exit from the logs module #611
Conversation
PersistentPreRun: func(cmd *cobra.Command, args []string) { | ||
logs.Initialize() | ||
PersistentPreRunE: func(cmd *cobra.Command, args []string) error { | ||
return logs.Initialize() | ||
}, |
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.
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.
exitCode := 1 | ||
klog.ErrorS(err, "Exiting due to error", "exit-code", exitCode) | ||
klog.FlushAndExit(time.Second, exitCode) | ||
} | ||
} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 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
Run
function....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: