Support config imports#3574
Conversation
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
|
Build succeeded.
|
Do not rely on toml metadata when decoding plugin's configs as it's not possible to merge toml.MetaData structs during import. Signed-off-by: Maksym Pavlenko <[email protected]>
|
Build succeeded.
|
Codecov Report
@@ Coverage Diff @@
## master #3574 +/- ##
==========================================
+ Coverage 42.38% 42.54% +0.15%
==========================================
Files 126 127 +1
Lines 13901 14069 +168
==========================================
+ Hits 5892 5985 +93
- Misses 7123 7185 +62
- Partials 886 899 +13
Continue to review full report at Codecov.
|
|
Looks reasonable; just remembered we have a man page for the config.toml file; it may already be missing some config options changes already (and definitely hasn't been updated for v2), but we should probably try and get that updated with info on how to use import and the rules for overwrite? Can be a separate PR |
Signed-off-by: Maksym Pavlenko <[email protected]>
|
I've updated the merging logic a bit so imported files will append |
|
Build succeeded.
|
|
Merging maps makes sense to me, appending slices worries me a bit. Looking through the config I see a few places that could be problematic. For example when configuring a list of endpoints, you may want to override the default but doing a slice append makes that impossible. |
|
@dmcgowan Slice overwrite can be more confusing IMO. For example if user wants to configure one specific plugin in a separate file, but instead overwrites the whole plugin section defined in parent config. |
Signed-off-by: Maksym Pavlenko <[email protected]>
|
Build succeeded.
|
|
@mxpv I understand, getting correct can be very tricky. Generally if you want a configuration to be mergeable, using a map is sufficient. For example for plugins, the plugins have a name which allows this to be done pretty easily. The only case where I see this is potentially an issue is with the stream processors, but we still have a chance to make that a map for this release. Are there other parts you think this would be a problem? I think the current append method is workable since there is no way to force the overwrite. |
|
What about just doing a replace based on the sections? I makes it easier to reason about and the user can just copy paste the section they want to change |
Signed-off-by: Maksym Pavlenko <[email protected]>
Signed-off-by: Maksym Pavlenko <[email protected]>
|
Done. @dmcgowan @crosbymichael PTAL |
|
Build succeeded.
|
|
|
|
Updates look good, I need to take another pass over it before LGTM. I want this in before rc1 though this week. |
fuweid
left a comment
fuweid
left a comment
There was a problem hiding this comment.
do we need a API to get current config for the running containerd? provide overview of merged config to user to check.
| path, pending = pending[0], pending[1:] | ||
|
|
||
| // Check if a file at the given path already loaded to prevent circular imports | ||
| if _, ok := loaded[path]; ok { |
There was a problem hiding this comment.
nit: if loaded[path] more clear :P or use map[string]struct{}
|
@fuweid a command to dump the config would be good, maybe |
Signed-off-by: Maksym Pavlenko <[email protected]>
|
Build succeeded.
|
|
Merge logic looks good, I found one bug in the dump logic that am I seeing if I can fix myself. Also another bug in the proxy plugins but that is a separate issue I will add a PR for. |
|
Build succeeded.
|
Signed-off-by: Derek McGowan <[email protected]>
|
Build succeeded.
|
|
LGTM |
6 participants
Carry #3310
Fixes #3289