Add conf-dir option and merge toml files#3310
Add conf-dir option and merge toml files#3310yylt wants to merge 1 commit intocontainerd:masterfrom yylt:dev
Conversation
|
Build succeeded.
|
|
recheck |
|
Build succeeded.
|
mxpv
left a comment
mxpv
left a comment
There was a problem hiding this comment.
Can you add tests to make sure merging logic works as expected?
|
|
||
| // MergeConfd search file in confd , require filename like [0-0]-xx.toml | ||
| // merge conf by Positive sequence into config | ||
| func MergeConfd(config *srvconfig.Config, confd string) error { |
There was a problem hiding this comment.
I think config merging logic should reside in config package as well.
There was a problem hiding this comment.
should I mock some .toml file in config directory? I think it's useless when use empty directory .
There was a problem hiding this comment.
I'm not sure what you mean by mocking a .toml file. I suggest to split the code up into something like:
- config.LoadFromFile(path string) <- loads a Config from a single file.
- config.merge(cfg ...Config) <- merges existing set of Config structs into one final struct
- config.search(dir, mask string) <- searches config files by mask
- config.LoadFromDir(dir string) <- search+LoadFromFile+merge
This way you can test merge/search/load easily.
There was a problem hiding this comment.
thank your suggestion, I will modify and push it later
|
What is the logic for how the configs are merged? Does it go by sections in the toml file and replaces that entire section or does it go by fields ? |
i think it should go by sections and replaces section when section is not null |
|
Does this change make the |
@crosbymichael This still makes the merging logic/file order a bit unclear. |
|
@mxpv I like that idea as it's explicit and people understand the changes. @Random-Liu @yylt does this work for your usecases or does it still introduce issues with editing the master config? |
In the beginning , add I'm sorry for have not work on it for a long time |
|
This one has been carried |
5 participants
Fix #3289
Signed-off-by: yylt [email protected]