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

add minId overloads #2842

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kijanawoodard
Copy link

@kijanawoodard kijanawoodard commented Jan 21, 2025

Fixes #1718

Initial commit. Still orienting on how to contribute.
It's building. Tests pass.

Before I continue, does the diff look sane?

I think I got the "public api" situated correctly.

The strangest thing I ran into was this error about default parameters. I'm not sure if this is pushing me towards adding a new method rather than an overload, e.g. StreamTrimWithMinId. I took out the default CommandFlags to build successfully.

error RS0026: Symbol 'StreamTrim' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'

I'm having trouble running the tests locally. I tried running the tests before making any changes and ~2/3 passed. It might be because I don't have all the versions of .net installed.

clarify doc comment

i now think the XTRIM documentation is saying that an entry at exactly MINID is kept.
https://redis.io/docs/latest/commands/xtrim/
forgot to update the length check.
@kijanawoodard
Copy link
Author

I can't tell what's wrong in the appveyor build. It says zero tests failed, but still errors.

@kijanawoodard
Copy link
Author

kijanawoodard commented Jan 22, 2025

I was able to get devcontainer working on WSL enough to build, but the tests can't connect to redis in the devcontainer.

I'll post my changes in case anyone else can follow the breadcrumbs.

I needed to enable Ubuntu integration in docker desktop
image

I added dotnet 8 installation to the Dockerfile under .devcontainer
RUN curl -sSL https://dot.net/v1/dotnet-install.sh | bash /dev/stdin --channel 8.0 --install-dir /usr/share/dotnet/

@kijanawoodard
Copy link
Author

I wasn't able to get dev containers running. The dockerfile in .devcontainer refers to a Dockerfile in tests/RedisConfigs that doesn't exist. I tried using the other ones, but they appear to depend on the docker-compose.yml file in the tests/RedisConfigs folder.

I was able to run the tests with only one failure: FailoverTests.SubscriptionsSurviveConnectionFailureAsync(RESP2) [FAIL]

I got that far by cloning the project in wsl2, running docker compose up in tests/RedisConfigs and then running dotnet test StackExchange.Redis.sln from the project root.

@kijanawoodard
Copy link
Author

kijanawoodard commented Jan 22, 2025

I was wrong about appveyor having no test failures. There were 3 failures that don't seem to be related to my changes here. All 3 timed out running more than 5000ms. If there are known test issues, let me know.

@kijanawoodard
Copy link
Author

I tried pushing an empty commit to rebuild appveyor. The error moved.

ConnectingFailDetectionTests.ConnectIncludesSubscriber(RESP2) [FAIL]

Now there's just that one.

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.

Add support for stream XTRIM MINID command
1 participant