Skip to content

sqlserver jdbc url should be supported by container provider - #989

Merged
kiview merged 4 commits into
testcontainers:masterfrom
neetkee:fix/mssql-naming
Jan 27, 2019
Merged

sqlserver jdbc url should be supported by container provider#989
kiview merged 4 commits into
testcontainers:masterfrom
neetkee:fix/mssql-naming

Conversation

@neetkee

@neetkee neetkee commented Nov 23, 2018

Copy link
Copy Markdown
Contributor

Solution to #988

kiview
kiview requested changes Nov 26, 2018
View reviewed changes

@kiview kiview left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey, thanks for the PR.

So using a "wrong" JDBC url like jdbc:tc:mssqlserver:2017-CU12://somehost:someport;databaseName=test also never worked?

Comment thread modules/mssqlserver/src/test/java/org/testcontainers/containers/SqlServerJDBCDriverTest.java Outdated
Comment thread modules/mssqlserver/src/test/resources/container-license-acceptance.txt Outdated
Comment thread modules/mssqlserver/build.gradle Outdated
@neetkee

neetkee commented Nov 26, 2018

Copy link
Copy Markdown
Contributor Author

It's possible to use JDBC url with mssqlserver, mentioned it here

@kiview

kiview commented Nov 26, 2018

Copy link
Copy Markdown
Member

@StefanHufschmidt Can you give it a look please?

@StefanHufschmidt

StefanHufschmidt commented Nov 26, 2018

Copy link
Copy Markdown
Contributor

LGTM

@StefanHufschmidt

StefanHufschmidt commented Nov 26, 2018

Copy link
Copy Markdown
Contributor

Btw do we automatically accept the license agreement on this ms sql server image in our tests?
Might that cause some problems for us?

@testcontainers testcontainers deleted a comment Nov 26, 2018
@testcontainers testcontainers deleted a comment Nov 26, 2018
@testcontainers testcontainers deleted a comment from neetkee Nov 26, 2018
@kiview

kiview commented Nov 27, 2018

Copy link
Copy Markdown
Member

@StefanHufschmidt You mean legal problems?
We accept it in the jdbc-test module.

@kiview

kiview commented Nov 27, 2018

Copy link
Copy Markdown
Member

@neetkee This will be a breaking API change, correct?
Would it make sense, to support both names?

@neetkee

neetkee commented Nov 27, 2018

Copy link
Copy Markdown
Contributor Author

@kiview Yes, it will be the breaking change for those, who use this image as temporary database container. But I'm not sure if we need to keep mssqlserver jdbc url working, documentation says that to work with it, it's only needed to Insert tc: after jdbc:

@kiview

kiview commented Nov 27, 2018

Copy link
Copy Markdown
Member

The failing tests in CI aren't a direct error in this PR, but are coming from #992.
They only appear in this PR, since we didn't have MS SQL tests before it seems?

kiview
kiview requested changes Nov 27, 2018
View reviewed changes
Comment thread modules/jdbc-test/src/test/java/org/testcontainers/jdbc/JDBCDriverTest.java Outdated
@kiview @neetkee
Update modules/jdbc-test/src/test/java/org/testcontainers/jdbc/JDBCDr…
d750fbf 
…iverTest.java

Co-Authored-By: neetkee <neetkee@users.noreply.github.com>
@neetkee

neetkee commented Jan 15, 2019

Copy link
Copy Markdown
Contributor Author

Is there anything I need to do?

@bsideup

bsideup commented Jan 15, 2019

Copy link
Copy Markdown
Member

Pinging @kiview

@kiview kiview added the type/breaking-api-change Public APIs are being changed label Jan 27, 2019
@kiview

kiview commented Jan 27, 2019

Copy link
Copy Markdown
Member

@neetkee Whoops sorry, everything looks good.
Since this is kind of API breaking, we should mention it in the release notes, but it's the right thing to do.

Thanks for your contribution!

@kiview
kiview merged commit e6e3331 into testcontainers:master Jan 27, 2019
@kiview kiview added this to the next milestone Jan 27, 2019
@kiview kiview mentioned this pull request Jan 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kiview kiview kiview approved these changes
@bsideup bsideup Awaiting requested review from bsideup
@rnorth rnorth Awaiting requested review from rnorth

Assignees

No one assigned

Labels

modules/jdbc type/breaking-api-change Public APIs are being changed

Projects

None yet

Milestone

1.10.6

Development

Successfully merging this pull request may close these issues.

4 participants

@neetkee @kiview @StefanHufschmidt @bsideup