Fix configuration dynamic reload logic #1194
Conversation
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Chengxuan
requested review from
peterbroadhurst,
nguyer,
awrichar and
shorsher
as code owners
| @@ -49,8 +49,7 @@ func (nm *namespaceManager) configFileChanged() { | |||
| // the config when it changes and re-read it. | |||
| // We are passed this by our parent, as the config initialization of defaults and sections | |||
| // might include others than under the namespaces tree (API Server etc. etc.) | |||
| nm.resetConfig() | |||
| err := viper.ReadInConfig() | |||
| err := nm.reloadConfig() | |||
There was a problem hiding this comment.
Spent a bunch of time thinking about the reason for this difference...
If you look at https://github.com/spf13/viper/blob/c898f59d3345a8317651876020fe4eb7322a00aa/viper.go#L1543 it does not replace v in the viper package variables.
There was a time in the writing of this code where I established the safest thing to do was to leave v in place because the config file had already been established, and it would retain it.
That way if any code was accessing config live during operation, there would not be a time where that was invalid.
However, this is something we try very hard to avoid in the FireFly core codebase, and copy the config only in constructors of long-lived manager.
I can see from the comments here that eventually I must have had to abandon that and call resetConfig which (when it gets down to coreconfig.Reset()) will do a reset in viper that will replace its v.
Sorry for the long detail, but I agree the change now makes sense, and the thing I was worried about already had had to happen in the last rev of this code.
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Codecov Report
@@ Coverage Diff @@
## main #1194 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 305 305
Lines 20057 20058 +1
=========================================
+ Hits 20057 20058 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -153,7 +156,7 @@ func run() error { | |||
| <-ffDone | |||
| // Re-read the configuration | |||
| resetConfig() | |||
| if err = config.ReadConfig(configSuffix, cfgFile); err != nil { | |||
| if err = reloadConfig(); err != nil { | |||
There was a problem hiding this comment.
Looks like you're calling resetConfig twice in a row along this path.
There was a problem hiding this comment.
......
Good catch there will submit a follow on PR to address this.
fixes #1193