-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Support for Microsoft SQL Server [JDBC] #1426
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.
Nice work, glad to see this one tackled! I added 2 minor comments inline.
//Not strictly necessary when using Agroal, as it also registers | ||
//any JDBC driver being configured explicitly through its configuration. | ||
//We register it for the sake of people not using Agroal. | ||
final String driverName = "com.microsoft.sqlserver.jdbc.SQLServerDriver"; |
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.
Can't you use the class itself to get the name? That would seem more future proof.
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.
Tradeoff choices.
it's not like they can change the driver name without breaking a million apps either.. I prefer to not initialize the class.
<groupId>com.microsoft.azure</groupId> | ||
<artifactId>adal4j</artifactId> | ||
<version>1.6.3</version> | ||
<exclusions> |
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.
If you want to exclude all its dependencies, you can use:
<groupId>*</groupId>
<artifactId>*</artifactId>
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.
unfortunately it wasn't possible to exclude them all :(
I think this is ready to be used.
There are some follow up things to discuss, but I don't think these should block merging the feature: