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

feat(modules.cockroachdb) Adds cockroachdb module #2131

Merged
merged 21 commits into from
Jan 29, 2024

Conversation

rcrowe
Copy link
Contributor

@rcrowe rcrowe commented Jan 21, 2024

What does this PR do?

Adds CockroachDB as a module

  • TLS support via cockroachdb.WithTLS & the helper cockroachdb.NewTLSConfig
  • COCKROACH_DATABASE support via cockroachdb.WithDatabase
  • COCKROACH_USER support via cockroachdb.WithUser
  • COCKROACH_PASSWORD support via cockroachdb.WithPassword

Why is it important?

Previously an example only, a module lets us develop it over time to include features like TLS.

@rcrowe rcrowe requested a review from a team as a code owner January 21, 2024 18:52
Copy link

netlify bot commented Jan 21, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 9153188
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/65b67b2e983bdd0008a5557b
😎 Deploy Preview https://deploy-preview-2131--testcontainers-go.netlify.app/modules/cockroachdb
📱 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.

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, thanks for this new module! I only left a comment regarding the WithImageTag option. Once discussed, I think this PR is ready to be merged, thanks!

docs/modules/cockroachdb.md Outdated Show resolved Hide resolved
@mdelapenya mdelapenya self-assigned this Jan 22, 2024
@mdelapenya mdelapenya added the enhancement New feature or request label Jan 22, 2024
@rcrowe rcrowe requested a review from mdelapenya January 22, 2024 08:18
@mdelapenya
Copy link
Member

@rcrowe the errors in the rootless-mode pipeline are not related to this PR but to an automatic bump in the Github worker, which is installing Docker v25.0.0 since a few days ago. I'm talking to the Moby team to check if that's a regression or the expected behaviour, because the failures are consistent:

  1. One of the failures is related to an error message that seems to have changed, when build target is not found, from failed to reach build target target-foo in Dockerfile to target stage 'target-foo' could not be found #46754
    • I think we can simply check for the error and avoid asserting for a specific text in the error message
  2. Second error is related to the number of network aliases for the the bridge network, which in rootless mode seems to have changed from 0 to 1: Expected number of aliases for 'bridge' network 0. Got 1.
    • I'm not sure about this one, as in non-rootless mode, the same test passes

@rcrowe
Copy link
Contributor Author

rcrowe commented Jan 22, 2024

Thanks @mdelapenya

Gives me some time to finish TLS support & push that up as a separate PR, unless you want me to include it here?

@rcrowe rcrowe force-pushed the module-cockroachdb branch 2 times, most recently from 0a6b8ca to 9e4fa57 Compare January 22, 2024 21:39
@rcrowe
Copy link
Contributor Author

rcrowe commented Jan 23, 2024

@mdelapenya I'm assuming docker rootless failing here because it's flaky, notice on main that it passed, rather than anything in my changeset?

Have addressed the feedback of COCKROACH_DATABASE, COCKROACH_USER & COCKROACH_PASSWORD, along with TLS support. The PR grew a little because of that, so apologies for the larger diff.

@rcrowe rcrowe requested a review from eddumelendez January 23, 2024 11:48
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.

Thanks for your hard work adding this new module. I left a few comments that are non-blocking, so I think we can merge the module once discussed.

In any case, great job!

modules/cockroachdb/cockroachdb.go Show resolved Hide resolved
docs/modules/cockroachdb.md Outdated Show resolved Hide resolved
modules/cockroachdb/cockroachdb.go Show resolved Hide resolved
@rcrowe
Copy link
Contributor Author

rcrowe commented Jan 25, 2024

@mdelapenya 🤞🏻 this is in a shape to get merged & iterate once it's in the hands of people 🤞🏻

Thanks for your feedback 🙇🏻

@rcrowe rcrowe requested a review from mdelapenya January 25, 2024 21:53
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, thank you for your contribution!

I added a few minor comments, once they are addressed, we are good to go! 🚀

modules/cockroachdb/cockroachdb.go Show resolved Hide resolved
modules/cockroachdb/cockroachdb.go Outdated Show resolved Hide resolved
rcrowe and others added 9 commits January 27, 2024 11:01
Cockroach wasn't being consistent with log messages to use that as a
reliable mechanism across the different authentication methods.

This commit makes an actual SQL query to validate CRDB is correctly
running.

So we can make use of sql.DB driver and `wait.ForSQL` we've had to
register a connection URL that includes TLS config, as there's no way to
do that through standard connection strings.
Co-authored-by: Manuel de la Peña <[email protected]>
@rcrowe rcrowe force-pushed the module-cockroachdb branch from f8e20cd to ebf8373 Compare January 27, 2024 11:03
@rcrowe rcrowe requested a review from mdelapenya January 27, 2024 11:19
Co-authored-by: Manuel de la Peña <[email protected]>
@rcrowe rcrowe requested a review from mdelapenya January 28, 2024 16:05
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, thanks for this contribution and for your patience while reviewing the code.

Merging it now

@mdelapenya mdelapenya merged commit 7151628 into testcontainers:main Jan 29, 2024
80 checks passed
@rcrowe rcrowe deleted the module-cockroachdb branch January 29, 2024 08:21
mdelapenya added a commit to laskoviymishka/testcontainers-go that referenced this pull request Jan 29, 2024
* main:
  chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#2162)
  feat(modules.cockroachdb) Adds cockroachdb module (testcontainers#2131)
mdelapenya added a commit to tateexon/testcontainers-go that referenced this pull request Jan 29, 2024
* main: (74 commits)
  chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#2162)
  feat(modules.cockroachdb) Adds cockroachdb module (testcontainers#2131)
  chore(deps): bump golang.org/x/crypto in /modules/minio (testcontainers#2161)
  chore(deps): bump golang.org/x/crypto in /modules/openldap (testcontainers#2165)
  chore(deps): bump github.com/google/uuid from 1.5.0 to 1.6.0 (testcontainers#2169)
  chore(deps): bump google.golang.org/api from 0.156.0 to 0.159.0, google.golang.org/grpc from 1.60.1 to 1.61.0, cloud.google.com/go/pubsub from 1.33.0 to 1.35.0 in /modules/gcloud (testcontainers#2168)
  chore(deps): bump github.com/hashicorp/consul/api in /examples/consul (testcontainers#2152)
  chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#2145)
  chore(deps): bump k8s.io/api, k8s.io/apimachinery and k8s.io/client-go from 0.29.0 to 0.29.1 in /modules/k3s (testcontainers#2167)
  chore: do not compile modules on macos workers on GH (testcontainers#2164)
  Openldap module support (testcontainers#2117)
  Adding inbucket module (testcontainers#2142)
  testifylint: enable compares rule (testcontainers#2143)
  Bump containerd version to v1.7.12 (testcontainers#2137)
  feat: Add Minio module (testcontainers#2132)
  Adding LogConsumers start as part of the ContainerRequest (testcontainers#2073)
  chore: bring back assertion for network aliases for bridge in rootless mode (testcontainers#2141)
  chore(deps): bump github.com/docker/compose/v2 from 2.23.3 to 2.24.0 in /modules/compose (testcontainers#2096)
  chore(deps): bump github.com/dvsekhvalnov/jose2go in /modules/pulsar (testcontainers#2136)
  fix: skip-host-cache option removed in latest MySQL 8.3.0 version (testcontainers#2130)
  ...
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Feb 14, 2024
* main: (33 commits)
  feat (postgres): support for creating and restoring Snapshots (testcontainers#2199)
  fix: apply volume options only to volumes (testcontainers#2201)
  redpanda/test: add admin client call (testcontainers#2200)
  chore(deps): bump cloud.google.com/go/spanner from 1.55.0 to 1.56.0 in /modules/gcloud, cloud.google.com/go/pubsub from 1.35.0 to 1.36.1 in /modules/gcloud, cloud.google.com/go/bigquery from 1.57.1 to 1.58.0 in /modules/gcloud (testcontainers#2197)
  chore(deps): bump github.com/docker/docker from 25.0.1+incompatible to 25.0.2+incompatible (testcontainers#2196)
  fix: go doc reference broken image (testcontainers#2195)
  Add Support for WASM Transforms to Redpanda Module (testcontainers#2170)
  feat(modules.clickhouse): Add zookeeper for clickhouse clusterization (testcontainers#1995)
  redpanda: allow using SASL and TLS together (testcontainers#2140)
  chore: do not panic in testable examples (testcontainers#2193)
  fix: all mounts should contain the testcontainers labels (testcontainers#2191)
  [redpanda] sasl test for wrong mechanism (testcontainers#2048)
  fix: deprecate BindMounts correctly (testcontainers#2190)
  chore(docker_mounts): stop doing misleading logging (testcontainers#2178)
  fix: Add HTTPStrategy WithForcedIPv4LocalHost To Fix Docker Port Map (testcontainers#1775)
  chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#2162)
  feat(modules.cockroachdb) Adds cockroachdb module (testcontainers#2131)
  chore(deps): bump golang.org/x/crypto in /modules/minio (testcontainers#2161)
  chore(deps): bump golang.org/x/crypto in /modules/openldap (testcontainers#2165)
  chore(deps): bump github.com/google/uuid from 1.5.0 to 1.6.0 (testcontainers#2169)
  ...
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Feb 14, 2024
* main: (33 commits)
  feat (postgres): support for creating and restoring Snapshots (testcontainers#2199)
  fix: apply volume options only to volumes (testcontainers#2201)
  redpanda/test: add admin client call (testcontainers#2200)
  chore(deps): bump cloud.google.com/go/spanner from 1.55.0 to 1.56.0 in /modules/gcloud, cloud.google.com/go/pubsub from 1.35.0 to 1.36.1 in /modules/gcloud, cloud.google.com/go/bigquery from 1.57.1 to 1.58.0 in /modules/gcloud (testcontainers#2197)
  chore(deps): bump github.com/docker/docker from 25.0.1+incompatible to 25.0.2+incompatible (testcontainers#2196)
  fix: go doc reference broken image (testcontainers#2195)
  Add Support for WASM Transforms to Redpanda Module (testcontainers#2170)
  feat(modules.clickhouse): Add zookeeper for clickhouse clusterization (testcontainers#1995)
  redpanda: allow using SASL and TLS together (testcontainers#2140)
  chore: do not panic in testable examples (testcontainers#2193)
  fix: all mounts should contain the testcontainers labels (testcontainers#2191)
  [redpanda] sasl test for wrong mechanism (testcontainers#2048)
  fix: deprecate BindMounts correctly (testcontainers#2190)
  chore(docker_mounts): stop doing misleading logging (testcontainers#2178)
  fix: Add HTTPStrategy WithForcedIPv4LocalHost To Fix Docker Port Map (testcontainers#1775)
  chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#2162)
  feat(modules.cockroachdb) Adds cockroachdb module (testcontainers#2131)
  chore(deps): bump golang.org/x/crypto in /modules/minio (testcontainers#2161)
  chore(deps): bump golang.org/x/crypto in /modules/openldap (testcontainers#2165)
  chore(deps): bump github.com/google/uuid from 1.5.0 to 1.6.0 (testcontainers#2169)
  ...
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