Support terminal resizes #240
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/assign @brendandburns |
| const kc = new k8s.KubeConfig(); | ||
| kc.loadFromDefault(); | ||
|
|
||
| const terminalSizeQueue = new k8s.TerminalSizeQueue(); |
There was a problem hiding this comment.
@lizardruss Thanks for the contribution! I haven't had the chance to test it out yet.
Wondering if it is better to move this logic private to the client? So the library binds the events and adds to the internal queues.
It feels like its a standard event on NodeJS.WriteStream so maybe we can hide this implementation logic from the users of the client?
It not only would solve a bunch of copy paste between attach and exec but also means that it becomes improvement clients get without any code changes.
Unless this is how it is with most of the other clients @brendanburns your thoughts?
There was a problem hiding this comment.
Thank you for the suggestion! I like the idea of removing the TerminalSizeQueue class. I mostly just ported it from the kubectl codebase, and it didn't feel quite right.
If I understand you correctly, you're suggesting to use the stdout argument to detect resizes. Currently its typed as Writable | null for exec and Writable | any for attach. Changing to NodeJS.WriteStream might be a breaking change.
Another potential issue is that NodeJS.WriteStream's rows and columns properties are optional. I think tty.WriteStream might be the more accurate type, since NodeJS.WriteStream doesn't have a documented resize event. I'll make that change in the examples.
For more context, I won't be using process.stdout for my use case. Instead I'll be using this from a set of web based services that support xterm.js. In this case a WebSocket will be connected to Readable and Writeable streams, and resizing will be done through a separate service call.
I'll experiment with creating a tty.WriteStream instance and connecting it to a WebSocket. That could be a more straightforward implementation, but I've never tried instantiating that class.
There was a problem hiding this comment.
Creating a new tty.WriteStream instance has an odd side effect of also writing to stdout. 👎
It's probably better to stick with the NodeJS.WriteStream interface. I'll try making an implementation that can be used with a web socket.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, lizardruss 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 |
k8s-ci-robot
added
the
approved
This adds resize support for
execandattach#239There were also some false positives with the associated tests. I created a solution, but open to alternative approaches.
The name
TerminalSizeQueuewas taken from the kubernetes code base, and in this context it's aReadablestream that abstracts the sending of aTerminalSizeinterface over the resize stream.I also added examples that resize the pod's terminal with
process.stdout