-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Added additional url params in JdbcDatabaseContainer (#1802) #1874
Added additional url params in JdbcDatabaseContainer (#1802) #1874
Conversation
Added the ability to add additional parameters to the URL (https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-configuration-properties.html)
Thanks @eaxdev! I'm curious about a couple of things. The connection string that you're modifying is usually only used during Testcontainers initial readiness checks for the container and running of init scripts. The expectated usage of the current API is that your normal code would call So firstly, what is the use case that means that the current API is not sufficient? I'm afraid I'm not quite clear on this yet. Secondly, if we do this, is there a specific reason not to support it for other JDBC container types? It seems that it would almost be easier to support it all-round! |
Hi, @rnorth!
|
Some comments from my side:
|
Thanks for your input @knutwannheden - I agree with your points. @eaxdev, would you be OK to follow up with changes? |
Hi, @rnorth! Yes, I'm ready to follow up with changes. But still I want to clarify the final requirements:
Did I understand the problem correctly? Thanks! |
… constructUrlParameters() (#1802)
Hi, @rnorth! I added support it for other JDBC container types. See, please. Thank! |
|
||
public SELF withUrlParam(String paramName, String paramValue) { | ||
throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't the implementation of this method be pulled up here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good idea. I will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done. I did it.
We seem to have some test failures around the MS SQL Server container - any ideas? |
@rnorth , Yes, Yes. I already got it. I'm already fixing... |
@rnorth, I fixed it. Now all tests pass |
@@ -46,6 +46,8 @@ public PostgreSQLContainer(final String dockerImageName) { | |||
@Override | |||
protected void configure() { | |||
addExposedPort(POSTGRESQL_PORT); | |||
// Disable Postgres driver use of java.util.logging to reduce noise at startup time | |||
withUrlParam("loggerLevel", "OFF"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't investigated what exactly this does, but it looks a bit weird. I can understand if by default the logging level should not be DEBUG or so, but I would definitely like to see errors and probably also warnings by default (i.e. without having to investigate how to change this manually).
Please disregard if I missed something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to be in the getJdbcUrl
method like this:
return "jdbc:postgresql://" + getContainerIpAddress() + ":" + getMappedPort(POSTGRESQL_PORT) + "/" + databaseName + "?loggerLevel=OFF";
I just moved it to the configure()
method.
I'm guessing the logger was turned off on purpose to remove the noise at the start of the container.
@@ -90,6 +90,11 @@ public OracleContainer withPassword(String password) { | |||
return self(); | |||
} | |||
|
|||
@Override | |||
public OracleContainer withUrlParam(String paramName, String paramValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since some DBs like Oracle don't support URL parameters, but do support Properties
parameters, I think it is worth considering renaming this method to withParam()
or withJdbcParam()
, so that client code doesn't have to call withUrlParameter()
for some DBs and something like withPropertiesParam()
for others (assuming that will be also supported in the future). I just think this would be worth considering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not entirely correct. What do you think about that, @rnorth ? Thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to take so long to come back to this PR to break the tie...
I think the current form is reasonable; Oracle does not support JDBC URL parameters (in the normal sense of a URL parameter, anyway). Also, we don't support setting connection properties for the Oracle driver.
It's good to think about the future case of being agnostic of whether the connection params are set in the URL or as properties. However, I think that:
- the params themselves are so database-specific that it's rarely going to be possible to write them generically, and
- trying to support both URL params and properties via one method is going to tie us in knots if we ever actually support pass-through of properties - we're not going to know which params go where.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Let's tackle that problem with connection properties separately. From my POV this commit looks good to go!
Any status update on this PR? Would be nice if it could get integrated in some form. |
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. |
@rnorth WDYT on how to proceed here? You were the most involved one. |
@rnorth Which status of this PR? Is something wrong with him? |
Eugh, sorry I let this slip through the cracks. I'll check out the merge conflicts and resolve. |
Added the ability to add additional parameters to the URL in JDBC containers