-
Notifications
You must be signed in to change notification settings - Fork 292
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 special casing with Fabric endpoints #3084
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3084 +/- ##
==========================================
+ Coverage 72.70% 72.88% +0.18%
==========================================
Files 283 283
Lines 58975 58978 +3
==========================================
+ Hits 42875 42984 +109
+ Misses 16100 15994 -106
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Only one comment on the code style, but the checks themselves are correct.
I think there are a handful of feature differences between Azure SQL and Fabric SQL which merit a test suite and documentation updates though. At a glance, the driver would be impacted by the lack of support for Always Encrypted and the BULK INSERT statement.
} | ||
|
||
internal static readonly string[] s_azureSqlServerEndpoints = { StringsHelper.GetString(Strings.AZURESQL_GenericEndpoint), | ||
StringsHelper.GetString(Strings.AZURESQL_GermanEndpoint), | ||
StringsHelper.GetString(Strings.AZURESQL_UsGovEndpoint), | ||
StringsHelper.GetString(Strings.AZURESQL_ChinaEndpoint)}; | ||
StringsHelper.GetString(Strings.AZURESQL_ChinaEndpoint), | ||
".database.fabric.microsoft.com"}; |
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.
Perhaps this should be a resource string?
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.
Yeah, wonder why the other ones are resource strings, I guess they are not localized ?
@edwardneal There are a number of feature differences between the many engine variations this driver supports or happen to work with - is it really the responsibility of the driver to document this, or is the page you are linking to not enough? |
I think the page I linked to would cover most cases, I'm thinking primarily about a once-over to make sure that the SqlClient documentation doesn't contradict it and is clear enough for people to understand what functionality the feature depends upon. The example I had in mind was |
#2995 refers to Fabric warehouse, which is something completely different from Fabric SQL Database |
Sorry, slightly delayed here! You're right, but the root cause is similar: some of SqlClient's functionality can't be supported on a particular engine variation, because that functionality relies upon a feature the engine doesn't support (in this case, SqlBulkCopy relies upon the BULK INSERT statement, which neither Fabric Warehouse nor Fabric SQL Database support.) My broader point is that we should make sure that the SqlClient documentation for functionality which depends on BULK INSERT or Always Encrypted doesn't claim to be compatible across all engine variations, and that specifically for SqlBulkCopy we should make it clear that the feature depends upon server-side BULK INSERT support (since that's hard to discover in the documentation, and without that insight it's hard to know what won't support the feature.) If you want to keep this PR scoped specifically to the endpoint detection, I'm happy enough to check that documentation in a separate PR. |
Sounds to me like a separate issue and PR would be suitable to prevent scope creep. |
Support the special casing for Fabric endpoints