Add support for the v5 protocol #2037
Conversation
k8s-ci-robot
added
approved
src/web-socket-handler.ts
Outdated
| } | ||
|
|
||
| public static closeStream(streamNum: number, streams: StreamInterface): void { | ||
| console.log('Closing stream: ' + streamNum); |
There was a problem hiding this comment.
There are three console.log()s left over.
There was a problem hiding this comment.
ugh, so embarrassing to leave test printfs in the PR :)
fixed.
src/web-socket-handler_test.ts
Outdated
| @@ -2,6 +2,7 @@ import { promisify } from 'util'; | |||
| import { expect } from 'chai'; | |||
| import WebSocket = require('isomorphic-ws'); | |||
| import { ReadableStreamBuffer, WritableStreamBuffer } from 'stream-buffers'; | |||
| import stream = require('stream'); | |||
There was a problem hiding this comment.
| import stream = require('stream'); | |
| import stream from 'node:stream'; |
src/web-socket-handler_test.ts
Outdated
| expect(sent!.readUint8(0)).to.equal(255); // CLOSE signal | ||
| expect(sent!.readUInt8(1)).to.equal(0); // Stdin stream is #0 |
There was a problem hiding this comment.
Is there a reason one of these uses readUint8() and the other uses readUInt8() (note difference in casing of the i).
There was a problem hiding this comment.
Converted all to readUint8 The Internet seems to be ambiguous on which is the right casing. Both appear to work...
|
/lgtm Learned a bit about streaming while reviewing this PR 🎉 |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, mstruebing 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 |
|
Does this make sense for the v1.x branch as well? |
Support for the v5 streaming protocol, described here:
https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/4006-transition-spdy-to-websockets#proposal-new-remotecommand-sub-protocol-version---v5channelk8sio
May help fix this: #1407