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