Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use os.Stderr instead of os.Stdout with fmt.Fprintf() #20

Open
wants to merge 1 commit into
base: mainline
Choose a base branch
from
Open

Use os.Stderr instead of os.Stdout with fmt.Fprintf() #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/datachannel/streaming.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ func (dataChannel *DataChannel) handleHandshakeComplete(log log.T, clientMessage
handshakeComplete.HandshakeTimeToComplete.Seconds())

if handshakeComplete.CustomerMessage != "" {
fmt.Fprintln(os.Stdout, handshakeComplete.CustomerMessage)
fmt.Fprintln(os.Stderr, handshakeComplete.CustomerMessage)
}

return err
Expand Down Expand Up @@ -763,9 +763,9 @@ func (dataChannel DataChannel) HandleChannelClosedMessage(log log.T, stopHandler

log.Infof("Exiting session with sessionId: %s with output: %s", sessionId, channelClosedMessage.Output)
if channelClosedMessage.Output == "" {
fmt.Fprintf(os.Stdout, "\n\nExiting session with sessionId: %s.\n\n", sessionId)
fmt.Fprintf(os.Stderr, "\n\nExiting session with sessionId: %s.\n\n", sessionId)
} 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)
Copy link

Choose a reason for hiding this comment

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

Why are there two \n characters here?

}

stopHandler()
Expand Down
2 changes: 1 addition & 1 deletion src/sessionmanagerplugin-main/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ import (
)

func main() {
session.ValidateInputAndStartSession(os.Args, os.Stdout)
session.ValidateInputAndStartSession(os.Args, os.Stderr)
}
2 changes: 1 addition & 1 deletion src/sessionmanagerplugin/session/portsession/basicportforwarding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func (p *BasicPortForwarding) handleControlSignals(log log.T) {
if err := p.session.DataChannel.SendFlag(log, message.TerminateSession); err != nil {
log.Errorf("Failed to send TerminateSession flag: %v", err)
}
fmt.Fprintf(os.Stdout, "\n\nExiting session with sessionId: %s.\n\n", p.sessionId)
fmt.Fprintf(os.Stderr, "\n\nExiting session with sessionId: %s.\n\n", p.sessionId)
p.Stop()
} else {
p.session.TerminateSession(log)
Expand Down
2 changes: 1 addition & 1 deletion src/sessionmanagerplugin/session/portsession/muxportforwarding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ func (p *MuxPortForwarding) handleControlSignals(log log.T) {
if err := p.session.DataChannel.SendFlag(log, message.TerminateSession); err != nil {
log.Errorf("Failed to send TerminateSession flag: %v", err)
}
fmt.Fprintf(os.Stdout, "\n\nExiting session with sessionId: %s.\n\n", p.sessionId)
fmt.Fprintf(os.Stderr, "\n\nExiting session with sessionId: %s.\n\n", p.sessionId)
p.Stop()
}()
}
Expand Down
2 changes: 1 addition & 1 deletion src/sessionmanagerplugin/session/session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Maybe a quiet mode to not print those messages

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Yep, it causes issues like this: https://github.com/zagaran/eb-ssm/pull/4/files#diff-e2c321f9e0a8996da965d12461b0e7d1216893f21adf5fde6cf7366fe288fe55R113

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks all, will test it and support the change in next release.

Copy link
Contributor

@Yangtao-Hua Yangtao-Hua Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

LOL, I am in awe of what some people do... yes, please introduce a quiet mode. What is the ETA?

Copy link

Choose a reason for hiding this comment

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

@Yangtao-Hua would #94 provide what you need to avoid breaking current user experience?


// sets the display mode
s.DisplayMode = sessionutil.NewDisplayMode(log)
Expand Down
2 changes: 1 addition & 1 deletion src/sessionmanagerplugin/session/sessionhandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func (s *Session) ResumeSessionHandler(log log.T) (err error) {
return
} else if s.TokenValue == "" {
log.Debugf("Session: %s timed out", s.SessionId)
fmt.Fprintf(os.Stdout, "Session: %s timed out.\n", s.SessionId)
fmt.Fprintf(os.Stderr, "Session: %s timed out.\n", s.SessionId)
os.Exit(0)
}
s.DataChannel.GetWsChannel().SetChannelToken(s.TokenValue)
Expand Down
2 changes: 1 addition & 1 deletion src/sessionmanagerplugin/session/sessionutil/sessionutil_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func (d *DisplayMode) DisplayMessage(log log.T, message message.ClientMessage) {
// refer - https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-writefile
if err = windows.WriteFile(d.handle, message.Payload, done, nil); err != nil {
log.Errorf("error occurred while writing to file: %v", err)
fmt.Fprintf(os.Stdout, "\nError getting the output. %s\n", err.Error())
fmt.Fprintf(os.Stderr, "\nError getting the output. %s\n", err.Error())
os.Exit(0)
}
}
Expand Down