In LOLWUT's reply, change "Redis ver." to "Valkey ver." #1559

sarthakaggarwal97 · 2025-01-14T19:52:08Z

Make LOLWUT it print the current Valkey version rather than the Redis version. Retain the old behaviour and print the compatible Redis version if the extended-redis-compat config is enabled.

Additionally, change "Redis" to "Valkey" in an error message "This Redis command is not allowed from script", also depending on the extended-redis-compat config.

codecov · 2025-01-14T20:07:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.00%. Comparing base (12ec3d5) to head (cf8971d).
Report is 106 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1559      +/-   ##
============================================
+ Coverage     70.98%   71.00%   +0.01%     
============================================
  Files           121      121              
  Lines         65176    65176              
============================================
+ Hits          46264    46276      +12     
+ Misses        18912    18900      -12

Files with missing lines	Coverage Δ
src/lolwut.c	`21.42% <100.00%> (+21.42%)`	⬆️
src/script.c	`87.69% <100.00%> (ø)`

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

zuiderkwast

Good, but we can avoid hard-coding the name.

(It's weird that we have SERVER_TITLE but we decided to name the version variable VALKEY_VERSION instead of SERVER_VERSION. Maybe there was a reason for that. I don't remember the discussion.)

src/lolwut.c

ranshid · 2025-01-15T15:48:24Z

Trying to understand the issue. AFAIK changing the lolwut can break some clients using it in order to get the current verison.
I agree that providing the unstable version is problematic and we might want to do the same as we did with the info server version (i.e use fixed 7.2.4).

The other issue is that we also have the "Easter eggs" lolwat (which are somewhat broken, for example you cannot enter lolwut which are also presenting the VALKEY_VERSION version and Redis brand (I wonder if these we can be changed).

In anycase I think this should be marked as a breaking change

ranshid · 2025-01-27T08:18:31Z

@sarthakaggarwal97 I just looked. seems like @zuiderkwast suggestion to control this feature via extended-redis-compat might be fine.

sarthakaggarwal97 · 2025-01-27T19:00:22Z

@ranshid @zuiderkwast if I understand this correctly, extended-redis-compat is stemming from #274 right?

I see we have a configuration available in valkey.conf as extended-redis-compatibility. So we can continue with this change relying on this configuration?

zuiderkwast · 2025-01-27T19:26:47Z

@ranshid @zuiderkwast if I understand this correctly, extended-redis-compat is stemming from #274 right?

I see we have a configuration available in valkey.conf as extended-redis-compatibility. So we can continue with this change relying on this configuration?

That's right. I think it's OK to use this config for lolwut. We should make a TSC decision about it though.

I remember when we added this config, there were some concerns that it will give us a lot of extra maintenance work, so we decided to limit the scope to only a few things and to make it temporary and be phased out in 9.0. See the test "Extended Redis Compatibility config" in tests/unit/other.tcl

src/lolwut.c

ranshid · 2025-02-03T12:23:07Z

Will just ask again:
why not rebrand the lolwut5Command and lolwut6Command?

sarthakaggarwal97 · 2025-02-06T22:14:47Z

@ranshid apologies, but could you please elaborate on the rebranding on commands? What exactly would be required here?

PingXie · 2025-02-08T18:45:56Z

I think it's OK to use this config for lolwut. We should make a TSC decision about it though.

+1

zuiderkwast · 2025-03-31T22:22:49Z

We discussed it in a meeting today and decided to use the extended redis compat config for this as implemented in this PR, but the same change needs to be done in lolwut5.c and lolwut6.c too. Waiting for this change before merging.

Signed-off-by: Sarthak Aggarwal <[email protected]>

sarthakaggarwal97 · 2025-04-02T20:23:13Z

@zuiderkwast added extended redis compat config for lolwut5, lolwut6 as well. Please check once!

zuiderkwast

LGTM, thanks.

@ranshid Please take another look.

tests/unit/other.tcl

enjoy-binbin · 2025-04-03T07:08:39Z

please also update the top comment.

Signed-off-by: Sarthak Aggarwal <[email protected]>

sarthakaggarwal97 · 2025-04-03T21:43:23Z

@enjoy-binbin added those suggestions!

zuiderkwast · 2025-04-03T21:54:20Z

please also update the top comment.

Done. Now describing the exact behavior with the config. @sarthakaggarwal97 please note.

sarthakaggarwal97 · 2025-04-03T21:55:58Z

@zuiderkwast ah sorry, I got confused. I thought it was just for the suggestion. thank you.

zuiderkwast · 2025-04-03T21:57:51Z

src/script.c

    /* There are commands that are not allowed inside scripts. */
    if (!server.script_disable_deny_script && (cmd->flags & CMD_NOSCRIPT)) {
-        *err = sdsnew("This Redis command is not allowed from script");
+        *err = sdscatprintf(sdsempty(), "This %s command is not allowed from script", server.extended_redis_compat ? "Redis" : "Valkey");


I think we should mention this change too in the top comment.

Make LOLWUT it print the current Valkey version rather than the Redis version. Retain the old behaviour and print the compatible Redis version if the `extended-redis-compat` config is enabled. Additionally, change "Redis" to "Valkey" in an error message "This Redis command is not allowed from script", also depending on the `extended-redis-compat` config. --------- Signed-off-by: Sarthak Aggarwal <[email protected]> Signed-off-by: Shai Zarka <[email protected]>

Make LOLWUT it print the current Valkey version rather than the Redis version. Retain the old behaviour and print the compatible Redis version if the `extended-redis-compat` config is enabled. Additionally, change "Redis" to "Valkey" in an error message "This Redis command is not allowed from script", also depending on the `extended-redis-compat` config. --------- Signed-off-by: Sarthak Aggarwal <[email protected]>

sungming2 approved these changes Jan 15, 2025

View reviewed changes

enjoy-binbin approved these changes Jan 15, 2025

View reviewed changes

enjoy-binbin requested a review from zuiderkwast January 15, 2025 06:07

zuiderkwast reviewed Jan 15, 2025

View reviewed changes

src/lolwut.c Outdated Show resolved Hide resolved

ranshid added breaking-change Indicates a possible backwards incompatible change client-changes-needed Client changes may be required for this feature rebranding Valkey is not Redis labels Jan 15, 2025

sarthakaggarwal97 force-pushed the nit-rename branch from 017d7ab to c53181f Compare January 27, 2025 20:42

hwware approved these changes Jan 30, 2025

View reviewed changes

hwware added the major-decision-pending Major decision pending by TSC team label Jan 30, 2025

zuiderkwast reviewed Jan 30, 2025

View reviewed changes

src/lolwut.c Outdated Show resolved Hide resolved

sarthakaggarwal97 force-pushed the nit-rename branch from c53181f to cf8971d Compare January 30, 2025 18:25

zuiderkwast added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Mar 31, 2025

sarthakaggarwal97 added 4 commits April 2, 2025 11:15

nit rename to valkey

fbcc203

Signed-off-by: Sarthak Aggarwal <[email protected]>

depending on extended-redis-compat

e750a13

Signed-off-by: Sarthak Aggarwal <[email protected]>

keeping fixed redis version

1c7bee1

Signed-off-by: Sarthak Aggarwal <[email protected]>

extended redis compat for lolwut5 and lolwut6

92f4645

Signed-off-by: Sarthak Aggarwal <[email protected]>

sarthakaggarwal97 force-pushed the nit-rename branch from cf8971d to 92f4645 Compare April 2, 2025 18:23

zuiderkwast approved these changes Apr 2, 2025

View reviewed changes

enjoy-binbin approved these changes Apr 3, 2025

View reviewed changes

tests/unit/other.tcl Show resolved Hide resolved

tests/unit/other.tcl Show resolved Hide resolved

adding more test

b49af21

Signed-off-by: Sarthak Aggarwal <[email protected]>

hpatro approved these changes Apr 3, 2025

View reviewed changes

zuiderkwast changed the title ~~Rename to Valkey for lolwut command~~ In LOLWUT's reply, change "Redis ver." to "Valkey ver." where version is printed Apr 3, 2025

zuiderkwast changed the title ~~In LOLWUT's reply, change "Redis ver." to "Valkey ver." where version is printed~~ In LOLWUT's reply, change "Redis ver." to "Valkey ver." Apr 3, 2025

zuiderkwast reviewed Apr 3, 2025

View reviewed changes

zuiderkwast merged commit 5cce160 into valkey-io:unstable Apr 3, 2025
49 checks passed

github-project-automation bot moved this from Todo to Done in Valkey 9.0 Apr 3, 2025

zuiderkwast added the release-notes This issue should get a line item in the release notes label Apr 3, 2025

In LOLWUT's reply, change "Redis ver." to "Valkey ver." #1559

In LOLWUT's reply, change "Redis ver." to "Valkey ver." #1559

Uh oh!

Conversation

sarthakaggarwal97 commented Jan 14, 2025 • edited by zuiderkwast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ranshid commented Jan 15, 2025

Uh oh!

ranshid commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarthakaggarwal97 commented Jan 27, 2025

Uh oh!

zuiderkwast commented Jan 27, 2025

Uh oh!

Uh oh!

ranshid commented Feb 3, 2025

Uh oh!

sarthakaggarwal97 commented Feb 6, 2025

Uh oh!

PingXie commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuiderkwast commented Mar 31, 2025

Uh oh!

sarthakaggarwal97 commented Apr 2, 2025

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

enjoy-binbin commented Apr 3, 2025

Uh oh!

sarthakaggarwal97 commented Apr 3, 2025

Uh oh!

zuiderkwast commented Apr 3, 2025

Uh oh!

sarthakaggarwal97 commented Apr 3, 2025

Uh oh!

zuiderkwast Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

sarthakaggarwal97 commented Jan 14, 2025 •

edited by zuiderkwast

Loading

codecov bot commented Jan 14, 2025 •

edited

Loading

ranshid commented Jan 27, 2025 •

edited

Loading

PingXie commented Feb 8, 2025 •

edited

Loading