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

default log levels in config file (revert breaking change) #3117

Closed
antiochp opened this issue Nov 14, 2019 · 3 comments · Fixed by #3124
Closed

default log levels in config file (revert breaking change) #3117

antiochp opened this issue Nov 14, 2019 · 3 comments · Fixed by #3124
Labels
Milestone

Comments

@antiochp
Copy link
Member

We tweaked log level config here - #3064

It used to be Info but now we also support INFO.

But - we now generate config file with default entries like -

#log level for file: Error, Warning, Info, Debug, Trace
file_log_level = "INFO"

These actually introduce a breaking change with previous code which does not support them
where Info is supported, not INFO.

We should revert the config file generation to add default log level consistent with previous behavior (and also consistent with config file comments).

#log level for file: Error, Warning, Info, Debug, Trace
file_log_level = "Info"
@antiochp antiochp added the bug label Nov 14, 2019
@antiochp antiochp added this to the 3.0.0 milestone Nov 14, 2019
@antiochp
Copy link
Member Author

For context this is what we see with previous code and new default config file -

thread 'main' panicked at 'Error loading server configuration: 
Error parsing configuration file at /antiochp/grin/node_mainnet/grin-server.toml - 
unknown variant `INFO`, 
expected one of `Error`, `Warning`, `Info`, `Debug`, `Trace` 
for key `logging.stdout_log_level`', src/bin/grin.rs:124:7
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

@JosephGoulden
Copy link
Contributor

It's a bit awkward because now we are using the log levels from the log crate, which when serialised are upper case. And the config file is generated by just serialising the config struct that has LogLevels as fields.

Couple of options, neither of which I particularly like.

  1. Apply some post config generation step to get the log levels into the correct (e.g. Info) format.
  2. Change the type from LogLevel to String on the LoggingConfig. This would work okay but we lose type safety.

Can we definitely not allow the change? Given that it only breaks if you used new code to generate the config file then reverted to a previous code version.

@antiochp
Copy link
Member Author

Either 1 or 2 would work fine here I think. It makes sense why this happened but we should avoid introducing breaking changes like this for little or no benefit.

Maybe the default value for these logging config entries simply post-process to get the capitalization to follow the existing format.

JosephGoulden added a commit to JosephGoulden/grin that referenced this issue Nov 18, 2019
antiochp pushed a commit that referenced this issue Nov 18, 2019
#3124)

* fix: #3117 For backwards compatibility only capitalise first letter of log level in config file

* fix: For forwards compatibility old config needs Warning log level changed to standard log::Level WARN

* refactor: renamed some variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants