-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Source MSSQL: Enabled SSL connections #3893
Conversation
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.
implementation looks good but some blocking comments on tests. Feel free to ship once they are addressed
import java.util.HashMap; | ||
import java.util.List; | ||
|
||
public class MssqlRdsSourceAcceptanceTest extends SourceAcceptanceTest { |
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.
instead of rewriting the entire class, why don't we extend the other acceptance test and make the db configuration overridable? This way we have complete DRY, just change which DB to connect to
|
||
@Override | ||
protected void setup(TestDestinationEnv testEnv) throws Exception { | ||
db = new MSSQLServerContainer<>(DockerImageName.parse("airbyte/mssql_ssltest:dev").asCompatibleSubstituteFor("mcr.microsoft.com/mssql/server")) |
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 do we need a docker image if we are testing against RDS?
…ommon mssql acceptance test
/test connector=source-mssql
|
/publish connector=connectors/source-mssql
|
What
Ticket: #3618 : Source MSSQL: Enable SSL connections
How
Exposed the configuration in the connector's spec.json which allows a user to choose one of the three configurations:
Constructed the correct JDBC URL based on the configurations input by the user.
Added Acceptance tests to run against the Amazon RDS instance (created in scope of #3619)
Pre-merge Checklist
Recommended reading order
test.java
component.ts