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

bugfix: setRawMode can only be set when the user set tty #1233

Merged
merged 1 commit into from
Apr 27, 2018
Merged

bugfix: setRawMode can only be set when the user set tty #1233

merged 1 commit into from
Apr 27, 2018

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Apr 27, 2018

Signed-off-by: Wei Fu fhfuwei@163.com

Ⅰ. Describe what this PR did

setRawMode is used to terminal case. If the shell is non-interactive or non-terminal, we should not set the raw mode for the input.

Ⅱ. Does this pull request fix one issue?

None

Ⅲ. Describe how you did it

check tty before set the raw mode

Ⅳ. Describe how to verify it

we need to set the shell in no-interactive mode, please use the following the script:

#!/bin/bash

cat >test <<'EOF'
set +i # set non-interactive
pouch run -i busybox:latest echo test
EOF

echo 'running bash -i <test'
bash -i <test

before this change:

➜  /tmp sh ./test-interactive
running bash -i <test
root@ubuntu-xenial:/tmp# set +i # set non-interactive
root@ubuntu-xenial:/tmp# pouch run -i busybox:latest echo test
Error: failed to set raw mode
root@ubuntu-xenial:/tmp# exit

after this change:

➜  /tmp sh ./test-interactive
running bash -i <test
root@ubuntu-xenial:/tmp# set +i # set non-interactive
root@ubuntu-xenial:/tmp# pouch run -i busybox:latest echo test
test
root@ubuntu-xenial:/tmp# exit

Ⅴ. Special notes for reviews

Signed-off-by: Wei Fu <fhfuwei@163.com>
@pouchrobot pouchrobot added kind/bug This is bug report for project size/S labels Apr 27, 2018
@fuweid fuweid changed the title bugfix: setRawMode should be set when the user set tty bugfix: setRawMode can only be set when the user set tty Apr 27, 2018
@codecov-io
Copy link

Codecov Report

Merging #1233 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1233      +/-   ##
==========================================
- Coverage   15.36%   15.36%   -0.01%     
==========================================
  Files         172      172              
  Lines       10629    10630       +1     
==========================================
  Hits         1633     1633              
- Misses       8876     8877       +1     
  Partials      120      120
Impacted Files Coverage Δ
cli/run.go 0% <0%> (ø) ⬆️

@HusterWan
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 27, 2018
@HusterWan HusterWan merged commit a51e352 into AliyunContainerService:master Apr 27, 2018
@fuweid fuweid deleted the bugfix_set_raw_mode_should_use_in_tty branch April 27, 2018 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants