Use os.Stderr instead of os.Stdout with fmt.Fprintf() #20
Conversation
42milez
changed the title
| } else { | ||
| fmt.Fprintf(os.Stdout, "\n\nSessionId: %s : %s\n\n", sessionId, channelClosedMessage.Output) | ||
| fmt.Fprintf(os.Stderr, "\n\nSessionId: %s : %s\n\n", sessionId, channelClosedMessage.Output) |
There was a problem hiding this comment.
Why are there two \n characters here?
|
@Yangtao-Hua @yuting-fan can we merge this? |
|
To be able to use session manager for various tooling, it would be essential to merge this. |
| @@ -190,7 +190,7 @@ func ValidateInputAndStartSession(args []string, out io.Writer) { | |||
|
|
|||
| //Execute create data channel and start the session | |||
| func (s *Session) Execute(log log.T) (err error) { | |||
| fmt.Fprintf(os.Stdout, "\nStarting session with SessionId: %s\n", s.SessionId) | |||
| fmt.Fprintf(os.Stderr, "\nStarting session with SessionId: %s\n", s.SessionId) | |||
There was a problem hiding this comment.
Many just notification messages like these. e.g. Start/stop session, or call out KMS enabled. Not sure changing the behavior to write to Stderr is a better solution.
There was a problem hiding this comment.
Maybe a quiet mode to not print those messages
There was a problem hiding this comment.
It's better behaviour because it doesn't mix the output with normal output. Say, you call a remote script to generate CSV. Right now you end up with log messages mixed with CSV output. With this change, the log messages go to stderr and aren't mixed with stdout.
There was a problem hiding this comment.
Yep, it causes issues like this: https://github.com/zagaran/eb-ssm/pull/4/files#diff-e2c321f9e0a8996da965d12461b0e7d1216893f21adf5fde6cf7366fe288fe55R113
There was a problem hiding this comment.
Thanks all, will test it and support the change in next release.
There was a problem hiding this comment.
Hi, just a quick update, we cannot merge this change as plan, since many customer are trying to extract info from the Stdout. e.g. plugin version. This change will break their current user experience. As @tapajos mentioned, we may need a quiet mode, or reduce some logs, change part of them to Stderr. Team will work on resolve this issue in a graceful way later. Thanks for understanding.
There was a problem hiding this comment.
LOL, I am in awe of what some people do... yes, please introduce a quiet mode. What is the ETA?
There was a problem hiding this comment.
@Yangtao-Hua would #94 provide what you need to avoid breaking current user experience?
Issue #, if available:
N/A
Description of changes:
In the latest version (1.2.279.0), fmt.Fprintf() writes to
os.Stdoutin any context. In some cases, this can lead to unexpected behaviors of the application which works with Session Manager.In my case, paramiko, a SSH implementaiton for Python, reads the output (e.g. session.go:193) of session-manager-plugin from stdout during the wait for SSH banner.
As the result, paramiko breaks out the loop of wating for SSH banner, and the banner_timeout parameter is reset to paramiko's default (e.g. transport.py:2201). For this reason, I cannot connect to a server via SSH through the tunnel of Session Manager in the environment that takes a long time to establish the connection.
So, if possible, I would like to make a change so that fmt.Fprint() writes to stderr.
Additional Information:
A similar topic is being discussed in the repository of aws-ssm-agent.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.