Skip to content

Adds Valkey Support#1

Open
MatthiasHowellYopp wants to merge 1 commit into
mainfrom
valkey-support
Open

Adds Valkey Support#1
MatthiasHowellYopp wants to merge 1 commit into
mainfrom
valkey-support

Conversation

@MatthiasHowellYopp
Copy link
Copy Markdown

Adds the changes to support either valkey or redis depending on environment variables.

Copy link
Copy Markdown

@tokiwadai tokiwadai left a comment

Choose a reason for hiding this comment

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

Put some small comments if you won't mind. Thanks!

Comment thread README.md
Comment thread README.md
Comment thread dist/index.js
Copy link
Copy Markdown

@jbrinkman jbrinkman left a comment

Choose a reason for hiding this comment

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

It looks like we are missing tests for Valkey. We should review the existing tests and make sure that we have similar coverage for Valkey.

Comment thread QUICKSTART.md
Comment thread README.md
Comment thread src/index.ts
Comment thread src/index.ts
@MatthiasHowellYopp
Copy link
Copy Markdown
Author

@jbrinkman I've updated tests so that the work for both valkey and redis - there are two scripts that don't work in main and I am waiting for a response from the recall owner.

Comment thread dist/index.js
Comment thread src/persistence/memory-store.ts Outdated
Comment thread src/persistence/redis-adapter.ts Outdated
Comment thread src/tools/export-import-tools.ts Outdated
Comment thread src/index.ts
Comment thread tests/README.md
Comment thread README.md
Comment thread README.md
Comment thread tests/test-v1.5.0-simple.sh Outdated
upped reasonable size
Copy link
Copy Markdown

@jbrinkman jbrinkman left a comment

Choose a reason for hiding this comment

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

LGTM once all comments are resolved. :shipit:

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