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

feat(redis): update to v9 and add context #130

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

glimchb
Copy link
Contributor

@glimchb glimchb commented Oct 18, 2023

following 9c5b7d7
Resolves #132

Signed-off-by: Boris Glimcher Boris.Glimcher@emc.com

@glimchb
Copy link
Contributor Author

glimchb commented Oct 18, 2023

@philippgille please review

Copy link
Owner

@philippgille philippgille left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

So far I was thinking to just update to latest minor+patch versions for the next gokv release, and then bump major versions in the following one. This would keep breaking changes low and people could update from the really old v0.6.0 to a fresh v0.7.0 without worrying too much but getting important security updates.

But OTOH maybe we don't need to follow the same strategy for all packages, for example if there are no breaking changes. Have you checked if in the Redis library v6 to v9 there's anything that could affect gokv users? We're only using a very limited subset of the Redis functionalities, so I could imagine we're not affected, but better to double check.

WDYT?

redis/go.mod Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
@glimchb
Copy link
Contributor Author

glimchb commented Oct 23, 2023

Thanks for the PR!

So far I was thinking to just update to latest minor+patch versions for the next gokv release, and then bump major versions in the following one. This would keep breaking changes low and people could update from the really old v0.6.0 to a fresh v0.7.0 without worrying too much but getting important security updates.

But OTOH maybe we don't need to follow the same strategy for all packages, for example if there are no breaking changes. Have you checked if in the Redis library v6 to v9 there's anything that could affect gokv users? We're only using a very limited subset of the Redis functionalities, so I could imagine we're not affected, but better to double check.

WDYT?

Excellent comments!

The main reason to update to v9 was due to otel in #131

Sorry, I haven't checked the Redis library v6 to v9 for breaking changes

CI fails on

Error: ../../../../go/pkg/mod/github.com/redis/go-redis/v9@v9.2.1/commands.go:111:19: undefined: strings.Cut
Error: ../../../../go/pkg/mod/github.com/redis/go-redis/v9@v9.2.1/commands.go:134:18: undefined: strings.Cut
Error: ../../../../go/pkg/mod/github.com/redis/go-redis/v9@v9.2.1/commands.go:154:26: undefined: reflect.Pointer
note: module requires Go 1.18

I understand concern of updating this and deprecating 1.17 is problematic ...

@glimchb glimchb force-pushed the redis branch 2 times, most recently from f25ac10 to 8cceb94 Compare October 23, 2023 18:16
@philippgille
Copy link
Owner

Sorry, I haven't checked the Redis library v6 to v9 for breaking changes

I just checked https://github.com/redis/go-redis/blob/v8.11.5/CHANGELOG.md (which covers v7 and v8 changes) and https://github.com/redis/go-redis/blob/v9.2.1/CHANGELOG.md (which covers only v9 changes) and I don't think that this PR will lead to breaking changes, thanks to using only basic commands, no pipelining etc.

Regarding the server version used in tests, we already use always the latest here, so that's fine.

@philippgille
Copy link
Owner

For the error in CI, I'm removing Go 1.17 support now.

Can you update the branch with the latest master changes? If CI is 🟢 (at least for the Redis store) then it's good to go 🚀

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Merging #130 (63a57e4) into master (bfdf6b8) will increase coverage by 0.26%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   65.00%   65.26%   +0.26%     
==========================================
  Files          25       25              
  Lines        2080     2096      +16     
==========================================
+ Hits         1352     1368      +16     
  Misses        601      601              
  Partials      127      127              
Files Coverage Δ
redis/redis.go 81.94% <100.00%> (+5.15%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@glimchb
Copy link
Contributor Author

glimchb commented Oct 31, 2023

Can you update the branch with the latest master changes? If CI is 🟢 (at least for the Redis store) then it's good to go 🚀

CI passed @philippgille !

Copy link
Owner

@philippgille philippgille left a comment

Choose a reason for hiding this comment

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

Thanks again 👍

@philippgille philippgille merged commit 084bda1 into philippgille:master Nov 4, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redis: update to v9
3 participants