Skip to content

Conversation

blancqua
Copy link

@blancqua blancqua commented Feb 26, 2025

See discussion #10031

@blancqua blancqua requested a review from a team February 26, 2025 08:51
Copy link
Contributor

@luketn luketn left a comment

Choose a reason for hiding this comment

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

Good idea! Only change I would recommend is using the term 'database specific connection string' because:

  1. connectionString is more consistent with the existing getConnectionString() method
  2. the database specific connection string is not related to replica sets. it's just a way to include the database name in the connection string rather than having to select a database afterward

@blancqua
Copy link
Author

Sounds good to me! Included your suggestion and merged them all together in 1 commit.

@blancqua
Copy link
Author

@eddumelendez, spotless changes applied. One flaky test (ArtemisContainerTest), but I don't see this having anything to do with the proposed change.

Kind regards, Wouter

@blancqua
Copy link
Author

blancqua commented Mar 5, 2025

Hi guys, anything else you would like to see changed before this can be merged?

Copy link
Contributor

@luketn luketn left a comment

Choose a reason for hiding this comment

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

Looks great!

@blancqua
Copy link
Author

Hello @eddumelendez, @luketn. I'm reaching out since this is now open for a while. Anything still holding you back from merging this?

Kind regards,
Wouter

@eddumelendez eddumelendez added this to the next milestone Apr 3, 2025
@eddumelendez eddumelendez merged commit e562568 into testcontainers:main Apr 3, 2025
106 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @blancqua!

Sign up for free to 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.

3 participants