Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rpk: cluster config import should only print changes for properties named in the input, or which are non-default #4877

Closed
jcsp opened this issue May 23, 2022 · 5 comments · Fixed by #7229
Closed

rpk: cluster config import should only print changes for properties named in the input, or which are non-default #4877

jcsp opened this issue May 23, 2022 · 5 comments · Fixed by #7229
Labels
area/rpk kind/enhance New feature or request
Milestone
v22.3.3

Comments

@jcsp
Copy link
Contributor

jcsp commented May 23, 2022 •
edited
Loading

It is legal for the user to supply a file that contains only the properties they want to set to a non-default value.

Currently, if they do this then they get an output like this (in this example the input file contains only the superusers property)

rpk2212 cluster config import --filename /tmp/config.yml
PROPERTY                                     PRIOR            NEW
superusers                                   []               [admin]
cloud_storage_enabled                        false            
kafka_connections_max_overrides              []               
delete_retention_ms                          6.048e+08        
enable_metrics_reporter                      true
...

The code that calculates the delta for user presentation should only include a property if:

  • The existing value is non-default and the property is absent from the input (i.e. it is being reset to default)
  • or, the property is present in the user input, and its value differs from the current value.
@jcsp jcsp added kind/enhance New feature or request area/rpk labels May 23, 2022
@jcsp
Copy link
Contributor Author

jcsp commented May 23, 2022

The GET method for cluster_config has an include_defaults flag that enables fetching only those properties which are set to a non-default value: that should be sufficient for calculating the cleaned up version of this delta list.

@twmb
Copy link
Contributor

twmb commented May 23, 2022

How much can this break current behavior? Should we prioritize this for a backport ASAP?

jcsp added a commit to jcsp/redpanda that referenced this issue May 24, 2022
@jcsp
rpk: improve handling of incomplete config import
be64560 
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 jcsp mentioned this issue May 24, 2022
Closed
jcsp added a commit to jcsp/redpanda that referenced this issue May 24, 2022
@jcsp
rpk: improve handling of incomplete config import
dcf1bc4 
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 added a commit to jcsp/redpanda that referenced this issue May 25, 2022
@jcsp
rpk: improve handling of incomplete config import
bc82251 
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 added a commit to jcsp/redpanda that referenced this issue May 25, 2022
@jcsp
rpk: improve handling of incomplete config import
2e3a374 
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
Copy link
Contributor Author

jcsp commented Aug 24, 2022

Earlier attempt at fix here #4910

@jcsp
Copy link
Contributor Author

jcsp commented Aug 24, 2022

@twmb flagging for your consideration for 22.3

@shane-runsafe shane-runsafe mentioned this issue Oct 16, 2022
Open
@secpanda secpanda mentioned this issue Oct 16, 2022
Merged
@snyk-bot snyk-bot mentioned this issue Oct 19, 2022
Closed
@twmb
Copy link
Contributor

twmb commented Oct 21, 2022 •
edited
Loading

Likely v23.1

@piyushredpanda piyushredpanda modified the milestones: v22.3.1-rc5, v22.3.2 Nov 9, 2022
r-vasquez pushed a commit to r-vasquez/redpanda that referenced this issue Nov 10, 2022
@jcsp @r-vasquez
rpk: improve handling of incomplete config import
f4e5539 
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

(cherry picked from commit 2e3a374)
@r-vasquez r-vasquez mentioned this issue Nov 10, 2022
Merged
6 tasks
r-vasquez pushed a commit to r-vasquez/redpanda that referenced this issue Nov 10, 2022
@jcsp @r-vasquez
rpk: improve handling of incomplete config import
aa52cfa 
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

(cherry picked from commit 2e3a374)
r-vasquez pushed a commit to r-vasquez/redpanda that referenced this issue Nov 10, 2022
@jcsp @r-vasquez
rpk: improve handling of incomplete config import
5a63bd1 
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

(cherry picked from commit 2e3a374)
@snyk-bot snyk-bot mentioned this issue Nov 20, 2022
Closed
r-vasquez pushed a commit to r-vasquez/redpanda that referenced this issue Jan 24, 2023
@jcsp @r-vasquez
rpk: improve handling of incomplete config import
fc25872 
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

(cherry picked from commit 2e3a374)
(cherry picked from commit 5a63bd1)
r-vasquez pushed a commit to r-vasquez/redpanda that referenced this issue Jan 24, 2023
@jcsp @r-vasquez
rpk: improve handling of incomplete config import
196dad8 
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

(cherry picked from commit 2e3a374)
(cherry picked from commit 5a63bd1)
joejulian pushed a commit to joejulian/redpanda that referenced this issue Mar 10, 2023
@jcsp @r-vasquez
(split) rpk: improve handling of incomplete config import
d7778a4 
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

(cherry picked from commit 2e3a374)
joejulian pushed a commit to joejulian/redpanda that referenced this issue Mar 24, 2023
@jcsp @r-vasquez
operator: rpk: improve handling of incomplete config import
5b6dac5 
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

(cherry picked from commit 2e3a374)
joejulian pushed a commit to joejulian/redpanda that referenced this issue Apr 12, 2023
@jcsp @joejulian
rpk: improve handling of incomplete config import
e091683 
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

(cherry picked from commit 2e3a374)
(cherry picked from commit 5a63bd1)
joejulian pushed a commit to joejulian/redpanda that referenced this issue Apr 12, 2023
@jcsp @joejulian
rpk: improve handling of incomplete config import
ae9754d 
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

(cherry picked from commit 2e3a374)
(cherry picked from commit 5a63bd1)
joejulian pushed a commit to joejulian/redpanda that referenced this issue Apr 13, 2023
@jcsp @joejulian
rpk: improve handling of incomplete config import
4aec082 
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

(cherry picked from commit 2e3a374)
(cherry picked from commit 5a63bd1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/rpk kind/enhance New feature or request
Projects
None yet
Milestone
v22.3.3
3 participants
@jcsp @twmb @piyushredpanda