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

feat: add -t and -i flags to docker run command so that docker contai… #66

Merged
merged 5 commits into from
Sep 28, 2023

Conversation

MSevey
Copy link
Contributor

@MSevey MSevey commented Sep 20, 2023

…ner can be stopped with crtl c

Overview

Close #65

REF: https://forums.docker.com/t/docker-run-cannot-be-killed-with-ctrl-c/13108/4

  -i, --interactive                    Keep STDIN open even if not attached
  -t, --tty                            Allocate a pseudo-TTY

@MSevey MSevey linked an issue Sep 20, 2023 that may be closed by this pull request
@MSevey MSevey self-assigned this Sep 20, 2023
jcstein
jcstein previously approved these changes Sep 20, 2023
Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

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

utACK

@nashqueue
Copy link
Member

          I think being able to stop it is useful with CTRL+C. however, it is also useful to be able to keep the container running if you exit the terminal. so, if being able to stop with CTRL+C doesn't also stop the container when you exit the terminal I think the PR is desirable

Originally posted by @jcstein in #65 (comment)

Cross-posting here to make sure it keeps running when you close the terminal

Manav-Aggarwal
Manav-Aggarwal previously approved these changes Sep 20, 2023
@smuu smuu dismissed stale reviews from Manav-Aggarwal and jcstein via 005dddf September 21, 2023 08:57
@nashqueue
Copy link
Member

If you want to run a container in the background (no opened terminal required), use the parameter -d. You can still get the logs with docker logs ... and attach a terminal to a running container using docker attach ....

Originally posted by @smuu in #65 (comment)

Should we Add the flag -d ?

nashqueue
nashqueue previously approved these changes Sep 21, 2023
Copy link
Member

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

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

utAck

@MSevey MSevey enabled auto-merge September 22, 2023 19:38
@smuu
Copy link
Contributor

smuu commented Sep 25, 2023

If you want to run a container in the background (no opened terminal required), use the parameter -d. You can still get the logs with docker logs ... and attach a terminal to a running container using docker attach ....

Originally posted by @smuu in #65 (comment)

Should we Add the flag -d ?

I don't think -d should be added by default. This will confuse people running this. Maybe we can add a comment that if someone wants to run it in the background, they can start it with the argument added.

@MSevey
Copy link
Contributor Author

MSevey commented Sep 25, 2023

If you want to run a container in the background (no opened terminal required), use the parameter -d. You can still get the logs with docker logs ... and attach a terminal to a running container using docker attach ....
Originally posted by @smuu in #65 (comment)
Should we Add the flag -d ?

I don't think -d should be added by default. This will confuse people running this. Maybe we can add a comment that if someone wants to run it in the background, they can start it with the argument added.

defer to @jcstein on this

@jcstein
Copy link
Member

jcstein commented Sep 27, 2023

I think I will follow the advice from @smuu and can add a note to the docs on using -d flag

@smuu can you confirm that the use of the -i and -t flags is something you think is worth adding?

@MSevey
Copy link
Contributor Author

MSevey commented Sep 28, 2023

I think I will follow the advice from @smuu and can add a note to the docs on using -d flag

@smuu can you confirm that the use of the -i and -t flags is something you think is worth adding?

If we no longer need to copy anything from the logs (which is a UX we should maintain imo) then we should add these flags so that users can stop the docker container with crtl c, since that is a standard practice.

Otherwise users need to detach from the container and use docker stop.

Either way we should include how to stop the container in the README.

I'll update with my proposed solution.

Copy link
Member

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: nashqueue <99758629+nashqueue@users.noreply.github.com>
Copy link
Member

@nashqueue nashqueue left a comment

Choose a reason for hiding this comment

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

utAck

Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

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

ACK

@MSevey MSevey merged commit 2fd64a0 into main Sep 28, 2023
@MSevey MSevey deleted the 65-ctrl+c-doesnt-exit-devnet branch September 28, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ctrl+c doesn't exit devnet
5 participants