Enable logging by default #65
Conversation
This patch should fix issue sourcegraph#64 Disabling logging by default makes 8000 it very unfriendly to diagnose configuration issues. Also, discarding logs with log.SetOutput at the package level is bad because it affects all packages that have imported the checkup package.
| @@ -98,5 +98,5 @@ func Execute() { | |||
| func init() { | |||
| RootCmd.PersistentFlags().StringVarP(&configFile, "config", "c", "checkup.json", "JSON config file") | |||
| RootCmd.Flags().BoolVar(&storeResults, "store", false, "Store results") | |||
| RootCmd.Flags().BoolVar(&printLogs, "v", false, "Enable logging to standard output") | |||
| RootCmd.Flags().BoolVarP(&quiet, "quiet", "q", false, "Disable logging to standard output") | |||
There was a problem hiding this comment.
This flag is documented as:
Disable logging to standard output
I don't think that accurately describes what it does. I think it should be:
Disable logging to standard error
Because the default output of the standard logger in log package is os.Stderr, not os.Stdout:
Package log implements a simple logging package. It defines a type, Logger, with methods for formatting output. It also has a predefined 'standard' Logger accessible through helper functions Print[f|ln], Fatal[f|ln], and Panic[f|ln], which are easier to use than creating a Logger manually. That logger writes to standard error and prints the date and time of each logged message.
There was a problem hiding this comment.
Good point.
Maybe we shouldn't confuse the user with stdout or stderr anyway, maybe it should just say "Do not display log messages"?
There was a problem hiding this comment.
The problem is we need to distinguish between logging and "standard output" -- the CLI is interactive (well, sorta -- you give the input via env vars and/or flags) and so the fmt.Print lines are supposed to show those things so the user knows what's going on. The logging output is something else, so maybe "quiet" is too ambiguous. What about instead, the main package disables logging by default unless logging is turned on. The standard output from the fmt.Prints shouldn't be hidden. The user will need to see them.
There was a problem hiding this comment.
I still think that the user doesn't need to see "One sec" but, OTOH, the user would need to see a fatal configuration parse error. So IMHO something's wrong with the current logging system.
Anyways, so you suggest we keep current behavior?
There was a problem hiding this comment.
Good point... there are also occasions where the output is credentials that are generated by a provisioning command and the user only has 1 shot at getting those, so it's critical that it can't be turned off.
Maybe the configuration errors need to be fmt.Print'ed instead of logged.
There was a problem hiding this comment.
Errors could also be sent to stderr with fmt.Fprint, so that they would not be in the data stream when checkup is called from a script.
However, the checkup "library" should not send anything directly to stdout/stderr by itself.
|
PR #130 also disables any kind of log redirection (currently we don't remap logs to stdout or stderr, but only change the default to not swallow the logs as they are output). Since parsing checkup exec data from the output doesn't seem to be a concern, I feel like a "print everything" default should be the way to do, and then we can correct behaviour where this isn't wanted. Closing this issue as stale/outdated (last comment in 2017). |
This patch should fix the 2 issues discussed in #64.
Disabling logging by default makes it very unfriendly to diagnose configuration issues.
Also, discarding logs with log.SetOutput at the package level is bad because it affects all packages that import the checkup package.
An example of side effect has been added by @shurcooL to the PR #55 (comment).