-
-
Notifications
You must be signed in to change notification settings - Fork 340
feat: Add OpenSearch module #1395
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
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@HofmeisterAn Hello, please take a look. |
|
I think this is ready for review |
|
Thanks for updating. I'll review it in the next days. |
HofmeisterAn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I finally had a time to review it. Overall, it looks good. I made a few adjustments for alignment and changed how the wait strategy is configured.
If we configure it using WithDisabledSecurity(bool), the order of method calls starts to matter, which we generally try to avoid. For example, if a user calls WithPassword(string) afterward, the readiness check will fail.
In cases where builder APIs have dependencies like this, we either use a dedicated method to handle it or apply the final configuration in Build().
I still need to review the tests tomorrow. If you're okay with the changes I made, we can go ahead and merge it then.
Hello. Thanks for review. Overall all changes look good, I appreciate you taking time to do it. |
HofmeisterAn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution 🙇.
What does this PR do?
Adds OpenSearch testcontainers implementation.
Why is it important?
Currently there is no OpenSearch testcontainers implementation.
Related issues
Hopefully closes #1137
How to test this PR
tests/Testcontainers.OpenSearch.Tests