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.

Sign up for GitHub

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

Jump to bottom

client: fix hijackedconn reading from buffer #36663

Merged
merged 1 commit into from
Mar 22, 2018
Merged

client: fix hijackedconn reading from buffer #36663

merged 1 commit into from
Mar 22, 2018

Conversation

tonistiigi
Copy link
Member

@tonistiigi tonistiigi commented Mar 21, 2018 •
edited
Loading

Fixes #36656
Regression from #36517

With the direct cast, the Read method is not inherited and the read doesn't go to the buffer but directly to the raw connection.

@jim-minter

Signed-off-by: Tonis Tiigi [email protected]

@tonistiigi tonistiigi requested a review from dnephin as a code owner March 21, 2018 22:32
@tonistiigi tonistiigi mentioned this pull request Mar 21, 2018
Merged
vdemeester
vdemeester approved these changes Mar 22, 2018
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@vdemeester
Copy link
Member

janky is actually green, it failed to build the e2e image because of a temporary failure on alpine packages 😓

thaJeztah
thaJeztah approved these changes Mar 22, 2018
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@jim-minter
Copy link
Contributor

thanks @tonistiigi

@codecov
Copy link

codecov bot commented Mar 22, 2018 •
edited
Loading

Codecov Report

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

@@            Coverage Diff            @@
##             master   #36663   +/-   ##
=========================================
  Coverage          ?   34.71%           
=========================================
  Files             ?      613           
  Lines             ?    45767           
  Branches          ?        0           
=========================================
  Hits              ?    15888           
  Misses            ?    27807           
  Partials          ?     2072

@vdemeester vdemeester merged commit 6909199 into moby:master Mar 22, 2018
@thaJeztah thaJeztah mentioned this pull request Mar 22, 2018
Closed
@kolyshkin kolyshkin mentioned this pull request Mar 26, 2018
Closed
@tonistiigi tonistiigi mentioned this pull request Mar 27, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdemeester vdemeester vdemeester approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@dnephin dnephin Awaiting requested review from dnephin

Assignees
No one assigned
Labels
status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tonistiigi @vdemeester @jim-minter @thaJeztah @cpuguy83 @GordonTheTurtle