feat: add the --redis-compress as the global flag to set redis compression.
#21786
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
| // If we're not in core mode, this function call will do nothing. The Redis Compression has been set | ||
| // to GZip because it is the default compression type. | ||
| // https://github.com/argoproj/argo-cd/issues/13458 | ||
| err := MaybeStartLocalServer(ctx, opts, ctxStr, nil, nil, cache.RedisCompressionGZip, nil) |
There was a problem hiding this comment.
It's probably better to make this a user configurable option with the default being gzip.
There was a problem hiding this comment.
We already have the --redis-compress flag for argocd admin dashboard command with default set to gzip - argocd admin dashboard --redis-compress gzip starts the Web UI with gzip compression.
There was a problem hiding this comment.
So your command should be using the input from that flag and not hard coding the compression value like you are doing rn?
There was a problem hiding this comment.
+1 to @gdsoumya 's question. Instead of setting the gzip compression directly here, should we pick it up from the value of --redis-compress?
There was a problem hiding this comment.
There are three ways to handle this scenario then:
- Search for the
redis-compressflag using theflag.Visit()function and see if it has been set. - Add the flag as a global flag to the root command which means adding it as a client option.
- Change the
NewClientOrDie(opts *apiclient.ClientOptions, c *cobra.Command)function to accept compression type as an argument (but since this function is used at a lot of places, I'm afraid if making changes to this function would be beneficial).
I'd personally suggest leaving the compression type as it is if none of the above ways seems efficient. Any suggestions here? cc @crenshaw-dev
There was a problem hiding this comment.
That's going to lead to a circular dependency in that case.
There was a problem hiding this comment.
Could we add the setting to apiclient.ClientOptions? That would keep the method signature the same but let us set the configurable value.
There was a problem hiding this comment.
I think I just rephrased what you described as number 2. :-) We've got too many global flags, but if any global flags make sense, it's probably this one.
There was a problem hiding this comment.
yes, that's what I was thinking to implement too. Thanks for confirming.
There was a problem hiding this comment.
That's going to lead to a circular dependency in that case|
I do not understand what you mean? We are already doing it just 2 lines above this to get the "context" flag.
ClientOptions was my initial suggestion but I removed it because we already have a flag and we can access it from there. If we can't then we can add a new field to clientopts.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21786 +/- ##
=========================================
Coverage ? 55.64%
=========================================
Files ? 339
Lines ? 56847
Branches ? 0
=========================================
Hits ? 31633
Misses ? 22560
Partials ? 2654 ☔ View full report in Codecov by Sentry. |
nitishfy
force-pushed
the
fix-redis-compression-type
branch
from
b3d13ad to
890a1c8
Compare
nitishfy
changed the title
--redis-compress as the global flag to set redis compression.
Signed-off-by: nitishfy <[email protected]> add --redis-compress flag Signed-off-by: nitishfy <[email protected]> remove comment Signed-off-by: nitishfy <[email protected]> fix lint checks Signed-off-by: nitishfy <[email protected]> run codegen Signed-off-by: nitishfy <[email protected]>
nitishfy
force-pushed
the
fix-redis-compression-type
branch
from
4ed23fb to
ba1e892
Compare
Closes #21742
Checklist:
After