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

feature: add cli attach command #2248

Closed
wants to merge 1 commit into from

Conversation

ZYecho
Copy link
Contributor

@ZYecho ZYecho commented Sep 17, 2018

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

  1. run a container and detach from terminal
    $ pouch run -it --detach-keys="ctrl-d" busybox
  2. get id or name
    $ pouch ps
  3. attach it
    $ pouch attach --detach-keys="ctrl-x" busybox

Ⅴ. Special notes for reviews

inspired by docker attach impl and pr#2240

cli/attach.go Outdated Show resolved Hide resolved
cli/attach.go Outdated Show resolved Hide resolved
cli/attach.go Outdated Show resolved Hide resolved
@allencloud
Copy link
Collaborator

Do we need to add some integration test to cover this command line's functionality? @ZYecho

@ZYecho ZYecho force-pushed the add-attach-cli branch 4 times, most recently from 4218820 to 7cc87d3 Compare September 18, 2018 03:15
@ZYecho
Copy link
Contributor Author

ZYecho commented Sep 18, 2018

Do we need to add some integration test to cover this command line's functionality? @ZYecho

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

@allencloud
Copy link
Collaborator

the pattern about how do the cli command integration test, is there is a doc or code sample about that?

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.

@codecov
Copy link

codecov bot commented Sep 19, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@401167e). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2248   +/-   ##
=========================================
  Coverage          ?   69.04%           
=========================================
  Files             ?      279           
  Lines             ?    18491           
  Branches          ?        0           
=========================================
  Hits              ?    12768           
  Misses            ?     4272           
  Partials          ?     1451
Flag Coverage Δ
#criv1alpha1test 31.26% <0%> (?)
#criv1alpha2test 35.28% <0%> (?)
#integrationtest 40.48% <0%> (?)
#nodee2etest 32.7% <0%> (?)
#unittest 26.76% <ø> (?)
Impacted Files Coverage Δ
pkg/ioutils/readers.go 0% <0%> (ø)

@ZYecho
Copy link
Contributor Author

ZYecho commented Sep 19, 2018

the pattern about how do the cli command integration test, is there is a doc or code sample about that?

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.

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.

cli/attach.go Outdated Show resolved Hide resolved
cli/attach.go Outdated Show resolved Hide resolved
pkg/term/proxy.go Outdated Show resolved Hide resolved
@ZYecho ZYecho force-pushed the add-attach-cli branch 2 times, most recently from 9979e94 to 71ed412 Compare September 25, 2018 12:16
cli/attach.go Outdated Show resolved Hide resolved
@ZYecho ZYecho force-pushed the add-attach-cli branch 4 times, most recently from 140440e to 41aa003 Compare October 20, 2018 09:13
@ZYecho
Copy link
Contributor Author

ZYecho commented Oct 20, 2018

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.

@CLAassistant
Copy link

CLAassistant commented Oct 22, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


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.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ZYecho ZYecho force-pushed the add-attach-cli branch 4 times, most recently from 2d3e957 to 298b14e Compare October 26, 2018 01:19
@ZYecho ZYecho force-pushed the add-attach-cli branch 2 times, most recently from 0348f11 to a7950fe Compare November 9, 2018 02:15
@ZYecho
Copy link
Contributor Author

ZYecho commented Nov 9, 2018

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"
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

@fuweid fuweid Dec 4, 2018

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.

Copy link
Contributor Author

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?

cli/attach.go Outdated Show resolved Hide resolved
Signed-off-by: zhangyue <zy675793960@yeah.net>
@ZYecho
Copy link
Contributor Author

ZYecho commented Mar 20, 2019

close it temporarily

@ZYecho ZYecho closed this Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants