valkey icon indicating copy to clipboard operation
valkey copied to clipboard

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

Open gaod opened this issue 1 year ago • 15 comments

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

gaod avatar Mar 29 '24 18:03 gaod

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.

mattsta avatar Mar 29 '24 22:03 mattsta

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 avatar Mar 30 '24 16:03 zuiderkwast

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)

zuiderkwast avatar Mar 30 '24 17:03 zuiderkwast

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

gaod avatar Mar 30 '24 17:03 gaod

@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.

zuiderkwast avatar Mar 30 '24 19:03 zuiderkwast

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

hwware avatar Apr 01 '24 13:04 hwware

@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

hwware avatar Apr 01 '24 16:04 hwware

@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 avatar Apr 02 '24 15:04 gaod

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.

zuiderkwast avatar Apr 02 '24 17:04 zuiderkwast

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?

gaod avatar Apr 02 '24 18:04 gaod

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 avatar Apr 02 '24 18:04 hwware

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

hwware avatar Apr 02 '24 18:04 hwware

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.

zuiderkwast avatar Apr 02 '24 18:04 zuiderkwast

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

gaod avatar Apr 02 '24 18:04 gaod

@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 avatar Apr 02 '24 19:04 hwware

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

hwware avatar May 27 '24 20:05 hwware