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

Jump to bottom

Change Z member type to string #2818

Merged
merged 3 commits into from Dec 17, 2023
Merged

Change Z member type to string #2818

merged 3 commits into from Dec 17, 2023

Conversation

ofekshenawa
Copy link
Collaborator

Closes #2817

@ofekshenawa ofekshenawa requested review from chayim and a team November 30, 2023 14:17
@ofekshenawa ofekshenawa merged commit e535124 into master Dec 17, 2023
9 checks passed
@jarno-rootz
Copy link

You don't do semantic versioning in go-redis? This is a breaking change.

@AlexanderYastrebov
Copy link
Contributor

This change breaks our code zalando/skipper#2806

# github.com/zalando/skipper/net
net/redisclient.go:404:47: cannot use val (variable of type int64) as string value in struct literal

as we used int64

@AlexanderYastrebov AlexanderYastrebov mentioned this pull request Jan 2, 2024
Closed
AlexanderYastrebov added a commit to zalando/skipper that referenced this pull request Jan 2, 2024
@AlexanderYastrebov
net: fix redis ZAdd
416bfe7 
There was a breaking change to go-redis library redis/go-redis#2818

I've checked how it handles interface{} command arguments and it
converts int64 to string so this change should fix the problem.

Closes #2806

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov AlexanderYastrebov mentioned this pull request Jan 2, 2024
Closed
@knadh
Copy link
Contributor

knadh commented Jan 2, 2024

I would expect a breaking change only happens at major version updates.

This change seems to be a mistake. Going by the spec, all values in Redis are "strings", including Set(), which currently takes an interface{}. Changing the type in ZMember to string doesn't make any sense.

Also, this is a semver breaking change pushed in a minor version that just broke our codebase.

@ofekshenawa ofekshenawa mentioned this pull request Jan 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvora-h dvora-h dvora-h approved these changes

@chayim chayim Awaiting requested review from chayim

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

zset member returned as an interface type instead of a string and is not type-safe
5 participants
@ofekshenawa @jarno-rootz @AlexanderYastrebov @knadh @dvora-h