Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: use recommended cluster settings for cockroachdb #2858
feat: use recommended cluster settings for cockroachdb #2858
Changes from all commits
c42273f
1fe0c72
fe3d3ee
fb1139a
dc03f27
fee481a
2553165
096672b
f7dc17f
ff63c4c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: This is somewhat confusing as this won't be combined with
DefaultStatements
which is how I would interpret this currently. We should clarify and ensure we clearly state the default if not configured isDefaultStatements
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are always adding the default statements, then I think they must be private, and probably prepended to the user-provided ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, but given the complications with TLS and users without MODIFYCLUSTERSETTING privilege I think prepending to the user-provided statements is probably going to cause a bit of pain.
Good spot on the comment. I'll hold on that change until we've established what we do about the TLS and user privileges thing though, as it might affect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Private vs public for these is an interesting discussion, here's some ideas from the perspective of making the var private:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the goals of any testcontainers module is to be simple, and maybe opinionated, helping users in not committing mistakes. So I'd look for the mechanism that creates this experience.
Whatever you find that matches that, it's fine for me 😊