Add support for terminal size when executing command inside a container #983

armandpicard · 2022-08-12T09:40:00Z

Motivation

Adding support for terminal size when executing command in a container allow to reproduce "kubectl exec -it example -- sh" which can be needed when creating developers tools for example.

Solution

added the TerminalSize type and use the channel 4 to send terminal size.
changed the select in start_message_loop to select on more than 2 channels.
added the pod_exec_terminal_size example that reproduce "kubectl exec -it example -- sh", using complex terminal application like htop work fine in this example.

kazk

Please sign off your commits for DCO, and remove unused imports.

examples/pod_exec_terminal_size.rs

kube-client/src/api/remote_command.rs

Signed-off-by: armandpicard <armandpicard71@gmail.com>

examples/pod_exec_terminal_size.rs

pod_exec_terminal size example Signed-off-by: armandpicard <armandpicard71@gmail.com>

Signed-off-by: armandpicard <armandpicard71@gmail.com>

outdated

clux

I think this looks great. The example is very readable. The refactor is also nice despite the heavy change of the remote_command loop, and am slightly confused about some details there. It's the kind of change that makes me wish we have better tests for some of the nitty-gritty of the ws handling, but I am ultimately pro this change. The way it plugs into crossterm is really nice.

kube-client/src/api/remote_command.rs

examples/pod_exec_terminal_size.rs

clux · 2022-11-01T09:15:09Z

Also, sorry, I did not get to this sooner. Had missed it entirely :(

Signed-off-by: armandpicard <armandpicard71@gmail.com>

examples/pod_exec_terminal_size.rs

Co-authored-by: kazk <kazk.dev@gmail.com> Signed-off-by: armand picard <33733022+armandpicard@users.noreply.github.com>

Signed-off-by: armandpicard <armandpicard71@gmail.com>

clux · 2022-11-04T22:10:16Z

I played around a bit with this now, and it does seem to work, but it's an incredibly frustrating example atm because every 3rd keystroke is ignored if it happens at a certain time. not quite sure what is causing it, but something in the example is not plumbed up correctly.

but crucially it does a much better job than the pod_shell example at being user friendly (backspace, up arrow, etc all do the right thing now) 👍

clux · 2022-11-04T22:11:53Z

examples/pod_exec_terminal_size.rs

@@ -0,0 +1,134 @@
+use futures::{channel::mpsc::Sender, SinkExt, StreamExt};


we should rename this to pod_shell_crossterm to match the other example and highlight functionality

examples/pod_exec_terminal_size.rs

armandpicard · 2022-11-07T08:05:16Z

The keystroke issue is related to crossterm. It looks likes crossterm is also reading stdin and get the keystroke and send it on the EventStream, that why we don't get it in stdin. I'll try to dig more into it.

Signed-off-by: armandpicard <armandpicard71@gmail.com>

armandpicard · 2022-11-08T18:44:34Z

I can't find any way to avoid the EventStream from reading on stdin so I use just use signal stream on SIGWINCH. This work on unix I don't know if I should also make it work in Windows ?

clux · 2022-11-11T04:06:30Z

Thanks a lot, that feels a lot more usable now!

[..] SIGWINCH. This work on unix I don't know if I should also make it work in Windows ?

I think SIGWINCH is the right thing for us to do; minimal and keeps the code focused on what WE do, plus it serves the majority of users on mac/linux.

clux

Am happy to merge this as is, but will want to rename the example to pod_shell_crossterm unless you have objections to that.

Signed-off-by: armandpicard <armandpicard71@gmail.com>

clux · 2022-11-12T15:25:17Z

Thanks! I'm going to update the branch for CI and start the job so it's all good to go before merging.

codecov-commenter · 2022-11-12T15:31:30Z

Codecov Report

Merging #983 (ea0d2ec) into main (f6ca7df) will increase coverage by 0.13%.
The diff coverage is 44.89%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #983      +/-   ##
==========================================
+ Coverage   72.34%   72.48%   +0.13%     
==========================================
  Files          65       65              
  Lines        4715     4757      +42     
==========================================
+ Hits         3411     3448      +37     
- Misses       1304     1309       +5

Impacted Files	Coverage Δ
kube-client/src/api/mod.rs	`68.08% <ø> (ø)`
kube-client/src/api/remote_command.rs	`60.00% <44.89%> (-7.75%)`	⬇️
kube-runtime/src/reflector/mod.rs	`100.00% <0.00%> (ø)`
kube-runtime/src/utils/stream_backoff.rs	`87.05% <0.00%> (ø)`
kube-client/src/lib.rs	`93.79% <0.00%> (+0.04%)`	⬆️
kube-runtime/src/scheduler.rs	`96.81% <0.00%> (+0.08%)`	⬆️
kube-runtime/src/controller/runner.rs	`95.31% <0.00%> (+0.31%)`	⬆️
kube-client/src/client/middleware/mod.rs	`93.54% <0.00%> (+0.44%)`	⬆️
kube-runtime/src/controller/mod.rs	`37.83% <0.00%> (+0.56%)`	⬆️
kube/src/lib.rs	`88.57% <0.00%> (+0.71%)`	⬆️
... and 5 more

clux · 2022-11-12T15:44:36Z

Ah, ok so two issues:

rustfmt (needs the nightly rust and run just fmt)
the example fails to build on windows so just wrap most of the example in an if_cfg linux type thing (can also be done in the example's Cargo.toml to disallow the example on windows i think)
ignore the cargo deny failure (we will fix that separately, and it's not caused by this PR)

Just force push over my change if necessary.

…+ run fmt Signed-off-by: armandpicard <armandpicard71@gmail.com>

armandpicard · 2022-11-12T22:27:33Z

I've been kind with Windows user, I've made the example work on Windows but without handling the terminal size change, I just send the size when the program start.

clux

ah, yeah, that is nicer. partial windows support at least!

clux · 2022-11-13T08:54:15Z

Thank you so much!

…er (kube-rs#983) * Add support for terminal size when executing command inside a container Signed-off-by: armandpicard <armandpicard71@gmail.com> * remove unused import + fix example Signed-off-by: armandpicard <armandpicard71@gmail.com> * Use crossterm::event::EventStream to get terminal change in pod_exec_terminal size example Signed-off-by: armandpicard <armandpicard71@gmail.com> * Fix error when terminal_resize_tx is None Signed-off-by: armandpicard <armandpicard71@gmail.com> * Add some comments Signed-off-by: armandpicard <armandpicard71@gmail.com> * Fix typos Co-authored-by: kazk <kazk.dev@gmail.com> Signed-off-by: armand picard <33733022+armandpicard@users.noreply.github.com> * Fix: uniformize message send Signed-off-by: armandpicard <armandpicard71@gmail.com> * Remove crossterm event stream Signed-off-by: armandpicard <armandpicard71@gmail.com> * Rename example pod_shell_crossterm Signed-off-by: armandpicard <armandpicard71@gmail.com> * Make example run on window but without handling terminal size change + run fmt Signed-off-by: armandpicard <armandpicard71@gmail.com> Signed-off-by: armandpicard <armandpicard71@gmail.com> Signed-off-by: armand picard <33733022+armandpicard@users.noreply.github.com> Co-authored-by: kazk <kazk.dev@gmail.com> Co-authored-by: Eirik A <sszynrae@gmail.com>

kazk previously requested changes Aug 12, 2022

View reviewed changes

examples/pod_exec_terminal_size.rs Outdated Show resolved Hide resolved

examples/pod_exec_terminal_size.rs Outdated Show resolved Hide resolved

kube-client/src/api/remote_command.rs Show resolved Hide resolved

armandpicard added 2 commits August 12, 2022 22:02

Add support for terminal size when executing command inside a container

5852779

Signed-off-by: armandpicard <armandpicard71@gmail.com>

remove unused import + fix example

a0f42ab

Signed-off-by: armandpicard <armandpicard71@gmail.com>

armandpicard force-pushed the exec-terminal-size branch from c30fda0 to a0f42ab Compare August 12, 2022 20:03

kazk reviewed Aug 12, 2022

View reviewed changes

examples/pod_exec_terminal_size.rs Outdated Show resolved Hide resolved

armandpicard added 2 commits August 13, 2022 00:40

Use crossterm::event::EventStream to get terminal change in

28b61fe

pod_exec_terminal size example Signed-off-by: armandpicard <armandpicard71@gmail.com>

Fix error when terminal_resize_tx is None

e173782

Signed-off-by: armandpicard <armandpicard71@gmail.com>

kazk self-requested a review September 7, 2022 04:51

kazk mentioned this pull request Nov 1, 2022

exec sh (shell) with tty not processing escape chars #1063

Closed

kazk requested a review from a team November 1, 2022 02:53

clux added the changelog-add changelog added category for prs label Nov 1, 2022

clux modified the milestone: 0.77.0 Nov 1, 2022

clux reviewed Nov 1, 2022

View reviewed changes

clux linked an issue Nov 1, 2022 that may be closed by this pull request

exec sh (shell) with tty not processing escape chars #1063

Closed

Add some comments

1c5fdaf

Signed-off-by: armandpicard <armandpicard71@gmail.com>

kazk reviewed Nov 1, 2022

View reviewed changes

examples/pod_exec_terminal_size.rs Outdated Show resolved Hide resolved

armandpicard and others added 2 commits November 1, 2022 22:39

Fix typos

d0ce48a

Co-authored-by: kazk <kazk.dev@gmail.com> Signed-off-by: armand picard <33733022+armandpicard@users.noreply.github.com>

Fix: uniformize message send

8a98434

Signed-off-by: armandpicard <armandpicard71@gmail.com>

clux reviewed Nov 4, 2022

View reviewed changes

examples/pod_exec_terminal_size.rs Outdated Show resolved Hide resolved

clux reviewed Nov 4, 2022

View reviewed changes

examples/pod_exec_terminal_size.rs Outdated Show resolved Hide resolved

Remove crossterm event stream

32a6769

Signed-off-by: armandpicard <armandpicard71@gmail.com>

clux approved these changes Nov 11, 2022

View reviewed changes

Rename example pod_shell_crossterm

80651ac

Signed-off-by: armandpicard <armandpicard71@gmail.com>

Merge branch 'main' into exec-terminal-size

4aa5ea2

Make example run on window but without handling terminal size change …

ea0d2ec

…+ run fmt Signed-off-by: armandpicard <armandpicard71@gmail.com>

clux approved these changes Nov 13, 2022

View reviewed changes

clux merged commit d574fbc into kube-rs:main Nov 13, 2022

clux added this to the 0.77.0 milestone Nov 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for terminal size when executing command inside a container #983

Add support for terminal size when executing command inside a container #983

armandpicard commented Aug 12, 2022

kazk left a comment

clux left a comment •

edited

clux commented Nov 1, 2022

clux commented Nov 4, 2022

clux Nov 4, 2022

armandpicard commented Nov 7, 2022

armandpicard commented Nov 8, 2022

clux commented Nov 11, 2022

clux left a comment

clux commented Nov 12, 2022 •

edited

codecov-commenter commented Nov 12, 2022 •

edited

clux commented Nov 12, 2022 •

edited

armandpicard commented Nov 12, 2022

clux left a comment

clux commented Nov 13, 2022

		@@ -0,0 +1,134 @@
		use futures::{channel::mpsc::Sender, SinkExt, StreamExt};

Add support for terminal size when executing command inside a container #983

Add support for terminal size when executing command inside a container #983

Conversation

armandpicard commented Aug 12, 2022

Motivation

Solution

kazk left a comment

Choose a reason for hiding this comment

clux left a comment • edited

Choose a reason for hiding this comment

clux commented Nov 1, 2022

clux commented Nov 4, 2022

clux Nov 4, 2022

Choose a reason for hiding this comment

armandpicard commented Nov 7, 2022

armandpicard commented Nov 8, 2022

clux commented Nov 11, 2022

clux left a comment

Choose a reason for hiding this comment

clux commented Nov 12, 2022 • edited

codecov-commenter commented Nov 12, 2022 • edited

Codecov Report

clux commented Nov 12, 2022 • edited

armandpicard commented Nov 12, 2022

clux left a comment

Choose a reason for hiding this comment

clux commented Nov 13, 2022

clux left a comment •

edited

clux commented Nov 12, 2022 •

edited

codecov-commenter commented Nov 12, 2022 •

edited

clux commented Nov 12, 2022 •

edited