Skip to content
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

Support using MariaDB JDBC URL to set database name, user and password #950

Merged
merged 7 commits into from
Nov 10, 2018

Conversation

guss77
Copy link
Contributor

@guss77 guss77 commented Nov 4, 2018

Implement feature compatibility with MySQL container requested in issue #949

Also support user name and password using query string parameters.

Implementation copied verbatim from `MySQLContainerProvider` to address issue testcontainers#949 .
Required to support the container provider changes for issue testcontainers#949
@testcontainers testcontainers deleted a comment Nov 4, 2018
@kiview
Copy link
Member

kiview commented Nov 5, 2018

Hi @guss77, thanks for the PR.
Can you please add the corresponding tests to

{"jdbc:tc:mysql:5.5.43://hostname/databasename?useSSL=false", EnumSet.noneOf(Options.class)},
?

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Add tests for JDBC driver

@guss77
Copy link
Contributor Author

guss77 commented Nov 5, 2018

Something like this?

@testcontainers testcontainers deleted a comment Nov 5, 2018
@kiview
Copy link
Member

kiview commented Nov 6, 2018

Yes, thanks.
Do you know if these check will fail, if you revert the changes you did to MariaDBContainer?

@guss77
Copy link
Contributor Author

guss77 commented Nov 6, 2018

Interestingly enough, without the MariaDbContainer support for user and password, the only test that fails is jdbc:tc:mariadb://hostname/databasename?user=someuser&TC_INITSCRIPT=somepath/init_mariadb.sql on an assert at JDBCDriverTest.java:140 which tests that the "current user" is someuser (hardcoded?).

This actually makes sense to me - this PR is about obeying the configuration from the URL, so without it the container still starts, but uses the "test" username so the only way to check that is to check if the current logged in used matches what we expect from the URL.

The only question is why the other tests with a user in the URL didn't fail, and I think I understand why.

@guss77
Copy link
Contributor Author

guss77 commented Nov 6, 2018

With the last change, all MariaDB tests with a custom user name fail without the PR changes.

@kiview
Copy link
Member

kiview commented Nov 6, 2018

Thanks, LGTM to me now.
But for JDBC PRs, it's always good if @rnorth gives it an additional look 🙂

@testcontainers testcontainers deleted a comment Nov 6, 2018
@rnorth
Copy link
Member

rnorth commented Nov 10, 2018

LGTM, thank you @guss77!

@rnorth rnorth merged commit e823ca2 into testcontainers:master Nov 10, 2018
@rnorth rnorth added this to the next milestone Nov 10, 2018
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.

None yet

3 participants