Fix: still warn on zero verbosity #68
Conversation
Previously, there was an issue where if there was no TTY, and verbosity was zero, the logger would be initialized with io.Discard instead of stderr. Log to stderr unless "--quiet" is passed. Signed-off-by: Will Murphy <[email protected]>
Signed-off-by: Will Murphy <[email protected]>
Signed-off-by: Will Murphy <[email protected]>
| @@ -41,7 +41,7 @@ func DefaultLogger(clioCfg Config, store redact.Store) (logger.Logger, error) { | |||
|
|
|||
| l, err := logrus.New( | |||
| logrus.Config{ | |||
| EnableConsole: cfg.Verbosity > 0 && !cfg.Quiet, | |||
There was a problem hiding this comment.
Is the problem that cfg.Verbosity is never set, so defaults to 0 and therefore EnableConsole == false? Is there a way that this configuration could be consolidated to just be based on a verbosity number instead of 3 distinct settings of verbosity, quiet, and level?
There was a problem hiding this comment.
Is the problem that cfg.Verbosity is never set, so defaults to 0 and therefore EnableConsole == false?
Yes, that's the problem. So if there's no TTY and there's no verbosity, there's nowhere for log.Warn outputs to go, because EnableConsole: false will cause the underlying writer to be io.Discard.
Is there a way that this configuration could be consolidated to just be based on a verbosity number instead of 3 distinct settings of verbosity, quiet, and level?
I don't think it can be simplified beyond this. We want -q but a log file and a verbosity set in the environment to still write to the environment I think? I'm not sure I understand your suggestion.
There was a problem hiding this comment.
I'm not suggesting to change any of the exposed options but rather make the options determine one specific verbosity level, and base all logging configuration on that. It looks like we have a function to do that: https://github.com/anchore/clio/blob/main/logging.go#L133 -- this takes all 3 options into consideration and returns a single logging level. We could modify this function to use the result of that function as the logging level, where anywhere we use quiet, instead we use levelFromSelectLevel != logger.Disabled or something like that, and we use that level to set Level: below, etc.
In fact, I'm not entirely sure why the selectLevel() function is modifying the configuration, it should probably not do that and instead just get called in PostLoad only to validate the options, then later in this function to get the effective logging level
There was a problem hiding this comment.
This has a giant TODO in it:
Lines 140 to 143 in ceccbdd
Is that something we should be addressing in this PR? Is there any issue for that?
There was a problem hiding this comment.
This TODO actually seems to be saying we don't want to collapse the amount of options here.
make the options determine one specific verbosity level, and base all logging configuration on that
I don't think we can sensibly do that, because we have 4 independent dimensions of input:
quiet- don't do fancy terminal UI or log to the terminal-v/-vvraise the level of the logger, regardless of where it prints- Whether stdout/stderr are a TTY - determines whether we can do fancy TUI stuff, but shouldn't determine anything else
logfile- change the logger, whatever its level is, to append to this path instead of stderr, regardless of whether--quietwas passed or a TTY is present.
In other words, something like SYFT_LOG_FILE=/tmp/log.txt syft --quiet -vvv ... should be a valid command, that should append trace logs of /tmp/log.txt and not do any fancy TUI stuff or log to stdout or stderr.
Does that make sense? Maybe we should have a discussion about how these options should interact, then revisit this PR?
There was a problem hiding this comment.
Here is the longer discussion about logging.
There was a problem hiding this comment.
Actually this is the topic I meant to link to.
|
I removed There's also a discourse thread for a discussion of configuring logging and TUI output. |
Relates to anchore/grype#2180 and anchore/syft#3081.
This PR makes 2 changes:
--verbosenor--quietis set,log.Warnshould be written to stderr.NO_TTYto be set to force disabling the fancy terminal UI even if a TTY is present.Number 1 will fix the linked bugs in Syft and Grype when this change is pulled into those repositories. Number 2 might be implemented differently or even removed - it really exists to enable testing bugs like number 1.