Add CockroachDB module

[leourbina]

Adds CockroachDB Support

This is mainly the same as the postgres container, with a few minor details:

• It uses CRDB's --insecure flag to avoid needing to deal with certs for local testing/connections
• Due to the above, no password is set

[netlify]

✅ Deploy Preview for testcontainers-node ready!

Name	Link
🔨 Latest commit	075173a
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-node/deploys/67c5d44ba29164000879a46d
😎 Deploy Preview	https://deploy-preview-910--testcontainers-node.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[leourbina]

@cristianrgreco Let me know what, if anything, you need from me here to get this merged.

[cristianrgreco]

@leourbina Sorry for the delay! Just one thing to address, rest LGTM.

[leourbina]

NP! Just addressed the latest comment and added the test. I had to tweak the wait condition to be more robust and ensure that it would work across restarts. Thanks for the thorough review.

[leourbina]

@cristianrgreco ok, I think it should be good now.

[leourbina]

Urgh... ok I think the healthcheck is not as robust as I actually thought. I'll look into it.

[leourbina]

Hey @cristianrgreco I'm running into the following issue:

Whenever the container starts for the first time, it will run the init scripts. These need to finish running as they setup the user/db and need to be waited for before running anything against it. The only reliable way that I've figured out to detect this is to wait for the log message as so:

    this.withExposedPorts(COCKROACH_PORT)
      .withCommand(["start-single-node", "--insecure"])
      .withWaitStrategy(Wait.forLogMessage(/.*end running init files.*/))
      .withStartupTimeout(120_000);

This is effectively the same thing that the postgres container is doing here.

However, on restarts the init scripts don't run again. I tried fixing it using a healthcheck (as seen on this PR), but I think it is working on my machine as the timing lines up, but based on the test results it is not reliably waiting for the init scripts.

As it is currently written the restart issue we're seeing on the Cockroachdb container exists too on the postgres container. I'm not sure if there are other better ways to ensure that the container is ready both on the first start as well as after a restart. Any ideas here?

[leourbina]

@cristianrgreco currently the postgres container does not work across restarts either as it suffers from the same issue I pointed above. Namely, the wait condition does not work across restarts. Any chance we can merge this as is? Or do you think we need to fix both CRDB and Postgres as part of this PR?

[cristianrgreco]

Hi @leourbina, I've raised a PR for fixing the PG container across restarts: #914

For the Cockroach container I see in the testcontainers-java implementation that they're calling an endpoint ending with init_success, which I guess returns 200 once the init is finished - this sounds like what you need:

waitStrategy.withStrategy(
    Wait.forHttp("/health").forPort(REST_API_PORT).forStatusCode(200).withStartupTimeout(Duration.ofMinutes(1))
);
if (this.isVersionGreaterThanOrEqualTo221) {
    waitStrategy.withStrategy(Wait.forSuccessfulCommand("[ -f ./init_success ] || { exit 1; }"));
}

You can also refactor the custom curl command in the healthcheck with Wait.forHttp(...)

[cristianrgreco]

Calling the init_success endpoint had the same issue as the PG container. If the interval is very frequent, the container is still not ready to accept connections even when that endpoint returns 200.

I've committed a change similar to the PG container healthcheck. Seems to work locally and it allows us to make the interval more frequent for faster startups. Pipeline is green.

At the moment the changes I've made mean that a user cannot override the CockroachDB healthcheck. I've fixed this as well in the PG PR, but will need to merge that first to be able to do the same here.

[cristianrgreco]

Latest changes should be final. @leourbina is there anything more or is this PR good to go?

[leourbina]

Hey @cristianrgreco! Thanks for looking into this 🔥. Those changes sound reasonable.

[cristianrgreco]

Thanks @leourbina for your contribution!

[leourbina]

Thank you @cristianrgreco!