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.

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

Change redis to server in INFO fields redis_{git_sha1,git_dirty,build_id} #1736

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

zuiderkwast
Copy link
Contributor

These fields were mentioned in #41 but we didn't change them. I don't remember why.

I hope we can changing them in 9.0 when we're phasing out the redis compatibility fields.

… and redis_build_id

Signed-off-by: Viktor Söderqvist <[email protected]>
@zuiderkwast zuiderkwast added the rebranding Valkey is not Redis label Feb 14, 2025
Signed-off-by: Viktor Söderqvist <[email protected]>
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.10%. Comparing base (c5ae37f) to head (07d3ea9).

Files with missing lines Patch % Lines
src/release.c 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1736      +/-   ##
============================================
- Coverage     71.17%   71.10%   -0.08%     
============================================
  Files           123      123              
  Lines         65536    65536              
============================================
- Hits          46645    46599      -46     
- Misses        18891    18937      +46     
Files with missing lines Coverage Δ
src/rdma.c 100.00% <ø> (ø)
src/server.c 87.54% <ø> (-0.03%) ⬇️
src/tls.c 100.00% <ø> (ø)
src/release.c 81.25% <75.00%> (ø)

... and 11 files with indirect coverage changes

"redis_git_sha1:%s\r\n", serverGitSHA1(),
"redis_git_dirty:%i\r\n", strtol(serverGitDirty(), NULL, 10) > 0,
"redis_build_id:%s\r\n", serverBuildIdString(),
"server_git_sha1:%s\r\n", serverGitSHA1(),
Copy link
Member

Choose a reason for hiding this comment

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

Some part of me says let's just leave them. One more thing to not list as a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

I remember we should keep this info message as is, otherwise is a break change. For the other update in this PR, everything is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a breaking change.

So I think we will not change it in 9.0 then. But I'm wondering: How long are we going to keep everything 'redis'? Forever? Five years? If we keep it forever, then users will always remember that Redis was the original and Valkey is just a fork.

If we want to accept the risk, we are allowed to make breaking changes in a major version. I understand "redis_version" is used by many clients, but the git fields are likely not used by many. If we decide to change it some time, I hope users will understand that we want to phase out 'redis' named fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rebranding Valkey is not Redis
Projects
Status: Implementation
Development

Successfully merging this pull request may close these issues.

3 participants