-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Refactor Relational Source and add MySQL Support #146
base: main
Are you sure you want to change the base?
Conversation
8036b8e
to
4451758
Compare
public static void main(String[] args) { | ||
|
||
log.info("Starting SQL Proxy for {}", SourceProxy.SourceId()); |
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.
Bring it back.
Reactivator.TerminalError(new IllegalArgumentException("Database host is required.")); | ||
|
||
var dbPassword = Reactivator.GetConfigValue("password"); | ||
if (dbPassword == null || dbPassword.isEmpty()) |
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.
Daniel: Make it optional
if (dbPort == null || dbPort.isEmpty()) | ||
Reactivator.TerminalError(new IllegalArgumentException("Database port is required.")); | ||
|
||
var dbUser = Reactivator.GetConfigValue("user"); |
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.
Daniel: Make it optional
// Port of the database server. | ||
.with("database.port", dbPort) | ||
// Username to be used when connecting to the database server. | ||
.with("database.user", dbUser) |
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.
Daniel: Can we add trustServerCertificate
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.
trustServerCertificate is specific to the SQL Server JDBC driver
config_schema: | ||
type: object | ||
properties: | ||
connector: |
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 we should add an enum field for this property that contains a list of valid connectors, that way the error will be caught when we do drasi apply
with an invalid connector
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.
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 one is kind of hard coded per source type. We aren't documenting it and we don't want to user to actually change 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.
Makes sense. Thanks
Description
Type of change
Fixes: #145
Refactoring from the existing architecture:
To this for easier extensibility: