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

filter-network-redis: add support for single-key STREAM commands #33436

Closed
wants to merge 1 commit into from

Conversation

nicknotfun
Copy link
Contributor

@nicknotfun nicknotfun commented Apr 10, 2024

Commit Message:
Add support for all the stream commands which are hashable on their first argument (the key)

Additional Description:
As above, it does lack support for the remaining stream commands which someone else can look at adding.
Is a partial fix for #32928

Risk Level: Low
Testing: Manual (unsure what is needed)

Docs Changes:
Probably should add to the supported command list, but want to ensure if the process is needed.

Release Notes:
Add support for xack, xadd, xautoclaim, xclaim, xdel, xlen, xrange, xrevrange, xtrim to the redis proxy.

Platform Specific Features: N/A

Signed-off-by: Nick Cooper <nick@not.fun>
Copy link

Hi @nicknotfun, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #33436 was opened by nicknotfun.

see: more, trace.

@RyanTheOptimist
Copy link
Contributor

/assign-from @envoyproxy/first-pass-reviewers

Copy link

@envoyproxy/first-pass-reviewers assignee is @nezdolik

🐱

Caused by: a #33436 (comment) was created by @RyanTheOptimist.

see: more, trace.

@RyanTheOptimist
Copy link
Contributor

Looks like the formatting of this PR needs to be fixed:
https://dev.azure.com/cncf/envoy/_build/results?buildId=167513&view=logs&jobId=c7de00aa-b0fb-52ba-707b-da6afd918eda&j=c7de00aa-b0fb-52ba-707b-da6afd918eda&t=449299ab-9da3-5c6b-afe3-e0c5dbf1dfd1
/wait

Copy link
Member

@nezdolik nezdolik left a comment

Choose a reason for hiding this comment

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

Looks like the testing of supported commands is covered by existing tests (by using values in simpleCommands() set as test input parameters). The format needs to be fixed, so that CI passes and changelog needs to be updated. Besides that, please update the supported commands table in the user docs: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/other_protocols/redis

Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 16, 2024
Copy link

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this May 23, 2024
mattklein123 pushed a commit that referenced this pull request Jul 9, 2024
)

Commit Message:
Add support for all the stream commands which are hashable on their
first argument (the key)

Additional Description:
This is a repeat of #33436 which
I sadly let get stale before following up on responses.

Risk Level: Low

Testing:
Manual, also covered by existing tests

Docs Changes:
Added additional commands in the docs.

Release Notes:
Added to release notes.

Platform Specific Features: N/A

Signed-off-by: Nick Cooper <nick@not.fun>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale stalebot believes this issue/PR has not been touched recently waiting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants