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: can't allow attach in non-tty client when container in ttyMode #2338

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

ZYecho
Copy link
Contributor

@ZYecho ZYecho commented Oct 21, 2018

Signed-off-by: zhangyue [email protected]

Ⅰ. Describe what this PR did

As the title told. The current implementation have two problems:

  1. setRawMode func didn't check container if in ttyMode or not.
  2. if you start(with open stdin) a container from a non-tty client with container in tty mode, pouch didn't work and not return any error. this pr fix it by add a checktty func to throw error.

Ⅱ. Does this pull request fix one issue?

None.

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

add an integration test TestStartInTTY.

Ⅳ. Describe how to verify it

None.

Ⅴ. Special notes for reviews

run cli also have same problems, will fix in another pr.

@codecov
Copy link

codecov bot commented Oct 21, 2018

Codecov Report

Merging #2338 into master will increase coverage by 12.37%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #2338       +/-   ##
===========================================
+ Coverage    55.6%   67.97%   +12.37%     
===========================================
  Files         265      265               
  Lines       18208    18208               
===========================================
+ Hits        10124    12377     +2253     
+ Misses       6889     4401     -2488     
- Partials     1195     1430      +235
Flag Coverage Δ
#criv1alpha1test 31.38% <ø> (-0.21%) ⬇️
#criv1alpha2test 35.59% <ø> (?)
#integrationtest 39.51% <ø> (+0.71%) ⬆️
#nodee2etest 33.02% <ø> (-0.02%) ⬇️
#unittest 25.31% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pkg/meta/store.go 62.5% <0%> (-1.57%) ⬇️
cri/v1alpha1/cri.go 60.38% <0%> (-0.65%) ⬇️
daemon/mgr/container_utils.go 82.82% <0%> (ø) ⬆️
daemon/logger/jsonfile/utils.go 71.54% <0%> (+0.81%) ⬆️
storage/volume/driver/driver.go 68.03% <0%> (+1.63%) ⬆️
daemon/mgr/spec_mount.go 84.11% <0%> (+1.86%) ⬆️
pkg/meta/boltdb.go 68.67% <0%> (+2.4%) ⬆️
cri/v1alpha2/cri.go 68.03% <0%> (+2.42%) ⬆️
daemon/daemon.go 56.15% <0%> (+2.46%) ⬆️
pkg/utils/timeutils.go 100% <0%> (+2.63%) ⬆️
... and 79 more

@pouchrobot pouchrobot added kind/bug This is bug report for project size/M labels Oct 21, 2018
cli/start.go Show resolved Hide resolved
@Ace-Tang
Copy link
Contributor

@ZYecho , the title not tell where is wrong, wdyt?

@ZYecho ZYecho changed the title bugfix: start cli setRawMode when in tty mode and Terminal bugfix: can't allow attach in non-tty client when attach ttyMode Container Oct 23, 2018
@ZYecho
Copy link
Contributor Author

ZYecho commented Oct 23, 2018

@ZYecho , the title not tell where is wrong, wdyt?

Yeah, I update the description of the pr, does fix it?

@ZYecho ZYecho changed the title bugfix: can't allow attach in non-tty client when attach ttyMode Container bugfix: can't allow attach in non-tty client when container in ttyMode Oct 23, 2018
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuweid fuweid merged commit 53bd896 into AliyunContainerService:master Oct 23, 2018
@fuweid
Copy link
Contributor

fuweid commented Oct 23, 2018

@ZYecho could you mind to help us to fix the same issue in the exec command? 😄 thanks

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 size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants