Skip to content

Fix Ctrl+C - #34

Merged
Tomasz Nguyen (swist) merged 1 commit into
masterfrom
fix-ctrlc
Jan 25, 2024
Merged

Tomasz Nguyen (swist) merged 1 commit into
masterfrom
fix-ctrlc

Conversation

@brianstrauch

Copy link
Copy Markdown

Started using this package for the new confluent ai command. Ctrl+C becomes unusable after exiting (not sure if you're seeing this in confluent flink shell or not...) but there's an unmerged fix for it here: c-bata#239. This PR copies this commit over and I have already tested that it works!

@brianstrauch
Brian Strauch (brianstrauch) requested a review from a team as a code owner January 24, 2024 23:28
@swist
Tomasz Nguyen (swist) merged commit 0140008 into master Jan 25, 2024
@swist
Tomasz Nguyen (swist) deleted the fix-ctrlc branch January 25, 2024 10:26
Yannick Pferr (YannickPferr) added a commit that referenced this pull request Feb 14, 2024
This reverts commit 0140008.
@YannickPferr

Copy link
Copy Markdown
Member

This breaks listening for CtrlC and other cancellation events in the flink shell, so I have to revert it.

To fix the issue you have with the ai command, you can instead save the terminal state before starting go-prompt, and then restore it to that state when exiting (that's what we did in flink shell). I can look into how to fix this inside of go-prompt directly in the future

Yannick Pferr (YannickPferr) added a commit that referenced this pull request Feb 14, 2024
@gustavodemorais

Copy link
Copy Markdown
Member

We actually had tried the same fix in the past and reverted it for the reason Yannick mentioned: #17

@gustavodemorais

Gustavo de Morais (gustavodemorais) commented Feb 14, 2024

Copy link
Copy Markdown
Member

Here's how we fixed it (what Yannick also mentioned we did for flink shell): https://github.com/confluentinc/cli/blob/main/pkg/flink/app/application.go#L68

We basically store stdinBefore before starting go-prompt and restore it as part of the cleanup when exiting go-prompt :) This way we had ctrl c not bugging and listening for keystrokes also works!

@YannickPferr

Copy link
Copy Markdown
Member

Here's how you could do it for the ai command:
https://github.com/confluentinc/cli-internal/pull/103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants