bugfix: align with docker v1.24 API #1226
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fuweid
commented
Apr 26, 2018
| defer func() { | ||
| if err := restoreMode(in, out); err != nil { | ||
| fmt.Fprintf(os.Stderr, "failed to restore term mode") | ||
| if e.Terminal { |
There was a problem hiding this comment.
If the command doesn't set tty, we don't need to set raw mode for terminal
HusterWan
reviewed
Apr 26, 2018
| @@ -493,6 +493,12 @@ | |||
| "revision": "53a58da551e961b3710bbbdfabbc162c3f5f30f6", | |||
| "revisionTime": "2018-01-31T23:13:00Z" | |||
| }, | |||
| { | |||
| "checksumSHA1": "H1rrbVmeE1z2TnkF7tSrfh+qUOY=", | |||
There was a problem hiding this comment.
Should we let docker vendor pkg version be same ? :)
There was a problem hiding this comment.
@HusterWan Yes. It's already "53a58da551e961b3710bbbdfabbc162c3f5f30f6"
HusterWan
reviewed
Apr 26, 2018
| @@ -95,19 +96,26 @@ func (e *ExecCommand) runExec(args []string) error { | |||
| wg.Add(1) | |||
| go func() { | |||
| defer wg.Done() | |||
| io.Copy(os.Stdout, reader) | |||
| if !e.Terminal { | |||
There was a problem hiding this comment.
I think we should add some test case for exec command -i and -it flags
we can add the test case later, because this feature is needed very hurry
fuweid
changed the title
The docker exec api uses custom stream bytes if the tty is false. In order to align with docker API, we need to use github.com/docker/docker/pkg/stdcopy package to modify stream bytes. In order to make the cri CI happy, we should skip the related `echo` test. Signed-off-by: Wei Fu <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1226 +/- ##
==========================================
- Coverage 15.37% 15.36% -0.02%
==========================================
Files 172 172
Lines 10619 10629 +10
==========================================
Hits 1633 1633
- Misses 8866 8876 +10
Partials 120 120
|
fuweid
changed the title
|
LGTM |
pouchrobot
added
the
LGTM
This was referenced May 3, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The docker exec api uses custom stream bytes if the tty is false.
In order to align with docker API, we need to use
github.com/docker/docker/pkg/stdcopy package to modify stream bytes.
Signed-off-by: Wei Fu [email protected]
Ⅰ. Describe what this PR did
Align with docker v1.24 API. As we known, the docker API has custom stream bytes when the client doesn't set tty. More information is here: https://github.com/moby/moby/blob/master/client/container_attach.go#L10-L33
Ⅱ. Does this pull request fix one issue?
Fix #1221
Ⅲ. Describe how you did it
use
github.com/docker/docker/pkg/stdcopyto wrap theio.Writer.Ⅳ. Describe how to verify it
after fix:
Ⅴ. Special notes for reviews
@allencloud @HusterWan @yyb196 This change is not ideal. We need to refactor the IO related things.