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

Fix: still warn on zero verbosity #68

Merged
merged 3 commits into from
Nov 15, 2024
Merged

Fix: still warn on zero verbosity #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee7b891
fix: allocate stderr logger if log.Warn needed
willmurphyscode Oct 16, 2024
adf59de
feat: respect NO_TTY env var when setting up UI
willmurphyscode Oct 16, 2024
b98ebbb
test: unit test for warn logging
willmurphyscode Oct 16, 2024
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
16 changes: 15 additions & 1 deletion logging.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@kzantow kzantow Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the longer discussion about logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is the topic I meant to link to.

EnableConsole: !cfg.Quiet,
FileLocation: cfg.FileLocation,
Level: cfg.Level,
Formatter: adaptLogFormatter(logrus.DefaultTextFormatter()),
Expand Down Expand Up @@ -163,6 +163,10 @@ func (l *LoggingConfig) selectLevel() (logger.Level, error) {
}

func (l *LoggingConfig) AllowUI(stdin fs.File) bool {
if forceNoTTY(os.Getenv("NO_TTY")) {
return false
}

pipedInput, err := isPipedInput(stdin)
if err != nil || pipedInput {
// since we can't tell if there was piped input we assume that there could be to disable the ETUI
Expand All @@ -188,6 +192,16 @@ func (l *LoggingConfig) AllowUI(stdin fs.File) bool {
return l.Verbosity == 0
}

func forceNoTTY(noTTYenvVar string) bool {
switch strings.ToLower(noTTYenvVar) {
case "1":
return true
case "true":
return true
}
return false
}

// isPipedInput returns true if there is no input device, which means the user **may** be providing input via a pipe.
func isPipedInput(stdin fs.File) (bool, error) {
if stdin == nil {
Expand Down
33 changes: 33 additions & 0 deletions logging_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,19 @@ func Test_newLogger(t *testing.T) {
assert.Equal(t, "[0000] INFO test *******\n", stripAnsi(buf.String()))
},
},
{
name: "warn logging is not discarded",
cfg: &LoggingConfig{Level: "warn"},
assertLogger: func(log logger.Logger) {
c, ok := log.(logger.Controller)
require.True(t, ok)
out := c.GetOutput()
// attempt to cast as *os.File, which will fail if output
// has defaulted to io.Discard
_, ok = out.(*os.File)
require.True(t, ok)
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down Expand Up @@ -258,6 +271,26 @@ func TestLoggingConfig_AllowUI(t *testing.T) {
}
}

func TestForceNoTTY(t *testing.T) {
tests := []struct {
envVarValue string
want bool
}{
{"", false},
{"0", false},
{"false", false},
{"other-string", false},
{"1", true},
{"true", true},
}

for _, tt := range tests {
t.Run(tt.envVarValue, func(t *testing.T) {
assert.Equal(t, tt.want, forceNoTTY(tt.envVarValue))
})
}
}

func Test_isPipedInput(t *testing.T) {

tests := []struct {
Expand Down
Loading