Skip to content

WIP | Create Extensions package #3471

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

Closed
wants to merge 1 commit into from

Conversation

paulmedynski
Copy link
Contributor

Description

Work in Progress - This PR is a draft while I get the CI machinery working.

As part of the Azure split work, a new Extensions package is being created that contains types shared between MDS and its future extensions (Azure will be the first). This PR creates that package (with no meaningful content) to setup the MDS dependency, and get testing and CI setup accordingly.

I'm also experimenting with simplified .slnx files and some project directory structure changes.

Issues

The first step towards #1108.

Testing

  • Unit tests for the sample class to prove that CI can run them successfully.
  • There are no MDS tests to prove out the new dependency yet. Those will naturally happen when the first real bit of Azure functionality is moved out into the new Azure extension package.

Copy link
Contributor Author

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added commentary for reviewers and found some things to fix.

@@ -327,9 +349,8 @@ stages:
# ways to detect if they were present) won't run consistently across forks and non-forks.
${{ if eq(variables['system.pullRequest.isFork'], 'False') }}:
AADPasswordConnectionString: $(AAD_PASSWORD_CONN_STR)
AADServicePrincipalId: $(AADServicePrincipalId)
${{ if eq(variables['system.pullRequest.isFork'], 'False') }}:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My YAML linter was complaining about duplicate keys because these ${{if ...} lines are the same. Similar changes below for the enclave test configurations.

used by the build tooling and may be unintentionally included in other
(non-MDS) projects.
-->
<NugetPackageVersion Condition="'$(NugetPackageVersion)' == ''">$(MdsVersionDefault).$(BuildNumber)-dev</NugetPackageVersion>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor tweak here - I added $(BuildNumber) - not sure if that's helpful, but it matches how the Extensions package does things.

@paulmedynski paulmedynski force-pushed the dev/paul/azure-split/extensions branch from ba927b7 to 6a52ede Compare July 11, 2025 19:09
Copy link

codecov bot commented Jul 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.82%. Comparing base (55cc333) to head (e912975).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3471       +/-   ##
===========================================
+ Coverage   59.44%   90.82%   +31.37%     
===========================================
  Files         268        6      -262     
  Lines       62160      316    -61844     
===========================================
- Hits        36950      287    -36663     
+ Misses      25210       29    -25181     
Flag Coverage Δ
addons 90.82% <ø> (?)
netcore ?
netfx ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@paulmedynski paulmedynski force-pushed the dev/paul/azure-split/extensions branch 3 times, most recently from 0364a29 to 9192601 Compare July 22, 2025 16:02
@paulmedynski paulmedynski force-pushed the dev/paul/azure-split/extensions branch from 9192601 to 54d6245 Compare July 23, 2025 14:06
Copy link
Contributor Author

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding commentary for reviewers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to give .slnx format a try, and so far the tooling supports it.

@edwardneal
Copy link
Contributor

Thanks for this - I've got no comments or feedback on the design, other than the cosmetic note that Microsoft.Extensions.Logging and Microsoft.Extensions.DependencyInjection rely upon packages with the ".Abstractions" suffix, rather than ".Extensions".

Interestingly, there's also a sizable improvement in AOT application size as a result of removing the Azure components from the main package. In my sample application, doing so cut the size of a 13.2MB application by about a third.

@paulmedynski
Copy link
Contributor Author

Thanks for this - I've got no comments or feedback on the design, other than the cosmetic note that Microsoft.Extensions.Logging and Microsoft.Extensions.DependencyInjection rely upon packages with the ".Abstractions" suffix, rather than ".Extensions".

I like the Abstractions name better than just Extensions. I'll discuss with my colleagues after I get back from vacation before I do a huge renaming :) I'm thinking:

Package Description
Microsoft.Data.SqlClient The main MDS package.
Microsoft.Data.SqlClient.Abstractions Shared types, classes, etc; used by the other packages.
Microsoft.Data.SqlClient.Extensions.Azure Functionality that relies on Azure dependencies.

Is there any benefit in Microsoft.DatalSqlClient.Extensions.Abstractions versus the above?

Interestingly, there's also a sizable improvement in AOT application size as a result of removing the Azure components from the main package. In my sample application, doing so cut the size of a 13.2MB application by about a third.

Excellent - more tangible benefits!

@Frulfump
Copy link

Maybe this issue would cause changes you would want to incorporate here?
#3509

- Added empty Extensions package with some sample class and docs to demonstrate packaging.
- Created CI stage to build, test, pack, and publish the Extensions NuGet package.
- Updated downstream CI stages/jobs to use the Extensions package.
- Updated build.proj Clean target to not delete packages/ dir.
- Updated BUILDGUIDE with instructions for the Extensions package.
- Cleaned up stale BUIDGUIDE sections.
- Added temporary GitHub Discussion content so the team can review before posting it as a real Discussion.
- Disable .pdb file inclusion in the application package.
@paulmedynski paulmedynski force-pushed the dev/paul/azure-split/extensions branch from c854293 to e912975 Compare August 8, 2025 12:26
@paulmedynski
Copy link
Contributor Author

Closing in favour of #3567 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants