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 logs with tty #2499

Merged
merged 2 commits into from Nov 4, 2013
Merged

Fix logs with tty #2499

merged 2 commits into from Nov 4, 2013

Conversation

@vieux
Copy link
Contributor

vieux commented Nov 1, 2013

Fixes #2414

It would be nice to reformat the whole logging system at some point, but for now, this fix will do.

@vieux

This comment has been minimized.

Copy link
Contributor Author

vieux commented Nov 1, 2013


if err := cli.hijack("POST", "/containers/"+name+"/attach?logs=1&stdout=1&stderr=1", false, nil, cli.out, cli.err, nil); err != nil {
if err := cli.hijack("POST", "/containers/"+name+"/attach?logs=1&stdout=1&stderr=1", container.Config.Tty, nil, cli.out, cli.err, nil); err != nil {

This comment has been minimized.

Copy link
@crosbymichael

crosbymichael Nov 1, 2013

Member

Why does this matter?

This comment has been minimized.

Copy link
@vieux

vieux Nov 1, 2013

Author Contributor

Because in hijack we use stdCopy instead of io.Copy if we are in ttymode.

This comment has been minimized.

Copy link
@vieux

vieux Nov 1, 2013

Author Contributor

try with #2414

@creack

This comment has been minimized.

Copy link
Contributor

creack commented Nov 4, 2013

Nice catch. I missed that one. @vieux could you add a test for CmdLog as well?

@crosbymichael

This comment has been minimized.

Copy link
Member

crosbymichael commented Nov 4, 2013

LGTM

@vieux don't forget about adding a test to CmdLogs ;)

crosbymichael added a commit that referenced this pull request Nov 4, 2013
@crosbymichael crosbymichael merged commit 57cd17f into master Nov 4, 2013
@crosbymichael crosbymichael deleted the 2414-logs_tty-fix branch Nov 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.