chirpstack / chirpstack-gateway-bridge

ChirpStack Gateway Bridge abstracts Packet Forwarder protocols into Protobuf or JSON over MQTT.
https://www.chirpstack.io
MIT License
423 stars 272 forks source link

Adding mapstructure tags in config variable. #163

Closed sophiekovalevsky closed 4 years ago

sophiekovalevsky commented 4 years ago

Is this a bug or a feature request?

Neither of both. Contribution.

What did you expect?

What happened?

When inspecting internal/config/config.go I found that not all of the fields contains the tag mapstructure:"value". As an example:

General struct {
  LogLevel    int  `mapstructure:"log_level"`
  LogToSyslog bool `mapstructure:"log_to_syslog"`
}

Does this declarion should not be as following:

General struct {
  LogLevel    int  `mapstructure:"log_level"`
  LogToSyslog bool `mapstructure:"log_to_syslog"`
} `mapstructure:"general"`

What version are your using?

v3.7.1

Could you share your log output?

I have seen that, when there is no mapstructure provided, if I set:

viper.SetDefault("general.log_level", 4)

This is mapped directly to log_level field, even though there is no explicit mapstructure tag over General struct.

There is a reason why this approach was taken?

If this is something that can be improved by adding the explicit mapstructure into each missing struct and field tag, then, I would be happy to contribute with this. I just want to be sure whether this was on purpose or maybe I am missing something.

brocaar commented 4 years ago

Initially I added the mapstructure tag only where it was needed (as General is implicitly mapped to general, it is not really needed but adding a mapstructure:"general" does make it more explicit).

I'm happy to merge a pull-request where all all fields have an explicit mapstructure :)

sophiekovalevsky commented 4 years ago

@brocaar thanks to explain. All right then, I'm gonna work on this and once is finished the pull request will be generated.

sophiekovalevsky commented 4 years ago

@brocaar I am on my way to close this issue since most of the changes have been introduced to the main code, the pull request at geolocation is planned to happen?

brocaar commented 4 years ago

@sophiekovalevsky sorry, forgot to merge the last one :-) All your work has been merged now. Thanks :+1: