Skip to content
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

APT-703 Properly Kill Subprocesses With ctrl-c #81

Merged
merged 14 commits into from
Oct 9, 2023
Merged

Conversation

afujiwara-roblox
Copy link
Collaborator

@afujiwara-roblox afujiwara-roblox commented Oct 4, 2023

Running commands like "rojo serve" and pressing ctrl-c does not kill the rojo process as intended. Foreman should pass the signal to the tools it runs. This PR implements the same solution from Aftman

Manually tested ctrl-c kills subprocesses on M2 mac and Windows machines
Added 2 scripts to CI to test subprocess are properly killed on linux and windows machines

@afujiwara-roblox afujiwara-roblox marked this pull request as ready for review October 4, 2023 18:04
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I think the script-driven e2e tests are reasonable, this is a difficult thing to pin with tests anyways.

@afujiwara-roblox afujiwara-roblox linked an issue Oct 9, 2023 that may be closed by this pull request
@afujiwara-roblox afujiwara-roblox merged commit 9b17551 into main Oct 9, 2023
10 checks passed
@afujiwara-roblox afujiwara-roblox deleted the APT-703 branch October 9, 2023 20:32
@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Foreman does not properly kill long-running processes
2 participants