-
Notifications
You must be signed in to change notification settings - Fork 98
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.
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
Fix logging on the simulator #873
Conversation
fileEnc := config.LogFormat.FileEncoder() | ||
|
||
var consoleWriter io.WriteCloser | ||
if config.DisableWriterDisplaying { | ||
consoleWriter = newDiscardWriteCloser() | ||
} else { | ||
consoleWriter = os.Stdout | ||
consoleWriter = os.Stderr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use Stderr in order not to interfere with the plans being piped from Stdin. This is hopefully a temporary solution.
x/programs/cmd/simulator/cmd/root.go
Outdated
// pre-execute the command to pre-parse flags | ||
err := cmd.Execute() | ||
if err != nil { | ||
fmt.Fprintln(os.Stderr, err) | ||
os.Exit(1) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this too hacky ? I haven't found a clean solution to pass a global flag to subcommands before the call to Execute
(parsing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to make a cmd.ParseFlags(os.Args[1:])
instead. This is cleaner in my opinion. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@najeal the issue is that this won't let us place the cli flag in another position than just after the call to the binary if I understand correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have read too fast, that's a good suggestion and I will add it thanks !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just proposing another solution for accessing the value of the log-level
flag
x/programs/cmd/simulator/cmd/root.go
Outdated
// pre-execute the command to pre-parse flags | ||
err := cmd.Execute() | ||
if err != nil { | ||
fmt.Fprintln(os.Stderr, err) | ||
os.Exit(1) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to make a cmd.ParseFlags(os.Args[1:])
instead. This is cleaner in my opinion. What do you think?
24217d8
to
94b4b94
Compare
# This is the 1st commit message: fix logging # The commit message #2 will be skipped: # fix nits # The commit message #3 will be skipped: # set log level to error in the simulator client & write logs to Stderr # The commit message #4 will be skipped: # preparse flags with `ParseFlags` # The commit message #5 will be skipped: # fix logging # The commit message #6 will be skipped: # fix nits # The commit message #7 will be skipped: # set log level to error in the simulator client & write logs to Stderr # The commit message #8 will be skipped: # fix logging # The commit message #9 will be skipped: # fix nits # The commit message #10 will be skipped: # set log level to error in the simulator client & write logs to Stderr # The commit message #11 will be skipped: # preparse flags with `ParseFlags`
94b4b94
to
02c63bf
Compare
Closing because I'm currently rewriting that part anyway |
Closes #864, closes #850, closes #787
The logging on the simulator was broken. The issue is that the logging level which is passed to subcommands was done before the call to
ExecuteContext
. We callExecute
and it's a bit hacky but there doesn't seem to be many very clean solutions. The logging can now be disabled using the new--log-disable-display-plugin-logs
flag.