feat!(cockroachdb): use recommended cluster settings #2869

stevenh · 2024-11-01T14:47:02Z

Configures the cockroachdb container with the settings recommended by Cockroach Labs for testing clusters.

Simplify the connection handling in cockroachdb so that ConnectionString can be used without the user doing extra work to handle TLS if enabled.

Deprecate TLSConfig which is no longer needed separately.

BREAKING_CHANGE: This now returns a registered connection string so is no longer compatible with pgx.ParseConfig, use ConnectionConfig for this use case instead.

netlify · 2024-11-01T14:47:17Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`c58631b`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/672a0f870e39050008034429
😎 Deploy Preview	https://deploy-preview-2869--testcontainers-go.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.

modules/cockroachdb/certs.go

…list of statements

Co-authored-by: Steven Hartland <[email protected]>

… in defaultOptions

Co-authored-by: Manuel de la Peña <[email protected]>

… to DefaultStatements

Simplify the connection handling in cockroachdb so that ConnectionString can be used without the user doing extra work to handle TLS if enabled. Deprecate TLSConfig which is no longer needed separately. BREAKING_CHANGE: This now returns a registered connection string so is no longer compatible with pgx.ParseConfig, use ConnectionConfig for this use case instead.

stevenh · 2024-11-06T10:33:46Z

@martskins could you review these changes based on top of your PR #2869 please.

mdelapenya · 2024-11-06T11:27:51Z

modules/cockroachdb/cockroachdb_test.go

 		opts: []testcontainers.ContainerCustomizer{
 			cockroachdb.WithUser("foo"),
 			cockroachdb.WithPassword("bar"),
+			// Do not run the default statements as the user used on this test is


question: after reading this comment, is it possible to have a test using WithUser and with the default statements?

question: after reading this comment, is it possible to have a test using WithUser and with the default statements?

as long as the user has MODIFYCLUSTERSETTING privilege, yes

So at that point, the user must provide statements to configure the MODIFYCLUSTERSETTING privilege for the user in order this to work, right? Should the library grant this privilege automatically in the case the WithUser option is passed?

I suppose that could an option, yes. Alternatively, it looks like we don't allow using the TLS option with a user other than the default one (root), should we maybe do the same here?

We can probably do it in a follow-up. Wdyt? Would you like to be involved in that?

I'm happy to be involved, but if the preferred path is to make this error if default statements are used with a non-root user then it might be better to do it in this PR so we don't ship this only to introduce a breaking change to it right after? Although I suppose the cluster would fail to start in that scenario anyways so maybe it's not that much of a breaking change.

martskins · 2024-11-06T14:54:09Z

@martskins could you review these changes based on top of your PR #2869 please.

Looks good to me! 👍

mdelapenya

@stevenh is this a breaking change? I do not see any change in the signatures of the public API, just changes in how we build the examples on how to use it, but maybe I'm wrong.

Other than that LGTM

eddumelendez · 2024-11-06T17:48:31Z

FYI, I have raised an issue in CockroachDb repo in order to consider having an env var to apply those settings automatically and take advantage of it in other Testcontainers implementation without changes. See cockroachdb/cockroach#134429

stevenh · 2024-11-06T19:03:49Z

@stevenh is this a breaking change? I do not see any change in the signatures of the public API, just changes in how we build the examples on how to use it, but maybe I'm wrong.

Other than that LGTM

The connection handling is not 100% backwards compatible as detailed in the PR message:

This now returns a registered connection string so is no longer compatible with pgx.ParseConfig, use ConnectionConfig for this use case instead.

mdelapenya · 2024-11-07T13:18:56Z

I think this is ready to be merge, but let's wait a bit until @eddumelendez 's question is addressed in the upstream project. That would be a good improvement if all the TC languages would benefit from this, instead of having each custom code.

eddumelendez · 2024-11-07T21:07:50Z

In order to follow the suggestion, the wait strategy should also check if a file was created. See testcontainers/testcontainers-java#9505. This approach is valid for CockroachDB >= 22.1.0

stevenh · 2024-11-08T02:37:55Z

In order to follow the suggestion, the wait strategy should also check if a fail was created. See testcontainers/testcontainers-java#9505. This approach is valid for CockroachDB >= 22.1.0

Thanks @eddumelendez I've incorporated that into refactored approach for the go CockroachDB module in #2883

This is still draft but I think the changes improve the flow, avoiding manually creating certs which prevents a bunch of the edge cases in the current version as well as streamlining to logic so users can use the functional options with less restrictions.

Be interested in feedback.

stevenh · 2024-11-08T10:22:05Z

Converted this to draft while we discuss the different approaches

stevenh mentioned this pull request Nov 1, 2024

feat: use recommended cluster settings for cockroachdb #2858

Closed

stevenh force-pushed the recommended-cluster-settings branch from f704010 to d34f4f2 Compare November 1, 2024 16:29

stevenh added the enhancement New feature or request label Nov 3, 2024

stevenh force-pushed the recommended-cluster-settings branch from d34f4f2 to 8f29ffd Compare November 3, 2024 16:48

mdelapenya reviewed Nov 5, 2024

View reviewed changes

modules/cockroachdb/certs.go Show resolved Hide resolved

stevenh force-pushed the recommended-cluster-settings branch 2 times, most recently from 430200b to aaab322 Compare November 5, 2024 10:59

martskins and others added 10 commits November 5, 2024 10:59

feat: use recommended cluster settings for cockroachdb

a2a7059

fix: wrap errors

1bcaa9d

feat: add WithStatements option and expose recommended settings as a …

4a7f3e3

…list of statements

Update modules/cockroachdb/options.go

4046527

Co-authored-by: Steven Hartland <[email protected]>

Update modules/cockroachdb/cockroachdb.go

d08af65

Co-authored-by: Steven Hartland <[email protected]>

fix: return close error on defer

a03ed97

chore: improvement documentation and set default value for statements…

94a81ba

… in defaultOptions

Update modules/cockroachdb/cockroachdb.go

4570397

Co-authored-by: Manuel de la Peña <[email protected]>

chore: remove unnecessary nolint directive and rename ClusterDefaults…

cf8625d

… to DefaultStatements

fix: only use recommended settings in root user tests

50878ca

stevenh force-pushed the recommended-cluster-settings branch from aaab322 to c58631b Compare November 5, 2024 12:28

stevenh added the breaking change Causing compatibility issues. label Nov 6, 2024

stevenh changed the title ~~feat: use recommended cluster settings for cockroachdb~~ feat!(cockroachdb): use recommended cluster settings Nov 6, 2024

stevenh marked this pull request as ready for review November 6, 2024 10:32

stevenh requested a review from a team as a code owner November 6, 2024 10:32

mdelapenya reviewed Nov 6, 2024

View reviewed changes

mdelapenya approved these changes Nov 6, 2024

View reviewed changes

eddumelendez mentioned this pull request Nov 6, 2024

sql: Apply recommended cluster settings and sql statements to Docker image via env var cockroachdb/cockroach#134429

Open

stevenh marked this pull request as draft November 8, 2024 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!(cockroachdb): use recommended cluster settings #2869

feat!(cockroachdb): use recommended cluster settings #2869

stevenh commented Nov 1, 2024 •

edited

Loading

netlify bot commented Nov 1, 2024 •

edited

Loading

stevenh commented Nov 6, 2024

mdelapenya Nov 6, 2024

martskins Nov 6, 2024

mdelapenya Nov 6, 2024

martskins Nov 6, 2024 •

edited

Loading

mdelapenya Nov 6, 2024

martskins Nov 6, 2024

martskins commented Nov 6, 2024

mdelapenya left a comment

eddumelendez commented Nov 6, 2024

stevenh commented Nov 6, 2024

mdelapenya commented Nov 7, 2024

eddumelendez commented Nov 7, 2024 •

edited

Loading

stevenh commented Nov 8, 2024

stevenh commented Nov 8, 2024

feat!(cockroachdb): use recommended cluster settings #2869

Are you sure you want to change the base?

feat!(cockroachdb): use recommended cluster settings #2869

Conversation

stevenh commented Nov 1, 2024 • edited Loading

netlify bot commented Nov 1, 2024 • edited Loading

✅ Deploy Preview for testcontainers-go ready!

stevenh commented Nov 6, 2024

mdelapenya Nov 6, 2024

Choose a reason for hiding this comment

martskins Nov 6, 2024

Choose a reason for hiding this comment

mdelapenya Nov 6, 2024

Choose a reason for hiding this comment

martskins Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

mdelapenya Nov 6, 2024

Choose a reason for hiding this comment

martskins Nov 6, 2024

Choose a reason for hiding this comment

martskins commented Nov 6, 2024

mdelapenya left a comment

Choose a reason for hiding this comment

eddumelendez commented Nov 6, 2024

stevenh commented Nov 6, 2024

mdelapenya commented Nov 7, 2024

eddumelendez commented Nov 7, 2024 • edited Loading

stevenh commented Nov 8, 2024

stevenh commented Nov 8, 2024

stevenh commented Nov 1, 2024 •

edited

Loading

netlify bot commented Nov 1, 2024 •

edited

Loading

martskins Nov 6, 2024 •

edited

Loading

eddumelendez commented Nov 7, 2024 •

edited

Loading