-
Notifications
You must be signed in to change notification settings - Fork 321
[Draft] Azure Split - Step 3 - Tie Everything Together #3908
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
base: dev/paul/azure/step2
Are you sure you want to change the base?
Conversation
…ogether - Brought over all of the remaining code and project changes.
…ogether - Brought over the pipeline changes. - Removed unnecessary flexibility from the MDS official pipeline tree.
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.
Pull request overview
This pull request is the third step in the Azure Split work, which removes Azure authentication implementation from the main SqlClient project and establishes dependencies on the new Abstractions package. The PR removes Azure-specific authentication code, updates project dependencies, adds test authentication providers, and updates CI/CD pipelines to handle the new package structure.
Changes:
- Removes
ActiveDirectoryAuthenticationProvider,SqlAuthenticationProvider,SqlAuthenticationToken,SqlAuthenticationParameters, and related Azure authentication classes from the MDS source - Adds dependency on
Microsoft.Data.SqlClient.Extensions.Abstractionspackage across all MDS projects (src and ref) - Updates test infrastructure with
UsernamePasswordProviderandManagedIdentityProviderimplementations for manual tests - Updates all pipeline YAML files to include
AbstractionsPackageVersionparameter and propagate it through build and test steps
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/specs/Microsoft.Data.SqlClient.nuspec | Adds Abstractions package dependency to all target frameworks |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlAuthenticationProviderManager.cs | Implements dynamic loading of Azure extension package as default provider |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs | Refactors token acquisition retry logic to use new exception model |
| src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/*.cs | Adds test authentication providers |
| src/Microsoft.Data.SqlClient/**/Microsoft.Data.SqlClient.csproj | Removes Azure.* package references, adds Abstractions reference |
| eng/pipelines/**/*.yml | Adds AbstractionsPackageVersion parameter throughout |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlAuthenticationProviderManager.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/DataCommon/UsernamePasswordProvider.cs
Show resolved
Hide resolved
…ogether - Removed Azure parameters from CI test tree.
…ogether - Fixed missing Abstractions and MDS package version parameters.
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.
Pull request overview
Copilot reviewed 45 out of 45 changed files in this pull request and generated no new comments.
| int retryPeriod = | ||
| ex.RetryPeriod > 0 | ||
| ? ex.RetryPeriod | ||
| : defaultRetryPeriod * (2 ^ attempt); |
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.
| : defaultRetryPeriod * (2 ^ attempt); | |
| : defaultRetryPeriod * (1 << attempt); |
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 I'll just use a linear backoff. We only perform max 2 attempts anyway, so there's no effective difference.
Good catch on the unintended XOR though. Surprised Copilot didn't at least comment on it.
| var activeDirectoryAuthProvider = new ActiveDirectoryAuthenticationProvider(instance._applicationClientId); | ||
| instance.SetProvider(SqlAuthenticationMethod.ActiveDirectoryIntegrated, activeDirectoryAuthProvider); | ||
| // Try to load our Azure extension. | ||
| var assembly = Assembly.Load(assemblyName); |
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.
To back up your TODO, this'll need to be loaded with a strong name, otherwise a client which doesn't use Azure connectivity will load any assembly at [application base]/Microsoft.Data.SqlClient.Extensions.Azure/Microsoft.Data.SqlClient.Extensions.Azure.dll and pass this any Azure credentials.
With the assembly signature verified by including the public key token in the assembly name, maybe we could simplify the load process and directly use Type.GetType with a fully-qualified assembly name.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/paul/azure/step2 #3908 +/- ##
========================================================
- Coverage 69.92% 69.82% -0.11%
========================================================
Files 263 260 -3
Lines 66190 65691 -499
========================================================
- Hits 46285 45868 -417
+ Misses 19905 19823 -82
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ogether - Re-activate the Azure tests. - Address PR comments.
Description
This PR ties everything together:
PR series:
Testing