fix: Logger not correctly applying named config

[jsimnz]

Relevant issue(s)

Resolves #693 #694

Description

There was a bug in the #658 that got merged which effectively broke the entire feature. Tests didn't catch it as they were accidentally poorly defined, as they would pass with the default logger settings as well as the custom ones set in the test.

There are two fixes in this PR that resolves the problem. During review, we decided to switch back the Config.Log from Config.Logger, but the viper Bind calls were still using Bind(logger.loglevel) (old name). Additionally, the validate call on the config.Log struct previously always returned nil (no errors), but was changed recently to validate the available options. But, if the --loglevel option is provided it will set the config.Log.Level to error,defra.cli=info,... for example, which would fail validation. I have no moved the validation rules, as it will be validated anyways later in the call stack (in the ToLoggerConfig).

Lastly, it fixes #694, as an accidental oversight, potentially during a rebase/merge-conflict.

Tasks

• I made sure the code is well commented, particularly hard-to-understand areas.
• I made sure the repository-held documentation is changed accordingly.
• I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
• I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Better defined integration tests that don't overlap with defaults.

Specify the platform(s) on which this was tested:

• (modify the list accordingly)
• Ubuntu Linux via WSL2

[codecov]

Codecov Report

Merging #696 (8d76845) into develop (23e728e) will increase coverage by 0.18%.
The diff coverage is 45.45%.

❗ Current head 8d76845 differs from pull request most recent head 27aa302. Consider uploading reports for the commit 27aa302 to get more accurate results

@@             Coverage Diff             @@
##           develop     #696      +/-   ##
===========================================
+ Coverage    57.24%   57.42%   +0.18%     
===========================================
  Files          143      143              
  Lines        16393    16396       +3     
===========================================
+ Hits          9384     9416      +32     
+ Misses        6127     6098      -29     
  Partials       882      882              

Impacted Files	Coverage Δ
cli/cli.go	18.75% <0.00%> (+0.71%)	⬆️
cli/root.go	47.67% <100.00%> (ø)
config/config.go	74.60% <100.00%> (+6.65%)	⬆️
logging/config.go	88.19% <0.00%> (+10.41%)	⬆️

[fredcarle]

Tested and seems to work as expected. Thanks for fixing.

[shahzadlone]

LGTM

[orpheuslummis]

suggestion: additionally support caller key-value for --logger flag.

defradb/cli/cli.go

Lines 145 to 167 in 2e378dd

	// handle field
	switch param := strings.ToLower(parsedKV[0]); param {
	case "level": // string
	logcfg.Level = parsedKV[1]
	case "format": // string
	logcfg.Format = parsedKV[1]
	case "output": // string
	logcfg.OutputPath = parsedKV[1]
	case "stacktrace": // bool
	boolValue, err := strconv.ParseBool(parsedKV[1])
	if err != nil {
	return fmt.Errorf("couldn't parse kv bool: %w", err)
	}
	logcfg.Stacktrace = boolValue
	case "nocolor": // bool
	boolValue, err := strconv.ParseBool(parsedKV[1])
	if err != nil {
	return fmt.Errorf("couldn't parse kv bool: %w", err)
	}
	logcfg.NoColor = boolValue
	default:
	return fmt.Errorf("unknown parameter for logger: %s", param)
	}

[orpheuslummis]

LGTM