Skip to content

Conversation

bimmlerd
Copy link
Member

@bimmlerd bimmlerd commented Aug 5, 2025

Some people just want to exit their shell. The server has code for exit/stop/quit - but all it does is closing the connection. However, the client immediately reconnected on connection close, so that did nothing.

But reconnecting on connection close isn't really necessary: this is a local connection, so it seems unlikely to break. Hence, take the closing as a hint to stop.

Note to review: the diff looks horrible but all I did was drop a for loop 😅 You probably want to click that Hide whitespace button in the review.

Some people just want to exit their shell. The server has code for
exit/stop/quit - but all it does is closing the connection. However, the
client immediately reconnected on connection close, so that did nothing.

But reconnecting on connection close isn't really necessary: this is a
local connection, so it seems unlikely to break. Hence, take the closing
as a hint to stop.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd bimmlerd added the release-note/misc This PR makes changes that have no direct user impact. label Aug 5, 2025
@bimmlerd
Copy link
Member Author

bimmlerd commented Aug 5, 2025

/test

@bimmlerd bimmlerd marked this pull request as ready for review August 5, 2025 14:05
@bimmlerd bimmlerd requested a review from a team as a code owner August 5, 2025 14:05
@bimmlerd bimmlerd requested a review from joamaki August 5, 2025 14:05
@bimmlerd
Copy link
Member Author

bimmlerd commented Aug 5, 2025

The other option would be to make https://github.com/cilium/cilium/blob/main/pkg/shell/server/shell_server.go#L148 a bit smarter and send down some explicit quit signal

Other a third would be to handle exit/etc locally.

I don't really have a preference

@bimmlerd
Copy link
Member Author

bimmlerd commented Aug 6, 2025

CI: ci-clustermesh hit #39370 in https://github.com/cilium/cilium/actions/runs/16752394168/job/47425817270. Rerunning.

@joamaki joamaki removed the release-note/misc This PR makes changes that have no direct user impact. label Aug 6, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 6, 2025
@joamaki joamaki added release-note/misc This PR makes changes that have no direct user impact. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 6, 2025
@joamaki joamaki added this pull request to the merge queue Aug 6, 2025
Merged via the queue into cilium:main with commit 3cc4e95 Aug 6, 2025
74 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants