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

Replace hiredis dependency with libvalkey #32

Merged
merged 2 commits into from
Aug 5, 2024
Merged

Replace hiredis dependency with libvalkey #32

merged 2 commits into from
Aug 5, 2024

Conversation

mkmkme
Copy link
Collaborator

@mkmkme mkmkme commented Jul 3, 2024

NOTE: This PR should not be merged until libvalkey-py is moved into valkey-io and the first version of libvalkey-py is released to PyPI!

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

This changeset changes hiredis dependency with libvalkey.

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 95.31250% with 6 lines in your changes missing coverage. Please review.

Project coverage is 75.02%. Comparing base (652669c) to head (51d3416).

Files Patch % Lines
tests/test_asyncio/test_search.py 33.33% 2 Missing ⚠️
valkey/_parsers/libvalkey.py 84.61% 2 Missing ⚠️
tests/test_search.py 50.00% 1 Missing ⚠️
tests/test_timeseries.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
+ Coverage   75.01%   75.02%   +0.01%     
==========================================
  Files         132      132              
  Lines       34367    34341      -26     
==========================================
- Hits        25782    25766      -16     
+ Misses       8585     8575      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mkmkme mkmkme marked this pull request as draft July 5, 2024 12:35
@mkmkme
Copy link
Collaborator Author

mkmkme commented Jul 5, 2024

converted to draft until PyPI package for libvalkey-py is published

@aiven-sal aiven-sal mentioned this pull request Jul 12, 2024
5 tasks
@ahmedsobeh ahmedsobeh mentioned this pull request Jul 20, 2024
aiven-sal added a commit that referenced this pull request Jul 22, 2024
hiredis 3.0.0 breaks compatibility with valkey-py.
This is just a temporary fix until #32 is ready.

Signed-off-by: Salvatore Mesoraca <[email protected]>
aiven-sal added a commit that referenced this pull request Jul 22, 2024
hiredis 3.0.0 breaks compatibility with valkey-py.
This is just a temporary fix until #32 is ready.

Signed-off-by: Salvatore Mesoraca <[email protected]>
aiven-sal added a commit that referenced this pull request Jul 22, 2024
hiredis 3.0.0 breaks compatibility with valkey-py.
This is just a temporary fix until #32 is ready.

Signed-off-by: Salvatore Mesoraca <[email protected]>
kjaymiller pushed a commit to kjaymiller/valkey-py that referenced this pull request Jul 31, 2024
hiredis 3.0.0 breaks compatibility with valkey-py.
This is just a temporary fix until valkey-io#32 is ready.

Signed-off-by: Salvatore Mesoraca <[email protected]>
@kjaymiller
Copy link
Contributor

@mkmkme - I see that both of the constraints you mentioned should be completed now (libvalkey and libvalkey-py are in the valkey org and libvalkey-py has been published to pypi)

@mkmkme
Copy link
Collaborator Author

mkmkme commented Aug 1, 2024

@kjaymiller I still haven't enabled the breaking changes yet, they are behind the flag in libvalkey.Reader. When I turn them on, the RESP3 tests start failing. Now I need to fix them, and then this is good to go :)

@mkmkme
Copy link
Collaborator Author

mkmkme commented Aug 2, 2024

@kjaymiller JFYI the release I've done was not good enough. I have to release a new one.

@mkmkme mkmkme marked this pull request as ready for review August 2, 2024 16:03
@mkmkme
Copy link
Collaborator Author

mkmkme commented Aug 2, 2024

The tests are failing. I know the reason and investigating it

mkmkme added 2 commits August 3, 2024 09:18
`pip install <pkg>` fails if the only available package version is a
beta. `libvalkey 3.0.0` is not suitable for the test and was yanked,
`libvalkey 4.0.0b1` is the most recent version. To use it, we need to
specify it explicitly. Provide it as a minimum version.

Signed-off-by: Mikhail Koviazin <[email protected]>
@mkmkme
Copy link
Collaborator Author

mkmkme commented Aug 3, 2024

@aiven-sal @ahmedsobeh all tests are green and invoke cluster-tests -c --protocol=3 --uvloop run locally passed as well.

Shall we remove the RESP3 libvalkey cluster exclusion as per https://github.com/valkey-io/valkey-py/pull/55/files or will we do this separately?

@ahmedsobeh
Copy link
Collaborator

@aiven-sal @ahmedsobeh all tests are green and invoke cluster-tests -c --protocol=3 --uvloop run locally passed as well.

Shall we remove the RESP3 libvalkey cluster exclusion as per https://github.com/valkey-io/valkey-py/pull/55/files or will we do this separately?

There is already a PR for it: #55

@aiven-sal aiven-sal enabled auto-merge August 5, 2024 08:16
Copy link
Member

@aiven-sal aiven-sal left a comment

Choose a reason for hiding this comment

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

Thank you!

@aiven-sal aiven-sal merged commit a87ce48 into valkey-io:main Aug 5, 2024
67 checks passed
@mkmkme mkmkme deleted the mkmkme/libvalkey branch August 5, 2024 08:17
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.

5 participants