-
Notifications
You must be signed in to change notification settings - Fork 6k
feat: relaunch on SIGUSR2 #4979
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
Conversation
This is because Node uses SIGUSR1 to enable the debug listener so even if you just want to restart code-server you end up enabling the debug listener as well. Opted to leave the SIGUSR1 handler in to avoid breaking existing workflows even though it does mean even if you only want to enable the debug listener you will end up restarting code-server as well. We could consider removing it after a transition phase.
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.
Nice fix! What prompted this btw?
Codecov Report
@@ Coverage Diff @@
## main #4979 +/- ##
==========================================
- Coverage 71.75% 71.62% -0.13%
==========================================
Files 30 30
Lines 1685 1688 +3
Branches 375 375
==========================================
Hits 1209 1209
- Misses 407 410 +3
Partials 69 69
Continue to review full report at Codecov.
|
This has been mostly an internal thing so I have just lived with the debugger turning on but today I was asked how to restart code-server without rebuilding a workspace and it felt bad that I had to say "here is how...but the debugger will turn on" so I decided to fix it real swift. 😄 |
It has been on my mind for a very long time, should have done this ages ago. |
😧 |
lol would probably be good to add a test for relaunching |
bump @code-asher what should we do about this? We could merge and disregard the test coverage drop, up to you. |
I was hoping to add tests although it will not actually increase coverage I guess because we have to spawn a separate process to test it. |
Up to you then! I mean we can always add tests in another place to keep coverage high |
@code-asher not to necrobump this, but what should we do with this? |
Best options right now are probably to merge as-is or close; unlikely I can add tests right now 😢 |
I'm okay with merging as is if you are! |
That sounds good to me! |
This is because Node uses SIGUSR1 to enable the debug listener so even
if you just want to restart code-server you end up enabling the debug
listener as well.
Opted to leave the SIGUSR1 handler in to avoid breaking existing
workflows even though it does mean even if you only want to enable the
debug listener you will end up restarting code-server as well. We could
consider removing it after a transition phase.