Fix: still warn on zero verbosity

[willmurphyscode]

Relates to anchore/grype#2180 and anchore/syft#3081.

This PR makes 2 changes:

1. If no TTY is present, and neither --verbose nor --quiet is set, log.Warn should be written to stderr.
2. Allow the environment variables NO_TTY to 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.

[kzantow]

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?

[willmurphyscode]

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.

[kzantow]

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

[willmurphyscode]

This has a giant TODO in it:

clio/logging.go

Lines 140 to 143 in ceccbdd

	// TODO: this is bad: quiet option trumps all other logging options (such as to a file on disk)
	// we should be able to quiet the console logging and leave file logging alone...
	// ... this will be an enhancement for later
	return logger.DisabledLevel, nil

Is that something we should be addressing in this PR? Is there any issue for that?

[willmurphyscode]

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:

1. quiet - don't do fancy terminal UI or log to the terminal
2. -v / -vv raise the level of the logger, regardless of where it prints
3. Whether stdout/stderr are a TTY - determines whether we can do fancy TUI stuff, but shouldn't determine anything else
4. logfile - change the logger, whatever its level is, to append to this path instead of stderr, regardless of whether --quiet was 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?

[willmurphyscode]

Here is the longer discussion about logging.

[willmurphyscode]

Actually this is the topic I meant to link to.

[willmurphyscode]

I removed needs-discussion because we discussed it on yesterday's live stream

There's also a discourse thread for a discussion of configuring logging and TUI output.