Skip to content

Add cockroach db support - #499

Merged
rnorth merged 20 commits into
testcontainers:masterfrom
tolkonepiu:cockroach
Oct 3, 2019
Merged

Add cockroach db support#499
rnorth merged 20 commits into
testcontainers:masterfrom
tolkonepiu:cockroach

Conversation

@tolkonepiu

Copy link
Copy Markdown
Contributor

Add support for CockroachDB

@kiview

kiview commented Nov 28, 2017

Copy link
Copy Markdown
Member

Hi @tolkonepiu, thanks for the PR. I think our current approach advises to add such functionality as a module in a seperate repo.

@rnorth We still encourage extra module repos, do we?

@kiview

Copy link
Copy Markdown
Member

Please ignore my last point, we'll include small modules in core for now 😃

kiview
kiview previously requested changes Dec 10, 2017

@kiviewkiview left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also add a test for CockroachContainer? Or am I missing something and it's tested implicitly?

@rnorth

Copy link
Copy Markdown
Member

We're shortly going to be merging #574, which changes our build system to Gradle. This is in part intended to make contributions of modules easier (per #564), but unfortunately means that for a short while your PR is going to show merge conflicts with the master branch.

I just want to let you know we don't want to create new work for you: we'll take care of the merge conflicts shortly. Please don't worry - we're grateful for your PR and want to help integrate it soon. Thank you.

@rnorth

Copy link
Copy Markdown
Member

@tolkonepiu I've locally made changes to bring this into the new Gradle-based build, as we'd love to merge this soon.

Could you perhaps allow us to push to your fork so that I can make the necessary changes? This page describes the steps: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

Thank you in advance!

@stale

staleBot commented Jan 7, 2019

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stalestaleBot added the stale label Jan 7, 2019
@rnorth

Copy link
Copy Markdown
Member

We should keep this open.

Sent with GitHawk

@kiview
kiview requested a review from bsideup as a code ownerJanuary 8, 2019 19:44
@stalestaleBot removed stale labels Jan 8, 2019
@kiview

Copy link
Copy Markdown
Member

@tolkonepiu Sorry for keeping thins dangling so long.
@rnorth I've fixed the merge conflicts, I think it's good to go?

@bsideup

Copy link
Copy Markdown
Member

@kiview let's drop the self typing

@testcontainerstestcontainers deleted a comment Jan 11, 2019
@testcontainerstestcontainers deleted a comment Jan 11, 2019
@rnorth

Copy link
Copy Markdown
Member

I'll take on @bsideup's comment.

@rnorthrnorth self-assigned this May 27, 2019
@rnorth
rnorth requested review from bsideup and kiviewMay 27, 2019 20:41
@rnorth

Copy link
Copy Markdown
Member

FWIW I've raised this ticket around the current gap in our docs for new module contributors. I'd like to have a bit of a think about the third point specifically before merging this, but otherwise I'm fine with this new module at an overall level: #1503

@stale

staleBot commented Aug 26, 2019

Copy link
Copy Markdown

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stalestaleBot added the stale label Aug 26, 2019
@rnorth

Copy link
Copy Markdown
Member

Not stale - reopening.

@stalestaleBot removed the stale label Aug 26, 2019
@reinierbos

Copy link
Copy Markdown

Any idea when this PR will be merged to master (and then released)? I would love to start using it!

@rnorth
rnorth dismissed kiview’s stale reviewOctober 1, 2019 19:50

Dismissing stale review

@rnorthrnorth added this to the next milestone Oct 1, 2019
super(dockerImageName);

withExposedPorts(REST_API_PORT, DB_PORT);
withEnv("COCKROACH_USER", username);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should go to "configure" method

@rnorthrnorthOct 3, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (disabling these settings, since they don't work!)

Since there is no current way to make them do what we want
@rnorth
rnorth requested a review from bsideupOctober 3, 2019 13:07
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@tolkonepiu@kiview@rnorth@bsideup@reinierbos@glebsts