-
-
Notifications
You must be signed in to change notification settings - Fork 309
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
feat: Add Firestore module #988
feat: Add Firestore module #988
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thank you for your pull request. I have some requests. As soon as they are addressed, we can merge the module. Thanks.
tests/Testcontainers.Firestore.Tests/Testcontainers.Firestore.Tests.csproj
Outdated
Show resolved
Hide resolved
tests/Testcontainers.Firestore.Tests/FirestoreContainerTests.cs
Outdated
Show resolved
Hide resolved
i pushed new changes to resolve comments made to PR, i didn't resolve comments themselves unsure what are practices here. if required i can do that. |
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.
Thank you for updating the pull request and addressing the requests. I made some minor adjustments, nothing significant. The pull request looks good 👍.
One thing I'd like to discuss is whether it makes sense to rename the module to "GCloud," similar to Java. This change would allow us to provide different container implementations, much like what Java does here. Additionally, should we renaming the container to FirestoreEmulatorContainer
? This PR is related to #862 too.
I can do a push with renaming tommorow, gcloud naming is similar to java containers but dotnet containers have different setup. E. G. Azure is not single module. What ever you decide. I plan to push pubsub in aonther pr also |
Hmm, I think it depends on how often these containers may be used together and whether they share common configurations. @eddumelendez , based on your experience with Java, do you think it makes sense to publish them as separate modules or within a single one (the GCloud emulators)? Any opinion regarding the name ( |
basically they don't share configuration at all, for usage point it depends on what are you testing if containers are used in unit test then responsibility of certain class is unlikely (should not) involve multiple concerns, repository, service ... should work with firestore or pubsub or bigquery ... IMHO i would stick to general testcontainers dotnet design which is one container per service, but this is up to you on naming i would go with FirestoreContainer , we all know it is an emulator :-) |
Testcontainers modules based on The relationship between modules and implementation shouldn't be always 1:1. In the future, a Feedback about this topic is welcome.
|
What does this PR do?
Why is it important?
Related issues