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

cmd: Adjust config load logs/errors #6032

Merged
merged 3 commits into from
Mar 5, 2024
Merged

cmd: Adjust config load logs/errors #6032

merged 3 commits into from
Mar 5, 2024

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Jan 10, 2024

Sometimes, users use Caddyfile file names that don't match our adapter-guessing rules, and this causes the config to be loaded as JSON. The error message is very confusing when that happens. I made some adjustments to improve it.

Before:

2024/01/10 19:54:15.805	INFO	using provided configuration	{"config_file": "caddyfile.test", "config_adapter": ""}
Error: loading initial config: decoding request body: invalid character 'd' looking for beginning of object key string

After:

2024/01/10 19:54:32.815	INFO	using config from file	{"file": "caddyfile.test"}
Error: config is not valid JSON: invalid character 'd' looking for beginning of object key string; did you mean to use '--adapter'?

Now, we're quickly doing a json.Unmarshal just to test that the JSON is valid (there is json.Validate() but it only returns a bool and not errors) before returning it to the caller after loading the config file.

Also, I moved the adapter log to be after the adapter guessing logic so that it doesn't show up empty in a confusing way in the logs. I also split the stdin and file log messages for clarity.

I'm not sure about the last bit though ; did you mean to use '--adapter'? is there a better way we could word that without being ambiguous? 🤔 The problems is "what if they actually meant to load JSON" then a mention like this might be confusing for those users. So it's hard to go too far in either direction with the warning.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Jan 10, 2024
@francislavoie francislavoie added this to the v2.8.0 milestone Jan 10, 2024
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

LGTM -- what about this error message?

cmd/main.go Outdated Show resolved Hide resolved
francislavoie and others added 2 commits February 14, 2024 03:13
Co-authored-by: Matt Holt <mholt@users.noreply.github.com>
@mholt mholt enabled auto-merge (squash) March 5, 2024 19:14
@mholt mholt merged commit e473ae6 into master Mar 5, 2024
25 checks passed
@mholt mholt deleted the adjust-load-config-logs branch March 5, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants