-
Notifications
You must be signed in to change notification settings - Fork 443
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(cli): properly shutdown subprocesses on abort #1605
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Mar 1, 2022
DanielMSchmidt
force-pushed
the
add-abort-handling
branch
2 times, most recently
from
March 2, 2022 15:51
94b237b
to
c4f82f5
Compare
DanielMSchmidt
changed the base branch from
main
to
detailed-diff-streaming-terraform-output
March 2, 2022 15:52
DanielMSchmidt
force-pushed
the
detailed-diff-streaming-terraform-output
branch
from
March 2, 2022 15:57
c4eca9c
to
c74b507
Compare
DanielMSchmidt
force-pushed
the
add-abort-handling
branch
2 times, most recently
from
March 2, 2022 16:16
872545b
to
e58784f
Compare
ansgarm
approved these changes
Mar 2, 2022
DanielMSchmidt
force-pushed
the
detailed-diff-streaming-terraform-output
branch
4 times, most recently
from
March 3, 2022 12:04
a7e9912
to
b1eec1c
Compare
Base automatically changed from
detailed-diff-streaming-terraform-output
to
main
March 3, 2022 12:32
This is needed for node 14 and can be dropped once we upgrade to node 16
DanielMSchmidt
force-pushed
the
add-abort-handling
branch
3 times, most recently
from
March 3, 2022 14:33
273d401
to
fa59466
Compare
AbortController shipped in node 14.17, but only with an experimental feature flag. This is hard to set and node 14 is LTS so we don't want to update to node 16, therefore this polyfill will help us in the meantime.
DanielMSchmidt
force-pushed
the
add-abort-handling
branch
from
March 3, 2022 15:42
fa59466
to
bfe8ce5
Compare
Merged
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR ensures that when we receive a termination signal we stop all corresponding node processes. I'd consider it out of scope to stop / delete terraform cloud runs. I'm not even sure if stopping requests for TFC is worth it.
Closes #771
You can see in the watch tab below how the terraform apply command is aborted
Created #1607 as follow up to abort TFC runs