refactor(cockroachdb): to use request driven options #2883

stevenh · 2024-11-08T01:55:30Z

Refactor cockroachdb module to to use request driven options simplifying the flow.

Clean up tests eliminating the use of suite which significantly speeds up test runs.

netlify · 2024-11-08T01:55:48Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`83a7838`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/6740cfcb162998000802f123
😎 Deploy Preview	https://deploy-preview-2883--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/options.go

stevenh · 2024-11-08T23:22:00Z

This should be split out into components if we want to go down this route.

mdelapenya

I did an initial pass and the overall feedback is positive. I detected the following possible chunks of separate PRs, please tell me what you think:

wait.walk for traversing multi strategies
wait for TLS
cockroachdb refactor

I think it would be much clearer for the review, but also for the release notes, as consumers would discover the new wait strategies as part of it.

mdelapenya · 2024-11-22T17:04:51Z

modules/cockroachdb/data/cluster_defaults.sql

@@ -0,0 +1,8 @@
+SET CLUSTER SETTING kv.range_merge.queue_interval = '50ms';


question: are these settings the same for different cockroachdb versions? If we always set them, no matter the chosen version, then it could lead to misconfiguring the container.

Looks like there is one change since this advice was introduced in v21.2.17 (a setting was removed in v21.2.22), so looks like we're good. If this was needed we could look to introduce version conditionals.

mdelapenya · 2024-11-22T17:09:26Z

modules/cockroachdb/options.go

-		o.StoreSize = size
+// WithInitScripts adds the given scripts to those automatically run when the container starts.
+// These will be ignored if data exists in the `/cockroach/cockroach-data` directory within the container.
+func WithInitScripts(scripts ...string) testcontainers.CustomizeRequestOption {


question: should we use paths or readers, to allow users fetch the files from wherever they want?

Readers provides more flexibility so switching to that would be a good idea. Users that want to read from a file can easily use embed + bytes.NewReader to achieve that.

mdelapenya · 2024-11-22T17:12:48Z

wait/tls.go

+var _ Strategy = (*TLSStrategy)(nil)
+
+// TLSStrategy is a strategy for handling TLS.
+type TLSStrategy struct {


docs: we need to add a page for this in the docs (see mkdocs.yml in the root dir, and the wait folder under ./docs/features)

mdelapenya · 2024-11-22T17:14:21Z

wait/tls_test.go

+)
+
+var (
+	//go:embed testdata/cert.crt


question: Have you considered using https://github.com/mdelapenya/tlscert instead?

I saw that, but it was hiding errors e.g. SelfSignedFromRequest returns nil on error, so followed the pattern in the standard library. If that got cleaned up we could change later.

mdelapenya · 2024-11-22T17:16:36Z

wait/walk_test.go

+)
+
+func TestWalk(t *testing.T) {
+	req := testcontainers.ContainerRequest{


suggestion: I think we do not need the req, just the WaitingFor field

You're correct, but I wanted the test to act as an example of its use as well, hence using request.

mdelapenya · 2024-11-22T17:22:52Z

wait/walk.go

+	if err := visit(*root); err != nil {
+		if errors.Is(err, VisitRemove) {
+			*root = nil
+		}
+
+		return err
+	}
+
+	if s, ok := (*root).(*MultiStrategy); ok {


question: shouldn't we check first if the root is a multi-strategy, to iterate through the child elements?

I wanted to allow the caller to customise the behaviour. This way round they could return an error or VisitRemove to remove the element without having to traverse it first. Added a comment to this effect.

mdelapenya · 2024-11-22T17:24:24Z

wait/walk_test.go

+			return nil
+		})
+		require.NoError(t, err)
+		require.Equal(t, 5, count)


question: because the root has no logic, apart from being a "container" for the children strategies, should we count 4 here, therefore affecting the processing? I think this is related to my previous question about the root node being a multi.

As we visit the root first to allow it to be customised, the root is also counted.

wait/walk_test.go

stevenh · 2024-11-22T17:50:15Z

I did an initial pass and the overall feedback is positive. I detected the following possible chunks of separate PRs, please tell me what you think:

wait.walk for traversing multi strategies

wait for TLS

cockroachdb refactor

I think it would be much clearer for the review, but also for the release notes, as consumers would discover the new wait strategies as part of it.

That's pretty much my thought's too, which is why I've left in draft.

…list of statements

Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>

… in defaultOptions

Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>

… 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.

Refactor cockroachdb module to to use request driven options, simplifying the flow.

Various fixes and remove the ability to configure certs using the default generated ones instead.

Add the missing data file and remove unused constants.

Extract TLS certificate wait strategy into a dedicated wait type so it can be reused. Implement walk method which can be used to identify wait strategies in a request chain. Use embed to simplify wait test loading of certs.

Remove the now unused customizer.

Fix lint style issue for blank line.

Improve test coverage as per review feedback.

stevenh force-pushed the refactor/cockroachdb branch 4 times, most recently from fbfb05c to 6dc9c5c Compare November 8, 2024 02:29

stevenh mentioned this pull request Nov 8, 2024

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

Draft

mdelapenya reviewed Nov 8, 2024

View reviewed changes

modules/cockroachdb/options.go Show resolved Hide resolved

stevenh mentioned this pull request Nov 22, 2024

fix(kafka): Fix internal docker connection #2894

Open

stevenh marked this pull request as ready for review November 22, 2024 15:54

stevenh requested a review from a team as a code owner November 22, 2024 15:54

stevenh marked this pull request as draft November 22, 2024 15:54

mdelapenya reviewed Nov 22, 2024

View reviewed changes

martskins and others added 16 commits November 22, 2024 18:38

feat: use recommended cluster settings for cockroachdb

f7d88ce

fix: wrap errors

bb32c1a

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

63b69f8

…list of statements

Update modules/cockroachdb/options.go

62e5828

Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>

Update modules/cockroachdb/cockroachdb.go

79e979e

Co-authored-by: Steven Hartland <stevenmhartland@gmail.com>

fix: return close error on defer

c8be97c

chore: improvement documentation and set default value for statements…

d4110fb

… in defaultOptions

Update modules/cockroachdb/cockroachdb.go

36f8594

Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>

chore: remove unnecessary nolint directive and rename ClusterDefaults…

4f46dff

… to DefaultStatements

fix: only use recommended settings in root user tests

d7bf682

refactor(cockroachdb): to use request driven options

f0ac8db

Refactor cockroachdb module to to use request driven options, simplifying the flow.

fix: various fixes and remove certs

76e812c

Various fixes and remove the ability to configure certs using the default generated ones instead.

fix: add missing data files

c757265

Add the missing data file and remove unused constants.

chore: add reference to ErrTLSNotEnabled

47eb6e3

fix: lint failure for missing test helper

0a5e679

stevenh added 6 commits November 22, 2024 18:38

feat: tls strategy and strategy walk

42697f6

Extract TLS certificate wait strategy into a dedicated wait type so it can be reused. Implement walk method which can be used to identify wait strategies in a request chain. Use embed to simplify wait test loading of certs.

chore(cockroachdb): remove customizer

ec7c495

Remove the now unused customizer.

fix: check setOptions error return

fac8449

fix: lint style

2b991b1

Fix lint style issue for blank line.

chore: clarify why visit it called before iterating

10f341e

tests: improve test coverage

83a7838

Improve test coverage as per review feedback.

stevenh force-pushed the refactor/cockroachdb branch from 158644d to 83a7838 Compare November 22, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cockroachdb): to use request driven options #2883

refactor(cockroachdb): to use request driven options #2883

stevenh commented Nov 8, 2024

netlify bot commented Nov 8, 2024 •

edited

Loading

stevenh commented Nov 8, 2024

mdelapenya left a comment

mdelapenya Nov 22, 2024

stevenh Nov 22, 2024

mdelapenya Nov 22, 2024

stevenh Nov 22, 2024

mdelapenya Nov 22, 2024

mdelapenya Nov 22, 2024

stevenh Nov 22, 2024

mdelapenya Nov 22, 2024

stevenh Nov 22, 2024

mdelapenya Nov 22, 2024

stevenh Nov 22, 2024

mdelapenya Nov 22, 2024

stevenh Nov 22, 2024

stevenh commented Nov 22, 2024

		@@ -0,0 +1,8 @@
		SET CLUSTER SETTING kv.range_merge.queue_interval = '50ms';

refactor(cockroachdb): to use request driven options #2883

Are you sure you want to change the base?

refactor(cockroachdb): to use request driven options #2883

Conversation

stevenh commented Nov 8, 2024

netlify bot commented Nov 8, 2024 • edited Loading

✅ Deploy Preview for testcontainers-go ready!

stevenh commented Nov 8, 2024

mdelapenya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenh commented Nov 22, 2024

netlify bot commented Nov 8, 2024 •

edited

Loading