Support terminal resizing for exec/attach/run

[ncdc]

Add support for terminal resizing for exec, attach, and run. Note that for Docker, exec sessions
inherit the environment from the primary process, so if the container was created with tty=false,
that means the exec session's TERM variable will default to "dumb". Users can override this by
setting TERM=xterm (or whatever is appropriate) to get the correct "smart" terminal behavior.

Fixes #2284

[ncdc]

@deads2k please review the revert commit and the 2 carry commits that replace it.

@openshift/ui-review please review the "Support Windows terminal resizing" commit to make sure that I got every place we potentially run the CLI and pass in stdin/stdout/stderr - we have to use term.StdStreams() instead to get the right handle for stdout on Windows.

@bparees please review the last commit that affects binary build streaming.

[ncdc]

@smarterclayton @sttts fyi

[ncdc]

@csrwng can you try this on Windows?

[bparees]

lgtm

[deads2k]

reverts and carrys lgtm.

Since they exist to allow the latest client perform exec against 3.0.0 server that hasn't had any updates for almost a year, I don't see reason to fix these again. Next time they need to be reverted, we can just drop them.

[csrwng]

@ncdc, I get the following with exec:

C:\Users\cewong\go\src\github.com\openshift\origin>oc exec simple-ruby-1-479pc -t -i -- bash
Unable to use a TTY - input is not a terminal or the right kind of file

(Same with powershell)

With oc rsh it doesn't give me a message, it just hangs there.

[csrwng]

The linux client works ok

[ncdc]

Investigating how to fix Windows

[ncdc]

For some reason using stdin from term.StdStreams on Windows breaks input with oc login - you see Username: and when you type, it doesn't echo. More fun stuff to dig into.

[ncdc]

So it turns out that what you get back in Windows from term.StdStreams() isn't compatible with things like bufio.Reader and bufio.Scanner. What Docker did where they require a prompt (docker login) is explicitly use os.Stdin.

I'm wondering now if maybe what @sttts had originally in his Windows work is what we should do - use os.Stdin/out/err everywhere, and only switch to using the values from term.StdStreams() for exec, run, and attach. What do you all think? @fabianofranz @smarterclayton @liggitt

[smarterclayton]

I would prefer to keep term.StdStreams isolated to only where it has to be.

On Fri, Jul 15, 2016 at 6:48 PM, Andy Goldstein [email protected]
wrote:

So it turns out that what you get back in Windows from term.StdStreams()
isn't compatible with things like bufio.Reader and bufio.Scanner. What
Docker did where they require a prompt (docker login) is explicitly use
os.Stdin.

I'm wondering now if maybe what @sttts https://github.com/sttts had
originally in his Windows work is what we should do - use os.Stdin/out/err
everywhere, and only switch to using the values from term.StdStreams()
for exec, run, and attach. What do you all think? @fabianofranz
https://github.com/fabianofranz @smarterclayton
https://github.com/smarterclayton @liggitt https://github.com/liggitt

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9878 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p-DMxwIqO87eeZJSlE9_UHy9LVMlks5qWA4ygaJpZM4JNeA1
.

[ncdc]

Yeah, that's my plan (and I'm going to do this in Kube so origin will get
it without any extra work).

On Monday, July 18, 2016, Clayton Coleman [email protected] wrote:

I would prefer to keep term.StdStreams isolated to only where it has to be.

On Fri, Jul 15, 2016 at 6:48 PM, Andy Goldstein <[email protected]
javascript:_e(%7B%7D,'cvml','[email protected]');>
wrote:

So it turns out that what you get back in Windows from term.StdStreams()
isn't compatible with things like bufio.Reader and bufio.Scanner. What
Docker did where they require a prompt (docker login) is explicitly use
os.Stdin.

I'm wondering now if maybe what @sttts https://github.com/sttts had
originally in his Windows work is what we should do - use
os.Stdin/out/err
everywhere, and only switch to using the values from term.StdStreams()
for exec, run, and attach. What do you all think? @fabianofranz
https://github.com/fabianofranz @smarterclayton
https://github.com/smarterclayton @liggitt <https://github.com/liggitt

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9878 (comment),
or mute the thread
<
https://github.com/notifications/unsubscribe-auth/ABG_p-DMxwIqO87eeZJSlE9_UHy9LVMlks5qWA4ygaJpZM4JNeA1

.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9878 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYiXF0sUR_3U4mHz19Huj-jCxbuesks5qXB4pgaJpZM4JNeA1
.

[ncdc]

@bparees @smarterclayton @csrwng updated to pull in my fixes from upstream for term.StdStreams. Please review the last commit.

[ncdc]

[test]

[openshift-bot]

Evaluated for origin test up to 49a14d5

[csrwng]

Last commit LGTM

[smarterclayton]

Looks ok to me to. Anything else anyone wants to say?

[openshift-bot]

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6721/)

[jwforres]

Just that I am anxiously waiting to hit the green button on this
kubernetes-ui/container-terminal#7

On Fri, Jul 22, 2016 at 11:58 AM, Clayton Coleman [email protected]
wrote:

Looks ok to me to. Anything else anyone wants to say?

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#9878 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7Yj6tIf8RVDzBgbWYCfv463TuIaTks5qYOiJgaJpZM4JNeA1
.

[smarterclayton]

LGTM [merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6789/) (Image: devenv-rhel7_4654)

[openshift-bot]

Evaluated for origin merge up to 49a14d5