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

Fix deprectated pipeline calls for Redis 4.6.0 #32

Merged
merged 1 commit into from
Mar 2, 2022
Merged

Fix deprectated pipeline calls for Redis 4.6.0 #32

merged 1 commit into from
Mar 2, 2022

Conversation

justinhoward
Copy link
Contributor

Redis 4.6.0 deprecates accessing redis within pipeline and multi calls. Fix our calls to these methods to use the PipelinedConnection given in the block argument.

This is to pair with the rpush PR rpush/rpush#636

Since the previous versions of redis passed the Client object to the block argument, our new calling syntax is compatible with both pre-4.6.0, and post-4.6.0.

I made some decisions here that probably need some feedback from maintainers:

  • Upgraded redis in the Gemfile to 4.6.0. We currently only test this single version. Should we use different versions in the CI matrix? I did write the specs to support this, mocking the correct behavior for before/after 4.6.0.
  • Turned on Redis 4.6.0 deprecation errors in specs

Redis 4.6.0 deprecates accessing `redis` within pipeline and multi
calls. Fix our calls to these methods to use the PipelinedConnection
given in the block argument.
@aried3r
Copy link
Member

aried3r commented Mar 2, 2022

Hey! Thanks for this PR!

  • Should we use different versions in the CI matrix?

Personally, I don't think so. Older versions of modis will work with older versions of redis and after this PR is merged and released, we'll support redis 4.6 without deprecation warnings.

I don't use the redis gem myself, but do they use semver? As in, will breaking changes only occur in major versions? If so, I'd say we can think about a testing matrix once 5.x is released to support 4.x and 5.x if necessary. WDYT?

  • Turned on Redis 4.6.0 deprecation errors in specs

I like it! Should make maintaining a little bit easier. But we should probably then also schedule a CI run every month.

@aried3r
Copy link
Member

aried3r commented Mar 2, 2022

But we should probably then also schedule a CI run every month.

I just saw this repo still uses Travis. I guess we should switch to GitHub actions. However I don't use modis myself and currently don't have enough time for this. I'll open an issue.

@justinhoward
Copy link
Contributor Author

Thanks for the review and merge @aried3r!

I don't use the redis gem myself, but do they use semver? As in, will breaking changes only occur in major versions? If so, I'd say we can think about a testing matrix once 5.x is released to support 4.x and 5.x if necessary. WDYT?

Yes, redis seems to follow semver, and your suggestion makes sense to me 👍

@benlangfeld
Copy link
Contributor

Switching to GH Actions in #34.

benlangfeld added a commit to powerhome/modis that referenced this pull request Jan 18, 2023
It's not clear why this complexity was added in rpush#32 but it was unnecessary and breaks the test suite with redis 4.7.1. Removing it continues with tests passing on 4.6.0 and fixes the tests with 4.7.1.

Compatibility with redis 4.8.x will come in rpush#35.

See rpush#36.
@justinhoward justinhoward deleted the fix-redis-pipeline-deprecations branch January 18, 2023 18:38
benlangfeld added a commit to powerhome/modis that referenced this pull request Feb 1, 2023
It's not clear why this complexity was added in rpush#32 but it was unnecessary and breaks the test suite with redis 4.7.1. Removing it continues with tests passing on 4.6.0 and fixes the tests with 4.7.1.

Compatibility with redis 4.8.x will come in rpush#35.

See rpush#36.
benlangfeld added a commit that referenced this pull request Feb 1, 2023
It's not clear why this complexity was added in
#32 but it was unnecessary and breaks
the test suite with redis 4.7.1. Removing it continues with tests
passing on 4.6.0 and fixes the tests with 4.7.1.

Compatibility with redis 4.8.x will come in
#35.

See #36.
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

3 participants