librespeed / speedtest-go

Go backend for LibreSpeed
GNU Lesser General Public License v3.0
706 stars 153 forks source link

Custom config file not working (-c) #15

Closed sooslaca closed 3 years ago

sooslaca commented 3 years ago

Description

Custom config file not being parsed.

Steps to reproduce

Copy settings.toml to a different directory Change eg listen_port to 8990 start: speedtest-go.exe -c c:\path_to_file\settings.toml Program still bind to 8989

Expected behaviour

Use given config

Findings

viper: Reading Config Files says: viper.SetConfigType("yaml") // REQUIRED if the config file does not have the extension in the name

https://github.com/librespeed/speedtest-go/blob/master/config/config.go#L84

There is no ConfigType defined in here

Either should use ReadInConfig or shall set ConfigType before ReadConfig.

Thanks

maddie commented 3 years ago

Thanks for your report.

I've tested locally and I cannot reproduce your issue, though I've only tested on Linux and macOS. And also there have been PRs against the configuration part in the past, and during the testing, no such problem was found.

Check your settings.toml again and make sure the values are right and also the path to it. I'm not really sure what's happening here, but if I had to guess, it could be the way that Go handles the path string under Windows. Maybe you can try adding quotes to the path or escaping the path separator (e.g. C:\\path_to_file\\settings.toml).

For the viper documentation part you've referenced to, it says it's required only if the config file given does not have an extension name. The fact that you gave ....\settings.toml to the -c option, it already tells Viper to parse it as a toml file. It's not necessary to set it explicitly to toml, and I didn't do so because there might be some user that is used to another format (i.e. JSON) for their configuration and they can still use it.

sooslaca commented 3 years ago

Thanks for checking, I’m away now cannot check until Tuesday. Maybe your go cache using older viper?

In the config.go code I remember you gave io.Reader as a parameter to viper. ReadConfig Hence it would not matter what the file suffix is as that routine will never see that information, given that the file is opened files before for reading, right?

What did you try with? Toml format from a different directory? I did try that, maybe yaml is the default for viper that’s why it worked.

Thank you

maddie commented 3 years ago

Hmm, I don't think I've used an io.Reader, I just used viper.ReadInConfig:

https://github.com/librespeed/speedtest-go/blob/c815103e20b0f090ee8973866bc85ffafe8dc0c1/config/config.go#L54-L71

I don't even have an io.Reader in the whole config.go file.

And I think it rules out using the older version of viper since we're using Go modules here unless the viper authors published a bug fixed release under the same version, which I doubt that they would do.

sooslaca commented 3 years ago

I might be wrong, but Load only kicks in if you don't give config file: https://github.com/librespeed/speedtest-go/blob/c815103e20b0f090ee8973866bc85ffafe8dc0c1/main.go#L25

you need to look in LoadFile: https://github.com/librespeed/speedtest-go/blob/c815103e20b0f090ee8973866bc85ffafe8dc0c1/config/config.go#L84

maddie commented 3 years ago

Hmm, I see, I was looking at the wrong place.

I've pushed a commit to let Viper read the file instead of me opening it manually, and I've tested it to be working for both toml and json.

Could you try compiling the master branch and see if it works correctly for you now?

Thanks.

sooslaca commented 3 years ago

awesome, thank you ! It works like a charm now.

maddie commented 3 years ago

Thanks, I'll make a new release now :)