Skip to content

Conversation

@jbbarth
Copy link

@jbbarth jbbarth commented Sep 16, 2023

Issue #: #29

Description of changes: this change switches the terminal in "raw" mode so that it handles Ctrl+key sequences better.

  • IMPORTANT: I have very little experience with Go, and very little experience with low level terminal applications. I'm not sure at all this is the right fix.
  • I suspect some of the stty "manual" manipulations in the codebase could be replaced by what the "term" package does, but again, I know nothing ; I tried removing them and the behaviour doesn't change
  • ⚠️ the fix seems to add a new blank line after the end of the session, it's ugly but it doesn't seem to have a real effect

How I tested:

# build (I'm on a MacOS M2 so darwin-arm64)
rm -rf vendor/src/github.com/aws/session-manager-plugin/session-manager-plugin && gofmt -w src && make clean release && cp bin/darwin_arm64_plugin/session-manager-plugin ~/bin

# test before the modification ("mainline")
# => behaviour similar to homebrew installed version, Ctrl+K crashes, Ctrl+O doesn't work

# test after the modification (this PR)
aws ssm start-session --target <instance>
# basic commands
ipython
# => ctrl+O => works
# ctrl+K => doesn't kill the session
# ctrl+C, D => still working
# ctrl+J => works at first sight in the shell (new shell prompt)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

log.Errorf("Error switching terminal to raw mode: %s", err)
return
}
defer term.Restore(int(os.Stdin.Fd()), oldState)
Copy link
Author

@jbbarth jbbarth Sep 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-review: this could be moved to Stop() too, I don't have a strong preference. The behaviour doesn't change when I do it (we then need to add a new field originalTermState term.State on the ShellSession struct).

@@ -0,0 +1,26 @@
# Contributing to Go
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-review: the project doesn't seem to use "go.mod", so I blindly cloned the repository:

git clone https://github.com/golang/term.git vendor/src/golang.org/x/term

... and removed the .git directory there.

@AndrewFarley
Copy link

Note: This fix works great for me and makes the "JOE" CLI editor (yes, old I know) common escape sequences to work properly and not cause random issues or shell ejection. I can't speak for the code quality, but I thumbs-up this from a functionality point of view.

@jlebonzec
Copy link

Any chance to see this merged into the main SSM code?

@AndrewFarley
Copy link

The longer we wait, the more someone has to go fix conflicts. Please figure this out and merge this. I'm using this locally and it fixes all my issues and complaints with SSM.

@ghthor
Copy link

ghthor commented Apr 26, 2025

Gonna get to 2 years, classic AWS

@ghthor
Copy link

ghthor commented Apr 26, 2025

@Yangtao-Hua @ziwangj Yall need some help maintaining, reviewing and accepting patches from the community? Happy to help.

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