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 projection reset to client #79

Merged
merged 2 commits into from
Oct 20, 2020

Conversation

BrunoZell
Copy link
Contributor

This adds a remote call ResetAsync to EventStoreProjectionManagementClient. Without it the client is not able to reset a projection.

The RPC call was already specified in the protobuf definition.

I am not sure if it is correct to always write a checkpoint.

Copy link
Contributor

@thefringeninja thefringeninja 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 @BrunoZell for your contribution! You are correct, it was indeed not implemented but should be supported. Would you feel comfortable adding some tests for this?

@BrunoZell
Copy link
Contributor Author

BrunoZell commented Oct 15, 2020

I created a test case. Is that one enough or are there some other considerations when resetting a projection? @thefringeninja

Also, I couldn't run the tests locally on my Windows dev machine. The EventStoreDB docker container created for each test runs into an System.UnauthorizedAccessException: Access to the path '/etc/eventstore/certs/node/node.key' is denied. exception. The file does exist locally and I used chmod 0755 -R ./certs on that folder in a WSL2 Linux distro. Still not accessible. Any Windows developers in your team?

Overall, the test suite looks quite solid 👍

Also, what's wrong with the one regression test?

@hayley-jean
Copy link
Member

Thanks @BrunoZell! For Docker on Windows you may need to add the ./certs directory to Docker's file sharing resources if you haven't already

The failing regression test looks unrelated, we'll rerun the tests and see if it comes right

@BrunoZell
Copy link
Contributor Author

@hayley-jean Can we try and get this merged soon, please?

@hayley-jean hayley-jean merged commit 2730403 into EventStore:master Oct 20, 2020
@hayley-jean
Copy link
Member

The tests run fine locally, so we're happy to merge @BrunoZell
Thanks for the PR!

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.

3 participants