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

Ensured -t flag was passed to mini script execution of the Hello World d... #2845

Closed
wants to merge 1 commit into from

Conversation

lhazlewood
Copy link

...aemon

The tutorial assumes detaching after attaching is possible, such that you can run sudo docker ps and see that the container is still running.

Prior to this edit, the tutorial instructed the reader to press Control-C to detach. As of Docker 0.6.5, this actually terminates the container. Running sudo docker ps next in the tutorial showed nothing and was confusing - it 'broke' the remainder of the tutorial.

Adding -t to docker run, the reader can exit via ctl-c and the remainder of the tutorial is possible.

…d daemon

The tutorial assumes detaching after attaching is possible, such that you can run `sudo docker ps` and see that the container is still running.  

Prior to this edit, the tutorial instructed the reader to press Control-C to detach.  As of Docker 0.6.5, this actually terminates the backing process.  Running `sudo docker ps` next in the tutorial showed nothing and was confusing - it 'broke' the remainder of the tutorial.

Adding `-t`, the reader can exit via ctl-p ctl-q and the remainder of the tutorial is possible.
@crosbymichael
Copy link
Contributor

ping @metalivedev

@creack
Copy link
Contributor

creack commented Nov 25, 2013

ping @shykes Wouldn't it be better to revert to the original Attach behavior?

@lhazlewood
Copy link
Author

@crosbymichael @metalivedev @shykes

I think most people feel that that if you run docker attach that Control-C should detach you and never shut down the container - in all cases. Stopping the container is what docker stop is for. It is important to have feature symmetry and not surprise people IMO.

I believe if people want to use Control-C to terminate a container, and want to use ctl-p ctl-q to detach, this should be explicitly specified when running docker run (i.e. the opposite of the 0.6.5-0.6.7 default behavior).

@shykes shykes mentioned this pull request Nov 25, 2013
12 tasks
@metalivedev
Copy link
Contributor

The doc change looks fine, but what is the correct behavior? Will we keep the Ctrl-C-kills-container, or not? That is, are we adapting to a bug here, or is this the new expected behavior?

@lhazlewood
Copy link
Author

@metalivedev My original PR was to correct the docs, but I believe the follow up comments better address the issue:

If possible, I strongly advocate that we keep the docs as they - ignoring this PR - and remove Ctrl-C-kills-container behavior.

It is my belief that end-users shouldn't be surprised by default behavior. When you docker attach, it is a surprise that Ctrl-C kills the container, because you expect the attachment to discontinue (i.e. 'detatch'), not stop the container entirely (that's what docker stop is for).

If the Docker team is open to this, how should we best represent this? Would you like me to open a new issue?

@metalivedev
Copy link
Contributor

It sounds like the right thing to do would be for you to close this PR (since you've concluded that the docs should stay as-are) and open a new bug report about the Ctrl-C behavior. If the discussion on that bug concludes that the current behavior is desired, then you can reopen this PR.

@lhazlewood
Copy link
Author

Sounds good - thanks

@lhazlewood
Copy link
Author

Issue created: #2855

This was referenced Nov 26, 2013
@urbandroid
Copy link

This should be default behaviour

@thaJeztah
Copy link
Member

@urbandroid please don't comment on pull requests that were closed 4 years ago, if you want to open a feature request; open an issue for it, but don't revive a 4 years old pull request. If you want to customize the key-combination for detaching, you can now do so in the CLI configuration file

@urbandroid
Copy link

@thaJeztah "but don't revive a 4 years old pull request" why is there any law against this? you guys love to repeat yourselves ? What is wrong with old? Calm the f down I just googled after weird unexpected behaviour appeared on my cli this thread was on top and i write a comment that is all. if you really like multi copies of same issue create on your spare time.

@vdemeester
Copy link
Member

@urbandroid please watch you language a bit. Commenting on closed pull-request is usually not the way forward, as most of people won't read closed pull-request and even if they do, it's a pull request that's against some piece of code that probably doesn't exists anymore. Pull-requests are not issues, they are a way to propose code change and discuss about these code changes. You could definitely open an issue, but it would just duplicate #2855. On the default, it really is just a matter of "point of view". Some people would argue the current behavior is a sane default, some wont — that's why it's configurable, to let user set their own default.

I'm locking this issue; the default key-combination is not going to change, and you can now set your own default using a configuration file #15666. I understand that for some people ^C is the preferred combination and they are now able to do so.

@moby moby locked and limited conversation to collaborators Feb 27, 2017
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.

7 participants