Make architect dev more robust
#661
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
…ing containers running, run docker compose stop behind the scenes to clean up.
tjhiggins
reviewed
Aug 11, 2022
|
It looks like there's a bit of a race in In this example the containers weren't stopped at all. As we're kind of just wrapping the We can however wait until the containers are definitely started and have the SIGINT do nothing until we can successfully stop. |
…ally passing down the SIGINT from parent->child process for docker compose cmd. Add some special logic to handle fixing a case where a user can SIGINT docker compose in just the right place and leave their containers running unintentionally.
TylerAldrich
changed the title
architect dev ctrl+c and ctrl+break properly on Windows Powershell/cmdarchitect dev more robust
Similarly I get an issue when I start then shortly stop our own local stack while a container is in the middle of being restarted. Note that I had just removed all containers Our process ends, but the container remains running ryan@ryan-ThinkPad-P53s:~/Code/architect-cli$ docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
b77ab9c37071 bitnami/redis:5.0 "/opt/bitnami/script…" 4 minutes ago Up 4 minutes 0.0.0.0:50003->6379/tcp architect-cloud--api-redis-1 |
|
@ryan-cahill That issue I ran into as well and should be fixed by this commit here: 4e58ab1 (Ignore the debugging file I committed 😛) |
…into windows-dev-interrupt
…dows-dev-interrupt
github-actions bot
pushed a commit
that referenced
this pull request
Aug 15, 2022
## [1.22.1-rc.6](v1.22.1-rc.5...v1.22.1-rc.6) (2022-08-15) ### Bug Fixes * **dev:** Make `architect dev` more robust ([#661](#661)) ([eda1cff](eda1cff))
github-actions bot
pushed a commit
that referenced
this pull request
Aug 15, 2022
# [1.23.0](v1.22.1...v1.23.0) (2022-08-15) ### Bug Fixes * **cli:** Switch from postinstall to prepare ([ffac972](ffac972)) * **dev:** Don't show ping access logs unless they fail ([33dc9b4](33dc9b4)) * **dev:** Make `architect dev` more robust ([#661](#661)) ([eda1cff](eda1cff)) * **exec:** Fix issue with commands run in no-tty but with stdin available exiting prematurely due to stdin closing ([#656](#656)) ([06ef0df](06ef0df)) * **exec:** no local selection when docker is unavailable ([#662](#662)) ([e3bf33b](e3bf33b)) * **register:** Allow register without build ([#660](#660)) ([033f261](033f261)) ### Features * **link:** 473 list linked components ([#658](#658)) ([b40c347](b40c347))
|
🎉 This PR is included in version 1.23.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The changes in this PR include:
docker compose stopdetached: trueso the abovedocker compose stophappens and the chlid process doesn't automatically get the sigint.architect devcould leave you with still-running-containersdocker compose upis attaching to the logs and actually starting the containers, if you^C, docker compose leaves your containers hanging. This is a problem with the underlying docker compose command, so to fix this for Architect,^Cdoesn't do anything until logs start getting output. This is generally a very short amount of time after the "Attaching..." starts and without artificial delays I was only able to get this to reliably happen accidentally.^C, the container will restart after thestopis run and leaves you hanging indefinitely, because not all containers will stop. This was because we checked a condition at the top of a loop, but within the loopawait, so the result can change (Unless nodejs is using multiple threads which makes things more complicated, but still a race condition regardless 🤷♂️)note: the
require("readline").createInterfaceisn't necessary anymore. This was tested in both Powershell and CMD - node now supports automatically mapping ctrl+c -> SIGINT and ctrl+break -> SIGBREAK: nodejs/node-v0.x-archive#5054 (comment)SIGBREAK is a windows-only, listen-only signal that just signifies ctrl+break was used on Windows (see: https://nodejs.org/api/all.html#all_process_signal-events) - it has no affect on other platforms so we don't need to guard it behind
process.platform === 'win32'Catching SIGBREAK just helps us cleanly kill containers with both supported windows "interrupt"-like commands.
Testing Checklist: