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

Fix merge issue tty #4882

Merged
merged 2 commits into from May 16, 2014
Merged

Fix merge issue tty #4882

merged 2 commits into from May 16, 2014

Conversation

vieux
Copy link
Contributor

@vieux vieux commented Mar 27, 2014

fix #4857
If you run a container with -t and then commit it. If you start to run an another container based on this image without -t you'll get an error, because the client will expect a no-tty communication, but the container will inherit from the image tty

One solution (this PR) is to not merge the -i and -tty stuff from parent.

Another solution would be to fall back to display a warning and prevent the run if the client/deamon aren't in the same mode.

/cc @creack @shykes @crosbymichael

@crosbymichael
Copy link
Contributor

I think this solution is good.

@creack
Copy link
Contributor

creack commented Mar 27, 2014

ping @shykes could you take look? Pretty huge change.

I think we should just return an error if the image requires tty but the client didn't provide it. Maybe even automatically set the tty if the image need it, but it would be magical.

However, because of the automatic merge, a lot of images has been created with 'requires a tty' where it does not..

@thaJeztah
Copy link
Member

From a user perspective, outputting some information would be welcome. I don't know if containers will always fail if tty is missing, so refusing to run may not be necessary in all cases? If a message is shown that the container may be requiring a tty, this will assist the user in resolving the problem if the container fails to run properly.

Something like; this container was built using a TTY, which is not currently enabled. Add the -t to your run command to enable TTY

@crosbymichael
Copy link
Contributor

I'm voting for this approach because right now there is no such thing as saying "this image requires a tty"

@vieux vieux added this to the 1.0 milestone May 15, 2014
@shykes
Copy link
Contributor

shykes commented May 16, 2014

(sorry I posted this in the wrong issue last week...)

I agree with this as an immediate solution.. I prefer moving away from "magical merging"
based on the commands you ran before. I think it's cleaner to unify the build interface,
so that you can explicitly tell Docker "please modify the image metadata in this or that way".
That's consistent with my docker commit --change PR for example.

Longer term, it would be nice to have a TTY build instruction to specify "the default entrypoint
should have a tty allocated". There are 2 caveats there:

  1. Note that TTY should apply to the entrypoint, and not apply if the entrypoint is changed
    (either at run time or by an overlay image). It makes sense to say "by default run bash with a tty",
    but it doesn't make sense to say "whatever command you run in this container, always allocate a tty".

  2. TTY is complicated to implement because it requires cooperation by the client, unlike most
    other build instructions. I am not sure how that work, but I have a feeling it's complicated :)

I will leave this long-term discussion for later.

@shykes
Copy link
Contributor

shykes commented May 16, 2014

(also posted this in the wrong issue last weeek...)

@vieux could you move the test from integration to integration-cli? It should make it
even simpler :)

After that I think we can merge.

Sorry for the slow review.

vieux added 2 commits May 16, 2014 22:23
Docker-DCO-1.1-Signed-off-by: Victor Vieux <victor.vieux@docker.com> (github: vieux)
Docker-DCO-1.1-Signed-off-by: Victor Vieux <victor.vieux@docker.com> (github: vieux)
@crosbymichael
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request May 16, 2014
@crosbymichael crosbymichael merged commit 58b49e6 into moby:master May 16, 2014
@vieux vieux deleted the fix_merge_issue_tty branch May 16, 2014 22:42
brownjohnf added a commit to balena-os/balena-yocto-scripts that referenced this pull request Jun 7, 2017
Based on the threads listed below, this should resolve the issue, which
is related to building an image with the interactive terminal flag (-t)
but then running it without the flag.

moby/moby#4882
moby/moby#4857
moby/moby#5327

Change-Type: patch
floion pushed a commit to balena-os/balena-yocto-scripts that referenced this pull request Jun 21, 2017
Based on the threads listed below, this should resolve the issue, which
is related to building an image with the interactive terminal flag (-t)
but then running it without the flag.

moby/moby#4882
moby/moby#4857
moby/moby#5327

Change-Type: patch
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.

Client issue on command failure
5 participants