-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: dim logs on shutdown #3235
Conversation
a325a07
to
92592ca
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.
Thanks for this @joske, I'll try to run this later today. Is it possible to add an explicit message in the terminal to wait for shutdown to complete?
8003421
to
489bdae
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.
LGTM with a few nits.
15b11b7
to
de654b3
Compare
CircleCi is having a moment, that failure is unrelated to the changes. Let's try rerunning a little later. |
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.
LGTM (also tested locally) 👍
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.
The initial message needs to be a bit more clear and should probably be highlighted or bolded. I've included a suggestion for wording.
Also:
- During shutdown, are any further SIGINT/SIGHUP/SIGTERM signals caught & ignored so that graceful shutdown can continue?
- I think this work is a good start, but in a follow-on PR we should maybe consider launching a display which displays graceful shutdown warnings more prominently.
This PR implements log dimming (if running in TTY mode) on shutdown (CTRL-C handler).
I've moved the shutdown flag earlier in the startup so it can be reused for the log dimming too.
There is some performance impact as it needs to check the boolean for every log event (and an extra method call).