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

Add WebSocket support and e2e test to verify. #3240

Merged
merged 2 commits into from
Feb 16, 2019
Merged

Add WebSocket support and e2e test to verify. #3240

merged 2 commits into from
Feb 16, 2019

Conversation

tcnghia
Copy link
Contributor

@tcnghia tcnghia commented Feb 15, 2019 •
edited

Proposed Changes

  • Add websocket support and e2e test to verify.

Release Note

Add WebSocket support.

@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2019
knative-prow-robot
knative-prow-robot reviewed Feb 15, 2019
View reviewed changes
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@tcnghia: 0 warnings.

In response to this:

This PR requires Go 1.12rc1 to get WebSocket support in ReverseProxy.
If we later decide that we want 1.12 for all of Knative we should pull
Go 1.12rc1 installation into base image or knative/test-infra.

Fixes #

Proposed Changes

  • Install Go 1.12rc1.
  • Add websocket support and e2e test to verify.

Release Note

Add WebSocket support.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mattmoor mattmoor added this to the Serving 0.4 milestone Feb 15, 2019
mattmoor
mattmoor reviewed Feb 15, 2019
View reviewed changes
pkg/websocket/hijack.go Outdated Show resolved Hide resolved
mattmoor
mattmoor reviewed Feb 15, 2019
View reviewed changes
test/presubmit-tests.sh Outdated
echo "> Installing Go 1.12"
go get golang.org/dl/go1.12rc1
go1.12rc1 download
cp $GOPATH/bin/go1.12rc1 $(which go)
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to know how we fix this. cc @chaodaiG

Copy link
Contributor

Choose a reason for hiding this comment

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

Should golang update be independent of the test CL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, @chaodaiG is helping

vagababov
vagababov reviewed Feb 15, 2019
View reviewed changes
pkg/network/websocket/hijack.go Outdated Show resolved Hide resolved
test/e2e/websocket_test.go Outdated Show resolved Hide resolved
test/e2e/websocket_test.go Outdated Show resolved Hide resolved
test/e2e/websocket_test.go Outdated Show resolved Hide resolved
test/e2e/websocket_test.go Show resolved Hide resolved
test/presubmit-tests.sh Outdated
echo "> Installing Go 1.12"
go get golang.org/dl/go1.12rc1
go1.12rc1 download
cp $GOPATH/bin/go1.12rc1 $(which go)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should golang update be independent of the test CL?

test/test_images/wsserver/echo.go Outdated Show resolved Hide resolved
pkg/network/websocket/hijack.go Outdated Show resolved Hide resolved
@tcnghia
Copy link
Contributor Author

tcnghia commented Feb 15, 2019

/assign @vagababov

I addressed all of your previous comments.

@mattmoor
Copy link
Member

Kicking the sockpuppet

@mattmoor mattmoor closed this Feb 15, 2019
@mattmoor mattmoor reopened this Feb 15, 2019
@mattmoor mattmoor closed this Feb 15, 2019
@mattmoor mattmoor reopened this Feb 15, 2019
mattmoor-sockpuppet
mattmoor-sockpuppet reviewed Feb 15, 2019
View reviewed changes
pkg/network/websocket/hijack.go Outdated Show resolved Hide resolved
pkg/network/websocket/hijack_test.go Outdated Show resolved Hide resolved
tcnghia
tcnghia commented Feb 15, 2019
View reviewed changes
pkg/network/websocket/hijack.go Outdated Show resolved Hide resolved
test/e2e/websocket_test.go Outdated Show resolved Hide resolved
test/e2e/websocket_test.go Outdated Show resolved Hide resolved
pkg/network/websocket/hijack.go Outdated Show resolved Hide resolved
pkg/network/websocket/hijack.go Outdated Show resolved Hide resolved
pkg/network/websocket/hijack_test.go Outdated Show resolved Hide resolved
test/e2e/websocket_test.go Outdated Show resolved Hide resolved
test/e2e/websocket_test.go Show resolved Hide resolved
test/presubmit-tests.sh Outdated
echo "> Installing Go 1.12"
go get golang.org/dl/go1.12rc1
go1.12rc1 download
cp $GOPATH/bin/go1.12rc1 $(which go)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, @chaodaiG is helping

test/test_images/wsserver/echo.go Outdated Show resolved Hide resolved
@tcnghia
Copy link
Contributor Author

tcnghia commented Feb 15, 2019

/hold

for golang update.

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2019
@chaodaiG chaodaiG mentioned this pull request Feb 15, 2019
Merged
@tcnghia tcnghia mentioned this pull request Feb 15, 2019
Merged
@chaodaiG chaodaiG mentioned this pull request Feb 15, 2019
Closed
vagababov
vagababov approved these changes Feb 15, 2019
View reviewed changes
Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

Looks fine to me.
/lgtm
/hold

pkg/network/websocket/hijack_test.go Outdated
// hijackable is a http.ResponseWriter that implements http.Hijacker
type hijackable struct {
http.ResponseWriter
w *bufio.ReadWriter
Copy link
Contributor

Choose a reason for hiding this comment

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

does this have to be *bufio.ReadWriter or just io.ReadWriter?

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 use that in the return value of Hijack() to test the plumbing, so have to be bufio.ReadWriter

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Feb 15, 2019
@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/activator/handler/handler.go 100.0% 97.4% -2.6
pkg/network/websocket/hijack.go Do not exist 100.0%
pkg/queue/timeout.go 97.8% 95.7% -2.1

Nghia Tran added 2 commits February 15, 2019 16:32
Add websocket support and e2e test to verify.
4eb1a73 
- Move pkg to address feedback.  Fix README.md file name.
- Address code style feedback.
- Fix markdown URL.
- Remove go 1.12 installation
Use knative/pkg/websocket. Delete moved code.
8e3536d
@mattmoor
Copy link
Member

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, tcnghia, vagababov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2019
@chaodaiG
Copy link
Contributor

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2019
@knative-prow-robot knative-prow-robot merged commit 1cb16e0 into knative:master Feb 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmoor mattmoor mattmoor left review comments

@mattmoor-sockpuppet mattmoor-sockpuppet mattmoor-sockpuppet left review comments

@knative-prow-robot knative-prow-robot knative-prow-robot left review comments

@vagababov vagababov vagababov approved these changes

@dushyanthsc dushyanthsc Awaiting requested review from dushyanthsc

@josephburnett josephburnett Awaiting requested review from josephburnett

@mdemirhan mdemirhan Awaiting requested review from mdemirhan

Assignees

@vagababov vagababov

@mattmoor mattmoor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/networking lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
0.4.x
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tcnghia @mattmoor @knative-metrics-robot @knative-prow-robot @chaodaiG @vagababov @mattmoor-sockpuppet