refactor: refactor config file resolve #1132
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1132 +/- ##
==========================================
- Coverage 16.06% 16.04% -0.02%
==========================================
Files 173 174 +1
Lines 9930 9992 +62
==========================================
+ Hits 1595 1603 +8
- Misses 8218 8272 +54
Partials 117 117
|
daemon/config/config.go
Outdated
| @@ -110,3 +115,96 @@ func (cfg *Config) Validate() error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| //MergeConfigConfigurations merges flagSet flags and config file flags into Config. | |||
| func (cfg *Config) MergeConfigConfigurations(config *Config, flagSet *pflag.FlagSet) error { | |||
There was a problem hiding this comment.
merge config configuration, do we have duplicated config in this function naming?
There was a problem hiding this comment.
how about MergeConfiguration.
There was a problem hiding this comment.
yeah, that is much better.
| } | ||
|
|
||
| // iterateConfig resolves key-value from config file iteratly. | ||
| func iterateConfig(origin map[string]interface{}, config map[string]interface{}) { |
There was a problem hiding this comment.
I think we need to make this function some kind of robust. If the config is nil, I am afraid this function would panic. How about doing this nil-checking to improve robustness?
There was a problem hiding this comment.
I refer to make this function before iterateConfig, so that origin can not be nil, and since it not a common function, will not be used by other function ,add nil check is redundancy
if len(origin) == 0 {
return nil
}
fileFlags := make(map[string]interface{}, 0)
iterateConfig(origin, fileFlags)
There was a problem hiding this comment.
I still insist on make functions independent from each other. As if you encapsulate this function out of the another place which is calling this. It has a trend to be general. If that, we cannot prevent others to use it in the future, as a result, there will be potential possibility to panic.
In addition, if the code which is calling this function changes or removes the nil checking in the upper layer, there will be potential panic as well.
There was a problem hiding this comment.
for range a nil map will never cause panic, since nil value will convert to a nil map(map[] ), that add a nil check is useless.
1. move daemon config code from main.go to daemon/config/config.go. 2. supports to resolve config file contains nested object. 3. skip check conflict in slice type flags and merge them. Signed-off-by: Ace-Tang <aceapril@126.com>
| return | ||
| } | ||
| if v, exist := fileFlags[f.Name]; exist { | ||
| conflictFlags = append(conflictFlags, fmt.Sprintf("from flag: %s and from config file: %s", f.Value.String(), v)) |
There was a problem hiding this comment.
I think we need to remove : in the error message.
Otherwise the returned error is quite unreadable.
|
LGTM |
pouchrobot
added
the
LGTM
Ⅰ. Describe what this PR did
move daemon config code from main.go to daemon/config/config.go,
suporrts to resolve config file contains nested object.
for eg, resolve file
to
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews