*: fix invalid key in example cfg file and support log_level from cfg file #1330
Conversation
|
After verification, the configuration of the log level "log-level" in the configuration file does not take effect. This is because the third-party library (github.com/mreiferson/go-options) can only recognize the "log_level". This patch fixes the configuration file Use "log_level" to set the log level. For "log-level", the old application behavior is still maintained. |
edoger
changed the title
mreiferson
changed the title
There was a problem hiding this comment.
Thanks for this comprehensive PR @edoger, a few changes if you don't mind!
nsqadmin/options.go
Outdated
| @@ -59,3 +62,23 @@ func NewOptions() *Options { | |||
| AdminUsers: []string{}, | |||
| } | |||
| } | |||
|
|
|||
| func (opts *Options) Resolve(flagSet *flag.FlagSet, cfg map[string]interface{}) error { | |||
There was a problem hiding this comment.
We already have a way to coerce config data into appropriate option types, see:
https://github.com/nsqio/nsq/blob/master/apps/nsqd/options.go#L69
So we don't actually need this new method for any of the binaries.
There was a problem hiding this comment.
@mreiferson Do we need to migrate the Validate method to Resolve? Because nsqadmin and nsqlookupd have no corresponding validate methods.
There was a problem hiding this comment.
No, I think each binary should have a config file validation method rather than moving that responsibility into the package / options.
apps/nsqd/main.go
Outdated
| @@ -64,7 +63,9 @@ func (p *program) Start() error { | |||
| } | |||
| cfg.Validate() | |||
|
|
|||
| options.Resolve(opts, flagSet, cfg) | |||
| if err := opts.Resolve(flagSet, cfg); err != nil { | |||
There was a problem hiding this comment.
This can be reverted as well after addressing my previous comment. For better or worse, options.Resolve fatally exits on errors.
|
@mreiferson Done! The test failure doesn't seem to be caused by the code? |
|
Changes look great, tests are failing due to |
|
@mreiferson Done! The code style has been fixed and everything looks good. Since the logFatal function cannot be unit tested, the decrease in test coverage is an inevitable result. |
No description provided.