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

Avoid redis 5.x deprecation warning when closing connection #376

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

sevdog
Copy link
Contributor

@sevdog sevdog commented Jan 5, 2024

Redis 5.0.1 adds a new method aclose and deprecates the close method in the async client.

This change aims to resolve the deprecation warnings which arise when using the new redis-py until redis 4.5 is still supported.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hey @sevdog. Thanks for this. Looks sensible.

Q: Do we need to add testing for the different versions of redis-py? (Perhaps just for the main tests?)

@sevdog
Copy link
Contributor Author

sevdog commented Jan 8, 2024

Q: Do we need to add testing for the different versions of redis-py? (Perhaps just for the main tests?)

I was not sure about this point, now that you are pointing it I think it would be fine to have in the CI/tox configuration also the matrix of supported redis-py versions. In this way we may ensure compatibility and also mark such tests. What do you think?

@carltongibson
Copy link
Member

I think that might be helpful... — we could then easily test against (e.g.) current and upcoming versions, and perhaps be clearer/stricter about what versions we support.

What's your take?

Fancy adding it? 🎁

@sevdog
Copy link
Contributor Author

sevdog commented Jan 8, 2024

Taht could be nice!

I think that a redis compatibility-matrix may be set-up in the scope of this PR.

I would also like to add the same testing mechanism also for channels (since currently no version is pinned in setup, there is a generic statement in the changelog but nothing else).

Maybe I could work on a PR which adds channels and redis to test matrix before merging this one.

@carltongibson
Copy link
Member

Maybe I could work on a PR which adds channels and redis to test matrix before merging this one.

That would be perfect 🤩

Not sure we need Channels... but if you think so — Did it ever fail? 🤔

I don't think we need to test every combination of channels against every redis 🤔, and I'd just run the core tests, rather than all the sentinel and cluster ones. (It seems unlikely there's an extra regression there so saving a few cycles is probably better 🌳)

@sevdog
Copy link
Contributor Author

sevdog commented Jan 8, 2024

Let me try to propose a test matrix:

redis develop redis stable redis 4.5
channels develop ✖️ ✔️ ✖️
channels stable ✔️ ✔️ ✔️
channels 3.0 ✖️ ✔️ ✖️

Writing it down: redis compatibility is tested only against the latest release of channels and channels compatibility is tested only angainst the latest release of redis.

@carltongibson
Copy link
Member

That looks great

@sevdog sevdog mentioned this pull request Jan 9, 2024
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Great. Let's have it. Thanks @sevdog

@carltongibson carltongibson merged commit dad0b13 into django:main Jan 12, 2024
6 checks passed
@sevdog sevdog deleted the redis-5 branch January 12, 2024 09:39
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.

None yet

2 participants