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.

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

Improve logging for csaf_aggregator when no config file is present #538

Open
lebogg opened this issue Apr 25, 2024 · 1 comment
Open

Improve logging for csaf_aggregator when no config file is present #538

lebogg opened this issue Apr 25, 2024 · 1 comment

Comments

@lebogg
Copy link

lebogg commented Apr 25, 2024

What happened?

I tried to test the csaf_aggregator by building and just running it without any parameter, in particular without a path to a config file. Then, of course, the aggregator was looking into the default paths but couldn't find any config file.
I got the following error message:
error: no providers given in configuration
This is due a check in the prepare function of config.go where the providers of the passed config is checked:
https://github.com/csaf-poc/csaf_distribution/blob/086c4ab48b292b05b2382bf7b26d99ed8b346a0d/cmd/csaf_aggregator/config.go#L456-L458

What would I expect?

For me, the error code is misleading since it suggests that just the providers are missing in the config file, but the whole config file is missing in the first place. Therefore, having, e.g., a typo in the passed path to the config file lets the user in the unknown that the path or the file was actually wrong. I would expect an error or additional warning message indicating sth. like

WARN: config file is not parsed: there is no config file in the provided or default paths
ERROR: no providers given in configuration

Suggestion

A simple solution would be to print a warning log message when the config file couldn't be found in the generic Parse() function if it doesn't clash with the usage in other places (it is still just a warning message though).
https://github.com/csaf-poc/csaf_distribution/blob/7a8cdb6d19271ac1b9d5a0a93891056a9f3994cc/internal/options/options.go#L47

For example:

@@ -12,6 +12,7 @@ package options
 import (
        "fmt"
        "log"
+       "log/slog"
        "os"
 
        "github.com/BurntSushi/toml"
@@ -81,6 +82,7 @@ func (p *Parser[C]) Parse() ([]string, *C, error) {
 
        // No config file -> We are good.
        if path == "" {
+               slog.Warn("No config file found. Maybe you want to specify one or store it in a respective default location")
                return args, &cmdLineOpts, nil
        }
 
@bernhardreiter
Copy link
Member

Hi @lebogg
thanks for your suggestion and for testing the aggregator!

If we can improve the messaging here, that looks like a good thing, we'd accept a pull request for this.

Note that in many cases it is fine to have no config file and just use the defaults. However for the aggregator that maybe different from the general case.

What about logging the tried config file path in general.
And possible add a hint to the aggregator error message to also check the place of the configuration file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants