Skip to content

Conversation

Zarux
Copy link
Contributor

@Zarux Zarux commented Mar 7, 2025

What does this PR do?

This PR adds an option to the gcloud module applicable only to RunFirestore that runs the emulator in Firestore in Datastore mode.

It also changes the base example to point to the non-depracated function in order to create a new example with datastore mode

Why is it important?

Datastore in GCP is now Firestore in Datastore mode, which means the datastore emulator behaves slightly different to what one would expect as it emulates the legacy datastore product.

In the gcloud cloud-sdk version 465.0.0 a parameter was added to run the firestore emulator in datastore mode which should behave more like the product in GCP.

How to test this PR

The simplest way to test this would be to run the firestore emulator with the gcloud.WithDatastoreMode() and connect using the datastore package.
There is an example test case added in this PR as well that does this.

@Zarux Zarux requested a review from a team as a code owner March 7, 2025 19:43
Copy link

netlify bot commented Mar 7, 2025

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 6c21628
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/680b621a36358000087cc5a5
😎 Deploy Preview https://deploy-preview-3009--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdelapenya
Copy link
Member

Thanks for submitting this improving. I took a quick look and initially it LGTM.

I want to share that after #3008, I'm considering following the same pattern of one submodule per package, deprecating all the existing GCP containers. As contributor of this GCP module, does it reasonable to you?

@Zarux
Copy link
Contributor Author

Zarux commented Mar 12, 2025

Thanks for submitting this improving. I took a quick look and initially it LGTM.

I want to share that after #3008, I'm considering following the same pattern of one submodule per package, deprecating all the existing GCP containers. As contributor of this GCP module, does it reasonable to you?

My view is quite limited as this is my first contribution to this module 😄 But at first glance it looks reasonable, yes 👍

@mdelapenya mdelapenya self-assigned this Mar 12, 2025
@mdelapenya mdelapenya added the enhancement New feature or request label Mar 12, 2025
@eddumelendez
Copy link
Member

more general approach would be to allow adding more flags to the main command. See testcontainers/testcontainers-java#10067 and testcontainers/testcontainers-node#926

@mdelapenya
Copy link
Member

more general approach would be to allow adding more flags to the main command. See testcontainers/testcontainers-java#10067 and testcontainers/testcontainers-node#926

We'd need to separate the containers in different modules, as with the current approach, all of them are sharing the flags config.

@Zarux
Copy link
Contributor Author

Zarux commented Apr 15, 2025

@mdelapenya Updated the pr to match the new structure.

Had to change firestore/options slightly to allow non-shared options using embedding to allow extending it instead of just aliasing.
Let me know if you want to do it a different way

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, just one thing is missing: updating the docs/modules/gcloud.md file with the new option 🙏

@Zarux Zarux force-pushed the zarux/add-firestore-in-datastore-mode branch from 9794f42 to 690d542 Compare April 15, 2025 10:12
@Zarux Zarux force-pushed the zarux/add-firestore-in-datastore-mode branch from 690d542 to 04c578c Compare April 15, 2025 10:13
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, just added a question regarding the Docker image version, but not a blocker, so I'll merge it once addressed.

Thanks for your hard work with testcontainers-go!

@mdelapenya
Copy link
Member

Oh wait! I forgot to mention that we added new functional options in the core, specifically one to contribute to the CMD: https://golang.testcontainers.org/modules/gcloud/#withcmdargs

Last minute change: could you please update the new option to internally call testcontainers.WithCmdArgs 🙏 ? This will also address @eddumelendez's concerns here: #3009 (comment)

@Zarux
Copy link
Contributor Author

Zarux commented Apr 16, 2025

could you please update the new option to internally call testcontainers.WithCmdArgs 🙏 ?

I don't think that will work without a refactor

  1. The request command is defined after the options are applied. Changing this would require changing the behaviour of default options
  2. The command running is not gcloud, but bash so any arguments added will not be added to the gcloud command, but the bash command. Thus they will not be applied correctly. This is why the extra options are just appended strings to the gcloud command

@mdelapenya
Copy link
Member

could you please update the new option to internally call testcontainers.WithCmdArgs 🙏 ?

I don't think that will work without a refactor

  1. The request command is defined after the options are applied. Changing this would require changing the behaviour of default options
  2. The command running is not gcloud, but bash so any arguments added will not be added to the gcloud command, but the bash command. Thus they will not be applied correctly. This is why the extra options are just appended strings to the gcloud command

Ok, let's do this. Because this PR is in good shape, let's make progress and merge, and think about that refactor as a follow up. Will do a last pass of the review, and eventually merge, thank you so much for your contribution!

mdelapenya
mdelapenya previously approved these changes Apr 17, 2025
@mdelapenya mdelapenya merged commit 37ce316 into testcontainers:main Apr 25, 2025
16 checks passed
mdelapenya added a commit to waroir20/testcontainers-go that referenced this pull request May 7, 2025
* main: (76 commits)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3137)
  chore(deps): bump github.com/shirou/gopsutil/v4 from 4.25.1 to 4.25.4 (testcontainers#3133)
  chore(deps): bump github.com/docker/docker from 28.0.1+incompatible to 28.1.1+incompatible (testcontainers#3152)
  feat(memcached): add memcached module (testcontainers#3132)
  fix(etcd): single node etcd cluster access (testcontainers#3149)
  feat(valkey): add TLS support for Valkey (testcontainers#3131)
  fix(dockermodelrunner): wait for the model to be pulled (testcontainers#3125)
  fix(localstack): remove checksum before parsing version (testcontainers#3130)
  fix(dockermodelrunner): dependency with socat
  chore: prepare for next minor development cycle (0.38.0)
  chore: use new version (v0.37.0) in modules and examples
  fix: handle stopped containers more gracefully when reuse is enabled (testcontainers#3062)
  feat(gcloud): add option to run firestore in datastore mode (testcontainers#3009)
  feat: support for mounting images (testcontainers#3044)
  chore(ci): close PR if it was sent from main (testcontainers#3123)
  feat: add `WithReuseByName` for modifying Generic Container Requests (testcontainers#3064)
  chore(deps): bump github/codeql-action from 3.28.15 to 3.28.16 (testcontainers#3120)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3119)
  chore(deps): bump github.com/magiconair/properties from 1.8.9 to 1.8.10 (testcontainers#3118)
  chore(ci): exclude more files for a full-blown build (testcontainers#3122)
  ...
mdelapenya added a commit to CTrando/testcontainers-go that referenced this pull request May 7, 2025
* main: (302 commits)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3137)
  chore(deps): bump github.com/shirou/gopsutil/v4 from 4.25.1 to 4.25.4 (testcontainers#3133)
  chore(deps): bump github.com/docker/docker from 28.0.1+incompatible to 28.1.1+incompatible (testcontainers#3152)
  feat(memcached): add memcached module (testcontainers#3132)
  fix(etcd): single node etcd cluster access (testcontainers#3149)
  feat(valkey): add TLS support for Valkey (testcontainers#3131)
  fix(dockermodelrunner): wait for the model to be pulled (testcontainers#3125)
  fix(localstack): remove checksum before parsing version (testcontainers#3130)
  fix(dockermodelrunner): dependency with socat
  chore: prepare for next minor development cycle (0.38.0)
  chore: use new version (v0.37.0) in modules and examples
  fix: handle stopped containers more gracefully when reuse is enabled (testcontainers#3062)
  feat(gcloud): add option to run firestore in datastore mode (testcontainers#3009)
  feat: support for mounting images (testcontainers#3044)
  chore(ci): close PR if it was sent from main (testcontainers#3123)
  feat: add `WithReuseByName` for modifying Generic Container Requests (testcontainers#3064)
  chore(deps): bump github/codeql-action from 3.28.15 to 3.28.16 (testcontainers#3120)
  chore(deps): bump mkdocs-include-markdown-plugin from 6.2.2 to 7.1.5 (testcontainers#3119)
  chore(deps): bump github.com/magiconair/properties from 1.8.9 to 1.8.10 (testcontainers#3118)
  chore(ci): exclude more files for a full-blown build (testcontainers#3122)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants