-
Notifications
You must be signed in to change notification settings - Fork 71
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
Conversation
@philippgille please review |
There was a problem hiding this 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?
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
I understand concern of updating this and deprecating 1.17 is problematic ... |
f25ac10
to
8cceb94
Compare
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. |
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 Report
❗ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
CI passed @philippgille ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again 👍
following 9c5b7d7
Resolves #132
Signed-off-by: Boris Glimcher Boris.Glimcher@emc.com