rpk: improve handling of incomplete config import #4910
Conversation
jcsp
force-pushed
the
issue-4877
branch
2 times, most recently
from
dcf1bc4
to
bc82251
Compare
Previously this would compare all the existing configuration values against the new ones, including if the new one was nil (reset to default), and the old one was the default. That meant that an import of a minimal file would generate a huge list of changes, and also a bunch of redundant 'remove' entries in the API request. Functional, but messy. Fix this by only comparing with non-default existing properties when doing an import. The 'edit' path still compares with all properties. Fixes redpanda-data#4877
jcsp
requested review from
dotnwat,
NyaliaLui,
twmb,
0x5d,
r-vasquez and
a team
as code owners
There was a problem hiding this comment.
I'm a bit confused here -- the PR description refers to redundant remove entries, but the code changes in import primarily affect the upsert path. The remove path continues to read through all old config values, but now the oldValue is coming from non-defaults? And the non-default could not exist because the key wouldn't be changed yet, but we're still removing the key?
| @@ -171,7 +171,7 @@ func (*unavailableError) Error() string { | |||
| return "unavailable" | |||
| } | |||
|
|
|||
| func (m *mockAdminAPI) Config() (admin.Config, error) { | |||
| func (m *mockAdminAPI) Config(_ bool) (admin.Config, error) { | |||
There was a problem hiding this comment.
Nit: this can be (bool). Only worth addressing if there are other comments.
| @@ -23,9 +23,12 @@ type Config map[string]interface{} | |||
|
|
|||
| // Config returns a single admin endpoint's configuration. This errors if | |||
| // multiple URLs are configured. | |||
| func (a *AdminAPI) Config() (Config, error) { | |||
| // If include_defaults is true, all properties are returned, including those | |||
There was a problem hiding this comment.
nit: include_defaults => includeDefaults, also an empty comment line just above this (//) so that it's not a partial comment line into a full comment line
| } | ||
| } | ||
|
|
||
| for k := range oldConfig { | ||
| for k := range oldConfigFull { |
There was a problem hiding this comment.
Line 146 uses oldConfig still, which I think given this PR, could not exist if it is a default value?
|
Sorry, this was meant to be a quickie but I didn't have time to follow up + it got stale, I'm going to close it + let #4877 get re-prioritized for an rpk specialized to work on. |
Cover letter
Previously this would compare all the existing configuration
values against the new ones, including if the new one was
nil (reset to default), and the old one was the default.
That meant that an import of a minimal file would generate
a huge list of changes, and also a bunch of redundant 'remove'
entries in the API request. Functional, but messy.
Fix this by only comparing with non-default existing properties
when doing an import. The 'edit' path still compares with all
properties.
Fixes #4877
Release notes
Improvements
rpk cluster config importis made more succinct if the input file leaves out properties and those properties are already set to the default.