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: flaky test case in cli_start_test.go #2665

Merged
merged 1 commit into from
Jan 15, 2019
Merged

bugfix: flaky test case in cli_start_test.go #2665

merged 1 commit into from
Jan 15, 2019

Conversation

fuweid
Copy link
Contributor

@fuweid fuweid commented Jan 15, 2019

Ⅰ. Describe what this PR did

When we create container, we should have ability to open STDIN even
though we don't attach it yet. Otherwise, the pouch start command's
attach with stdin will not work well.

Therefore, pouch create need to support -i flag to keep STDIN open.
And in order to make the case easy to read, we can use StdinPipe
instead of github.com/kr/pty which we don't need any more.

Signed-off-by: Wei Fu fuweid89@gmail.com

Ⅱ. Does this pull request fix one issue?

NONE

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

no need.

Ⅳ. Describe how to verify it

check it manually and use test case to verify.

if we don't keep stdin open, the start will return immediately even though with -a -i.

# first, create container without openStdin
$ sudo pouch create --name t1 ubuntu cat

# start it
$ pouch start -a -i t1

if we keep stdin open, it will act like echo server.

➜  test git:(bugfix_container_io) sudo pouch create --name t2 -i ubuntu cat
106adbedb380377a76b814f221fa503bdb6211fbfcc55f68ca8ea2e9d467eeec
➜  test git:(bugfix_container_io) sudo pouch start -a -i t2
skdfj
skdfj
sfkj
sfkj

Ⅴ. Special notes for reviews

When we create container, we should have ability to open STDIN even
though we don't attach it yet. Otherwise, the `pouch start` command's
attach with stdin will not work well.

Therefore, `pouch create` need to support `-i` flag to keep STDIN open.
And in order to make the case easy to read, we can use `StdinPipe`
instead of `github.com/kr/pty` which we don't need any more.

Signed-off-by: Wei Fu <fuweid89@gmail.com>
@codecov
Copy link

codecov bot commented Jan 15, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2665      +/-   ##
==========================================
+ Coverage    68.6%   69.62%   +1.02%     
==========================================
  Files         281      281              
  Lines       18699    18699              
==========================================
+ Hits        12828    13020     +192     
+ Misses       4399     4230     -169     
+ Partials     1472     1449      -23
Flag Coverage Δ
#criv1alpha1test 31.69% <ø> (-0.19%) ⬇️
#criv1alpha2test 36.04% <ø> (-0.02%) ⬇️
#integrationtest 42.12% <ø> (-0.03%) ⬇️
#nodee2etest 32.53% <ø> (?)
#unittest 27.23% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pkg/streams/utils.go 82.14% <0%> (-9.53%) ⬇️
cri/v1alpha1/cri.go 59.43% <0%> (-0.67%) ⬇️
daemon/mgr/container.go 59.22% <0%> (+0.42%) ⬆️
ctrd/image.go 78.99% <0%> (+0.91%) ⬆️
cri/v1alpha2/cri_utils.go 88.44% <0%> (+1.78%) ⬆️
cri/stream/remotecommand/httpstream.go 46.63% <0%> (+2.59%) ⬆️
ctrd/container.go 58.49% <0%> (+2.76%) ⬆️
ctrd/watch.go 80.28% <0%> (+2.81%) ⬆️
daemon/mgr/spec_linux.go 79.73% <0%> (+2.99%) ⬆️
daemon/logger/jsonfile/encode.go 81.52% <0%> (+3.26%) ⬆️
... and 7 more

@pouchrobot pouchrobot added areas/test kind/bug This is bug report for project size/XXL labels Jan 15, 2019
@fuweid fuweid requested a review from rudyfly January 15, 2019 04:04
@rudyfly
Copy link
Collaborator

rudyfly commented Jan 15, 2019

LGTM, wait ci pass

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jan 15, 2019
@allencloud allencloud merged commit 182f7b8 into AliyunContainerService:master Jan 15, 2019
@fuweid fuweid deleted the bugfix_container_io branch January 15, 2019 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/test kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants