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

#5032 - MS SQL Server disable encrypt by default #5033

Conversation

StefanHufschmidt
Copy link
Contributor

@StefanHufschmidt StefanHufschmidt commented Feb 8, 2022

This PR disables the JDBC driver encrypt option by default for MS SQL Server containers. Since release 10.1.0 of MS JDBC driver the encrypt option is enabled by default (see https://github.com/microsoft/mssql-jdbc/releases/tag/v10.1.0). To be able to use the container without having to manually add params the encrypt option will be switched off if it is not set.

Closes #5032

This commit disables the JDBC driver encrypt option by default for MS SQL Server containers. Since release 10.1.0 of MS JDBC driver the encrypt option is enabled by default (see https://github.com/microsoft/mssql-jdbc/releases/tag/v10.1.0). To be able to use the container without having to manually add params the encrypt option will be switched off if it is not set.

Signed-off-by: Decker, Stefan <Stefan.Decker@gdata.de>
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.

Implementation LGTM, thanks a lot @StefanHufschmidt!
See my minor comments regarding the test.

…anation

Signed-off-by: Decker, Stefan <Stefan.Decker@gdata.de>
Signed-off-by: Decker, Stefan <Stefan.Decker@gdata.de>
Signed-off-by: Decker, Stefan <Stefan.Decker@gdata.de>
@StefanHufschmidt
Copy link
Contributor Author

Thanks for your suggestions @kiview. I think I've applied them accordingly. Would you mind having another look?

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.

Thanks, all good from my side 👍

@kiview
Copy link
Member

kiview commented Feb 8, 2022

After further thought and discussion with the other maintainers, it seems with the currently used dependencies, the new test itself is not really meaningful and just asserts an implementation detail.

WDYT of deleting the new test altogether, but instead bumping the version of mssql-jdbc we use internally for testing to a version, that incorporates this change (> 10.1.0)? Without the change, all existing tests would fail in this case.

…2.0.jre8

This makes specific testing for "encrypt" url param obsolete since it is required to apply for successful tests at all.

Signed-off-by: Decker, Stefan <Stefan.Decker@gdata.de>
@StefanHufschmidt
Copy link
Contributor Author

After further thought and discussion with the other maintainers, it seems with the currently used dependencies, the new test itself is not really meaningful and just asserts an implementation detail.

WDYT of deleting the new test altogether, but instead bumping the version of mssql-jdbc we use internally for testing to a version, that incorporates this change (> 10.1.0)? Without the change, all existing tests would fail in this case.

Totally agree. I've applied the changes necessary for a version bump and removed the now obsolete test case.


testImplementation project(':r2dbc')
testImplementation 'io.r2dbc:r2dbc-mssql:0.8.7.RELEASE'

// MSSQL's wait strategy requires the JDBC driver
testImplementation testFixtures(project(':r2dbc'))
testImplementation 'com.microsoft.sqlserver:mssql-jdbc:9.5.0.jre8-preview'
testImplementation 'com.microsoft.sqlserver:mssql-jdbc:10.2.0.jre8'
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something, but why do we have this duplicate line? (we also had it before)
Seems it what introduced in #2556, maybe just a copy-paste error @rnorth?

@kiview kiview merged commit affee6c into testcontainers:master Feb 10, 2022
@kiview kiview added this to the next milestone Feb 10, 2022
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.

Encryption for MSSQLServerContainer by default
3 participants