-
Notifications
You must be signed in to change notification settings - Fork 949
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
feature: add cli attach command #2248
Conversation
0e98bef
to
d7f6690
Compare
Do we need to add some integration test to cover this command line's functionality? @ZYecho |
4218820
to
7cc87d3
Compare
I did't find the pattern about how do the cli command integration test, is there is a doc or code sample about that? could you help me with this? thank you |
Yes, we have. Please check the documents here https://github.com/alibaba/pouch/blob/master/docs/test/test.md to get noticed of how to run local integration test. And an example is in https://github.com/alibaba/pouch/blob/master/test/cli_restart_test.go, you can see the restart command integration test there. @ZYecho Please feel free to tell us if you have any questions. Thanks. |
7cc87d3
to
fcc5ca8
Compare
Codecov Report
@@ Coverage Diff @@
## master #2248 +/- ##
=========================================
Coverage ? 69.04%
=========================================
Files ? 279
Lines ? 18491
Branches ? 0
=========================================
Hits ? 12768
Misses ? 4272
Partials ? 1451
|
fcc5ca8
to
0f7af8d
Compare
I add my integration test in test/cli_attach_test.go, but it seems not work. But when I test the attach command manually in the same way as code does, it works correctly. Could you help to review the test code? Thank you. |
9979e94
to
71ed412
Compare
71ed412
to
85da35e
Compare
140440e
to
41aa003
Compare
I’ve updated this pr, make integration test right , but CI blocked at criv1alpha2test, could you help to trigger the CI again? @allencloud , ping @fuweid for review to move on this pr either. |
41aa003
to
c4e56bc
Compare
zhangyue seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
c4e56bc
to
9c4e468
Compare
|
2d3e957
to
298b14e
Compare
0348f11
to
a7950fe
Compare
CI have passed, and fix some issue. @fuweid @allencloud PTAL, thanks. |
"checksumSHA1": "GFsDxJkQz407/2nUBmWuafG+uF8=", | ||
"path": "github.com/docker/docker/pkg/term", | ||
"revision": "8e908cab46ee73104bedc4cf5fc4a15b7647b500", | ||
"revisionTime": "2018-09-25T23:55:46Z" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep the same revision with existing docker package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how to keep it. could you give me some suggestions?
} | ||
}() | ||
|
||
escapeKeys := defaultEscapeKeys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the current design, we have not supported the EscapeKeys
yet in backend. Please check this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just want to know, why the backend need work to support EscapeKeys
?
Signed-off-by: zhangyue <zy675793960@yeah.net>
a7950fe
to
e82e57e
Compare
close it temporarily |
Signed-off-by: zhangyue zy675793960@yeah.net
Ⅰ. Describe what this PR did
add attach cli command.
Ⅱ. Does this pull request fix one issue?
None
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)
Add proxy and term unit test.
Ⅳ. Describe how to verify it
$ pouch run -it --detach-keys="ctrl-d" busybox
$ pouch ps
$ pouch attach --detach-keys="ctrl-x" busybox
Ⅴ. Special notes for reviews
inspired by docker attach impl and pr#2240