-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
fix(cli): pass redis compression to cluster stats and shards commands (#16060) #16421
fix(cli): pass redis compression to cluster stats and shards commands (#16060) #16421
Conversation
Co-authored-by: clavinjune <24659468+clavinjune@users.noreply.github.com> Signed-off-by: phanama <yudiandreanp@gmail.com>
Co-authored-by: clavinjune <24659468+clavinjune@users.noreply.github.com> Signed-off-by: phanama <yudiandreanp@gmail.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #16421 +/- ##
==========================================
- Coverage 49.55% 49.52% -0.03%
==========================================
Files 269 269
Lines 47046 47240 +194
==========================================
+ Hits 23313 23395 +82
- Misses 21445 21546 +101
- Partials 2288 2299 +11 ☔ View full report in Codecov by Sentry. |
Co-authored-by: clavinjune <24659468+clavinjune@users.noreply.github.com> Signed-off-by: phanama <yudiandreanp@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @phanama , LGTM!!
…argoproj#16060) (argoproj#16421) Signed-off-by: Kevin Lyda <kevin@lyda.ie>
Both
admin cluster stats
andadmin cluster shards
command still use hardcodedRedisCompressionNone
. In case users are using Redis gzip compression, these commands return incorrect output. We should pass down the configuration from the--redis-compress
flag (default gzip) to these subcommands.Example bad output:
After the fix:
The shard # number is still incorrect though, showing only
0
as the shard number. Most likely it's a regression introduced in #13018I will raise another PR later to address this.
Partially fixes: #16060 (everything except the shard # number issue)
cc @clavinjune
Checklist:
Either (a) I've created an enhancement proposal and discussed it with the community,(b) this is a bug fix,or (c) this does not need to be in the release notes.