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

sqlserver jdbc url should be supported by container provider #989

Merged
merged 4 commits into from
Jan 27, 2019
Merged

sqlserver jdbc url should be supported by container provider #989

merged 4 commits into from
Jan 27, 2019

Conversation

neetkee
Copy link
Contributor

@neetkee neetkee commented Nov 23, 2018

Solution to #988

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.

Hey, thanks for the PR.

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

@neetkee
Copy link
Contributor Author

neetkee commented Nov 26, 2018

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

@kiview
Copy link
Member

kiview commented Nov 26, 2018

@StefanHufschmidt Can you give it a look please?

@StefanHufschmidt
Copy link
Contributor

LGTM

@StefanHufschmidt
Copy link
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
Copy link
Member

kiview commented Nov 27, 2018

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

@kiview
Copy link
Member

kiview commented Nov 27, 2018

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

@neetkee
Copy link
Contributor Author

neetkee commented Nov 27, 2018

@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
Copy link
Member

kiview commented Nov 27, 2018

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?

…iverTest.java

Co-Authored-By: neetkee <neetkee@users.noreply.github.com>
@neetkee
Copy link
Contributor Author

neetkee commented Jan 15, 2019

Is there anything I need to do?

@bsideup
Copy link
Member

bsideup commented Jan 15, 2019

Pinging @kiview

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

kiview commented Jan 27, 2019

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules/jdbc type/breaking-api-change Public APIs are being changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants