Skip to content

[WIP] Rename all files from redis* to valkey*#88

Closed
gaod wants to merge 29 commits intovalkey-io:unstablefrom
gaod:rename-from-redis-to-valkey
Closed

[WIP] Rename all files from redis* to valkey*#88
gaod wants to merge 29 commits intovalkey-io:unstablefrom
gaod:rename-from-redis-to-valkey

Conversation

@gaod
Copy link

@gaod gaod commented Mar 29, 2024

Rename all files from redis* to valkey* and fixes related code changes causing compilation issues.

@mattsta
Copy link
Contributor

mattsta commented Mar 29, 2024

Looks like a clean replace.

There are still references to product names, but not sure if that is getting cleaned up elsewhere, example:

is the official C client library for Redis. It is used by valkey-cli, valkey-benchmark and Redis Sentinel. It is part of the Redis official ecosystem but is developed externally from the Redis repository, so we just upgrade it as needed.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for the PR.

I see you have done these changes in separate commits. That's good because I have to ask you to skip some of them. :)

Everything else LGTM.

@zuiderkwast
Copy link
Contributor

Regarding redis-trip, it's deprecated since long ago. It's better that we don't touch it, so please skip this commit too. We plan to keep compatibility with the redis binaries with symlinks, so the script can still work. In the next major release we can delete redis-trib.

@zuiderkwast
Copy link
Contributor

CI failure

!!! WARNING The following tests failed:

*** [err]: FUNCTION - redis version api in tests/unit/functions.tcl
Expected '255.255.255' to be equal to '' (context: type eval line 15 cmd {assert_equal  [r fcall get_version_v1 0] [r fcall get_version_v2 0]} proc ::test)

@gaod
Copy link
Author

gaod commented Mar 30, 2024

Thanks, I'll take a look about CI failed & the rest of suggestions.

@zuiderkwast
Copy link
Contributor

@gaod OK, good.

It's probably easier to revert the commits "Move redismodule.h to valkeymodule.h" and "Move redis-trib to valkey-trib" instead of using my suggestions.

@hwware
Copy link
Contributor

hwware commented Apr 1, 2024

Becuase there are thousand of lines in this pr, could you please run a dialy CI and post the result here. Thanks

@hwware
Copy link
Contributor

hwware commented Apr 1, 2024

@gaod I found many redis workds do not change to redis, include redis-server, redis cluster etc, could you please check all your changed files again and update them? Thanks

@gaod
Copy link
Author

gaod commented Apr 2, 2024

@gaod I found many redis workds do not change to redis, include redis-server, redis cluster etc, could you please check all your changed files again and update them? Thanks

Sure, I'll update the PR of the wordings you mentioned or related.

gaod and others added 15 commits April 2, 2024 15:49
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
…y-benchmark.tcl

Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
roshkhatri and others added 4 commits April 2, 2024 15:49
… support to MAKEFILE (valkey-io#66)

This resolves (1.viii) from
valkey-io#43
> REDIS_FLAGS will be updated to SERVER_FLAGS. Maybe we should also
allow REDIS_FLAGS -> SERVER_FLAGS as well, for an extra layer of
compatibility.

---------

Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Vitah Lin <linw1225@gmail.com>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
gaod and others added 6 commits April 2, 2024 15:59
This reverts commit b61e4bd.

Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
This reverts commit fcb53e2.

Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
…rror

Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
@gaod gaod changed the title Rename all files from redis* to valkey* [WIP] Rename all files from redis* to valkey* Apr 2, 2024
gaod added 2 commits April 2, 2024 16:39
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
Signed-off-by: Hung-Yi Chen <gaod@hychen.org>
@zuiderkwast
Copy link
Contributor

I'm sorry but this is starting to be very difficult to review.

The change of REDIS_ variables is not complete. There are still variables like that, such as REDIS_TLS_PROTO_TLSv1_2. Don't need to do it, but write exactly what is done in the PR description.

With this kind of mass replacements, it is better to explain exactly what is done, so we can review how it was done instead of reading the full diff.

Also, I'm starting to think that one PR for each renamed binary would be good. At lease one commit for each binary and no revert and applied suggestion commits. Force-push is OK.

@gaod
Copy link
Author

gaod commented Apr 2, 2024

I believe the original purpose was to rename the binary, but the discussion somewhat diverged from that. I agree that it may be better to split this into different pull requests. Could the core team decide whether to handle them together in the same pull request or to split them into multiple pull requests?

@hwware
Copy link
Contributor

hwware commented Apr 2, 2024

I believe the original purpose was to rename the binary, but the discussion somewhat diverged from that. I agree that it may be better to split this into different pull requests. Could the core team decide whether to handle them together in the same pull request or to split them into multiple pull requests?

One big pr is not problem. Atfer review it, please run CI on your own repo and post result. Thanks

@hwware
Copy link
Contributor

hwware commented Apr 2, 2024

I believe the original purpose was to rename the binary, but the discussion somewhat diverged from that. I agree that it may be better to split this into different pull requests. Could the core team decide whether to handle them together in the same pull request or to split them into multiple pull requests?

Could you please first rebase unstable branch and add DCO in your commit? Thanks

@zuiderkwast
Copy link
Contributor

OK, I can accept one big PR. I prefer to skip those "Update src/module.c" and Revert commits though, so it will be easier to see only one commit for each renamed binary. You need to fix those commits without a signed-off-by anyway.

@gaod
Copy link
Author

gaod commented Apr 2, 2024

I believe the original purpose was to rename the binary, but the discussion somewhat diverged from that. I agree that it may be better to split this into different pull requests. Could the core team decide whether to handle them together in the same pull request or to split them into multiple pull requests?

One big pr is not problem. Atfer review it, please run CI on your own repo and post result. Thanks

The latest CI report is in https://github.com/gaod/valkey/actions/runs/8527284341

--loadmodule src/redis-tls.so

To connect to this Redis server with `redis-cli`:
To connect to this Redis server with `valkey-cli`:
Copy link
Contributor

@Shivshankar-Reddy Shivshankar-Reddy Apr 2, 2024

Choose a reason for hiding this comment

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

is this "To connect to this valkey server with valkey-cli"?

@hwware
Copy link
Contributor

hwware commented Apr 2, 2024

@gaod Hi Gaod, Thanks for your hard work first, But this pr involved too much changes and files. Could you please split it into mulltiply small pr so that we could be easier to review and merge them? Thanks.

Note: Make sure add DCO in your commit,

@hwware
Copy link
Contributor

hwware commented May 27, 2024

Most rebranding work was done, we would like to close this PR soon.

@gaod gaod deleted the rename-from-redis-to-valkey branch June 12, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rebranding Valkey is not Redis to-be-closed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants