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

Enhancements and Fixes around JDBC URL Based Containers #617

Merged
merged 4 commits into from
Jun 13, 2018

Conversation

manikmagar
Copy link
Contributor

@manikmagar manikmagar commented Mar 21, 2018

This PR implements #566.

[Originally implemented in PR #594 - Allow setting database name and user when using JDBC URL but that PR branch got messed up during rebasing with upstream, so this New PR replaces it. Check the discussion of #594 for more details. If this is merged, PR #594 must be closed without merging.]

  1. Extracts out JDBC Url manipulations to a separate class - ConnectionUrl.
  2. Adds an overloaded method JdbcDatabaseContainerProvider.newInstance(ConnectionUrl), with default implementation delegating to the existing newInstance(tag) method.
  3. Adds an implementation of newInstance(ConnectionUrl) in MySQLContainerProvider to use Database Name, user, and password from JDBC URL while creating new Container.
  4. Added/Updated Test cases for new functionality.
  5. Bug Fix: Query Parameters are not used when a connection is created from a cached container. (see this comment) Fixes db extra connection params are only passed to the first connection in a pool  #610
  6. Bug Fix: This also fixes Oracle Connection String is not accepted by JDBC Container #596 and now accepts JDBC Connection Strings for all supported Database Engines.

Note: Other Containers Providers such as PostgreSQL will remain unaffected and continue to use earlier implementation of newInstance(tag) due to default delegation, until another implementation is provided.

@rnorth
Copy link
Member

rnorth commented Apr 7, 2018

This had some merge conflicts, so I've resolved those. As a result, we now have a few more JDBC driver tests than we did previously.

@manikmagar
Copy link
Contributor Author

Thanks @rnorth for merging this with master. There are few test failures in jdbc-test module. I know why they are failing. I'll fix the failing tests in jdbc-test module.

@manikmagar
Copy link
Contributor Author

Ping :) ... Any further thoughts on this one?

@bsideup
Copy link
Member

bsideup commented Apr 26, 2018

@manikmagar FYI I've triggered a re-run of Travis / CircleCI

@manikmagar
Copy link
Contributor Author

Thanks @bsideup. I looked at the logs for failed tests and both looks to be environment related.

@manikmagar
Copy link
Contributor Author

manikmagar commented May 30, 2018

@bsideup @rnorth Any thoughts on this one, please? Looking to try Oracle JDBC Driver. (#596 ). Thanks.

@rnorth
Copy link
Member

rnorth commented May 30, 2018

Sorry @manikmagar - I'll scoop up all the SQL related tickets on an evening this week and crunch through them. Sorry for the long delay :(

@manikmagar
Copy link
Contributor Author

Thanks @rnorth, I appreciate your time and efforts!

@rnorth
Copy link
Member

rnorth commented Jun 10, 2018

I've rebased onto master - this included updating to be compatible with our change to use a default tag, rather than latest, whenever possible. This had some impact on JdbcDatabaseContainerProvider and the JDBC tests, but all is working OK now.

As I've had ample opportunity to read and tweak during the rebasing effort, this LGTM :)

Sorry for taking an eternity to complete this.

@manikmagar
Copy link
Contributor Author

manikmagar commented Jun 10, 2018 via email

@rnorth rnorth merged commit dbb9267 into testcontainers:master Jun 13, 2018
@bsideup bsideup added this to the next milestone Jun 13, 2018
@rnorth
Copy link
Member

rnorth commented Jun 13, 2018

Thanks @manikmagar - merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants