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

Rename to Valkey for lolwut command #1559

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

Rename to Valkey for lolwut command #1559

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lolwut.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ void lolwut6Command(client *c);
/* The default target for LOLWUT if no matching version was found.
* This is what unstable versions of the server will display. */
void lolwutUnstableCommand(client *c) {
sds rendered = sdsnew("Redis ver. ");
sds rendered = sdsnew("Valkey ver. ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sds rendered = sdsnew("Valkey ver. ");
sds rendered = sdsnew(SERVER_TITLE " ver. ");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we left this intentionally because it's technically breaking change. Why do we need to change it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess because seeing "Redis" anywhere sticks out. :D If we want to keep LOLWUT without updates, then we should probably instead return REDIS_VERSION rather than VALKEY_VERSION?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess because seeing "Redis" anywhere sticks out.

Yeah, that's why I raised the change. I wasn't aware that we left this out intentionally.
Do you think that we should take this up in the next major version?

Also, we kinda still use it here as well: https://github.com/valkey-io/valkey/blob/unstable/src/networking.c#L4277

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think we missed those security warning logs. We decided during the initial rebranding work that we don't need compatibility for log messages, so I guess it's OK to change them.

I did a quick grep and I found one more occurrence in an error message in script.c: "This Redis command is not allowed from script". My first thought is that I don't think it's risky to change it, but if we think it is, then we could let it depend on the extended-redis-compat configuration.

Copy link
Contributor Author

@sarthakaggarwal97 sarthakaggarwal97 Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided during the initial rebranding work that we don't need compatibility for log messages, so I guess it's OK to change them.

I guess I can raise another PR for this, if that's okay. If we decide on changing in the script.c as well, I am happy to do it as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madolson @ranshid WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the lolwut command I would suggest to keep the Redis and fixed version (7.2.4). I would also suggest that we deprecate the lolwut and setup a path forward to remove support for it. (BTW we should do the same for the version 5/6 lolwut IMO)

For the security logs, I have no real concern as this is reported in logs. I do wonder if this cross-application-scripting is really a security concern? maybe we should just remove this whole thing?

rendered = sdscat(rendered, VALKEY_VERSION);
rendered = sdscatlen(rendered, "\n", 1);
addReplyVerbatim(c, rendered, sdslen(rendered), "txt");
Expand Down
Loading