Skip to content

use log level configuration from database#28290

Open
tolot27 wants to merge 8 commits intoevcc-io:masterfrom
tolot27:dbloglevel
Open

use log level configuration from database#28290
tolot27 wants to merge 8 commits intoevcc-io:masterfrom
tolot27:dbloglevel

Conversation

@tolot27
Copy link
Contributor

@tolot27 tolot27 commented Mar 16, 2026

First part for #25763. The default log level can be specified using "default" as key.

What is missing:

  • GUI configuration

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • The call to parseLogLevels has moved to only occur after configureDatabase succeeds, which means log level configuration will not be applied for database-related errors anymore; consider whether you want to preserve the previous behavior of having logging configured before attempting DB setup.
  • The parseLogLevels function now has side effects on viper (setting level and levels) in addition to parsing; if this is intentional, consider updating the function name or adding a brief comment to make the mutating behavior explicit for future callers.
  • There is a commented-out viper.Set("log", conf.Log) in configureEnvironment; please either remove it or document why it must remain commented to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The call to `parseLogLevels` has moved to only occur after `configureDatabase` succeeds, which means log level configuration will not be applied for database-related errors anymore; consider whether you want to preserve the previous behavior of having logging configured before attempting DB setup.
- The `parseLogLevels` function now has side effects on `viper` (setting `level` and `levels`) in addition to parsing; if this is intentional, consider updating the function name or adding a brief comment to make the mutating behavior explicit for future callers.
- There is a commented-out `viper.Set("log", conf.Log)` in `configureEnvironment`; please either remove it or document why it must remain commented to avoid confusion.

## Individual Comments

### Comment 1
<location path="cmd/setup.go" line_range="504-513" />
<code_context>
 	err := wrapErrorWithClass(ClassDatabase, configureDatabase(conf.Database))

+	// load persisted log levels and apply logging configuration
+	if err == nil {
+		if settings.Exists(keys.Levels) {
+			if err := migrateYamlToJson(keys.Levels, &conf.Levels); err != nil {
+				return err
+			}
+		}
+		if len(conf.Levels) > 0 {
+			viper.Set("levels", conf.Levels)
+		} else {
+			log.INFO.Printf("No log levels configured in database, using defaults from command line: %v", conf.Log)
+			//viper.Set("log", conf.Log)
+		}
+		parseLogLevels()
+	}
+
</code_context>
<issue_to_address>
**question (bug_risk):** Skipping `parseLogLevels` when `configureDatabase` fails might leave logging in an inconsistent state during error handling.

Because `parseLogLevels()` now runs only when `configureDatabase` succeeds, any error-handling paths that log database issues may run with whatever logging state existed before `configureEnvironment` (e.g., CLI defaults or none). If you want consistent logging even on DB setup failure, consider invoking `parseLogLevels()` regardless of `configureDatabase`’s result (possibly in a reduced form using only CLI/config values).
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@naltatis naltatis added the waiting for feedback Suspended progress label Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting for feedback Suspended progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants