-
Notifications
You must be signed in to change notification settings - Fork 315
Create Abstractions package #3567
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
Conversation
e912975
to
261f4af
Compare
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.
Adding comentary for reviewers.
I will be renaming the Extensions package to Abstractions. That round of changes is next.
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.
Commentary for reviewers, and issues for myself to fix.
eng/pipelines/dotnet-sqlclient-ci-package-reference-pipeline.yml
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/azure-split #3567 +/- ##
====================================================
- Coverage 70.67% 69.96% -0.71%
====================================================
Files 277 276 -1
Lines 61686 61641 -45
====================================================
- Hits 43598 43130 -468
- Misses 18088 18511 +423
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:
|
c2a8e97
to
d4464ec
Compare
- 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. - Renamed Extensions package to Abstractions. - Updated README related to extensions design.
d4464ec
to
f6bbbaf
Compare
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'm omitting all my comments about directory structure as per offline discussion. But I think we want to be really careful and thoughtful with how we set up pipelines, build targets, and project structure.
- Addressed some of the review comments.
- Fixed missing Abstractions package version.
* Task 38530: Prepare release notes - Added 7.0.0 Preview 1 release notes. * Task 38530: Prepare release notes - Addressed review comments. * Task 38530: Prepare release notes - Filled in actual build number.
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.
Awaiting the rest of the changes, but we're definitely making progress
* netcore, netfx: sync declarations of TdsOperationStatus variables * netcore: reorder TryProcessUDTMetadata * netfx: reorder WriteTceFeatureRequest * netfx: reorder WriteAzureSQLSupportFeatureRequest * netcore, netfx: merge WriteInt and SerializeInt netcore: removed a simple WriteInt which did nothing besides call BinaryPrimitives. netcore: when there's space in the packet buffer, WriteInt will now write to it directly (rather than write to a stack-allocated span and copy.) * netcore: remove ConstructGuid method * netcore, netfx: refactor and sync serialization of guids * netcore, netfx: refactor and sync serialization of floats and doubles This removes the need for TdsParser.netcore.cs. * netfx: adjust TraceString - parameter was being passed to it which was never referenced in the format string * netcore: sync exception messages with netfx Messages of the netfx exceptions are more detailed * netfx, netcore: centralise masking of received server options Added mask to netcore logic. Also removed mask on _encryptionOption from netfx - this will never be outside the range of EncryptionOptions.OPTIONS_MASK. * netcore: enforce server certificate validation if AccessTokenCallback is set and certificate is not automatically trusted * netfx: add static lambda to TdsExecuteSQLBatch * netfx: remove failed attempt at static lambda This passed state around, but never passed enough state to remove the state machine. Sync with netcore. * netfx: sync reference to length of JSON metadata substitution sequence * netfx: pre-PR correction: match previous behaviour when writing Guid instances from WriteUnterminatedSqlValue * Flip order of debug assertion * Next round of code review Remove unnecessary conditional compilation. Add XML documentation to clarify SerializeShort, WriteShort et al. * netcore, netfx: sync TraceString Both platforms will now show the TransparentNetworkIPResolution status. This is always disabled on netcore. * Expand ParametersTest to include SqlGuid (including null value) * netfx: port RequestContinue call from netcore
) Co-authored-by: Shreya Rao <[email protected]>
- Changed MDS to reference Abstractions via project or package depending on build parameters. - Removed obsolete package reference files. - Updated CI pipelines to build Abstractions in parallel for Project-based builds.
- Modularized Abstractions CI templates.
- Cleaned up the project vs package pathways.
* Merge: * _parameters * _pendingCancel * _preparedConnectionCloseCount * _preparedConnectionReconnectCount * _prepareHandle * s_cachedInvalidPrepareHandle * Statistics * Merge Connection, DbConnection * Merging RetryLogicProvider and _retryLogicProvider # Conflicts: # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs * Merge Transaction, DbTransaction * Merge CommandText * Replace "" with string.Empty * Merge CommandTimeout, _commandTimeout, and ADP.InvalidCommandTimeout * Merge CommandType * Removed temp storage of _commandType * Merge ColumnEncryptionSetting, _columnEncryptionSetting * Merge Parameters and DbParameterCollection * Merging DesignTimeVisible, _designTimeVisible * Merging EnableOptimizedParameterBinding, UpdatedRowSource, _updatedRowSource * Merge StatementCompleted and _statementCompletedEventHandler * Merging Notification, NotificationAutoEnlist, _notification, _notificationAutoEnlist, _sqlDep * Merge DefaultCommandTimeout, InternalTdsConnection, IsColumnEncryptionEnabled, InternalRecordsAffected, _rowsAffected, PropertyChanging() * merge _transaction and _batchRPCMode * Restore ResetCommandTimeout that got accidentally obliterated in a previous commit * Fix logic issue in IsColumnEncryptionEnabled, reinstate TypeDescriptor.Refresh on netfx
- Replaced generic "NugetPackageVersion" with variables names specific to the package (i.e. MdsPackageVersion, AkvPackageVersion, etc). - Removed top-level Abstractions package version parameters in favour of a variable.
- Fixes to Abstractions version number construction. - Fixes for some build.proj targets.
- Fixes for Abstractions and AKV default versioning.
- Added missing CI variable $(abstractionsPackageVersion)
- Fixed missing parameter when packing Abstractions.
* Expand code coverage for UDT serialization * Address test failures on 32-bit OSes * Make Microsoft.SqlServer.Server a PackageReference * Reformat SerializedTypes.cs * Switch to implicit object creation Also enforce this via editorconfig * Add and correct comments Also mandating this by generating a documentation XML file * Enable nullable for project * Declare and assign variables together * Moved MemoryStream to an instance variable * Post-merge build corrections * Reapply IDE0090 recommendations, disable CS1591 on newly-added tests * Next code review (first response) * Switch to using TheoryData * Nit: => movement * Rename tests * Split delegate for Assert.Throws into separate arrange/act/assert segment of each method * Disable CS1591 on new tests * Disable mandatory XML documentation * Change formatting of test data * Remove trailing XML documentation override
- Removed obsolete ADO Library inclusion in CI variables. - Moved MDS CI versions numbers into CI variables. - Fixed Abstractions versioning to handle CI version format.
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.
Adding commentary for reviewers, and some items for me to address.
variables: | ||
- template: ../../../libraries/variables.yml@self | ||
- ${{ if parameters.isPreview }}: | ||
- name: NugetPackageVersion |
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 found NugetPackageVersion confusing - which nuget package? So I gave them names specific to the package. I didn't rename any of these yml files to match such changes though. I figure that's more trouble than it's worth, and we intend to re-factor the whole works here soon anyway.
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 to me. I appreciate that kind of verbosity, and it was something I aimed to resolve in the AKV provider pipeline. Needs to be spread to other pipelines :D
Get-Content -Path "NuGet.config" | ||
displayName: 'Read NuGet.config [debug]' | ||
- task: DotNetCoreCLI@2 |
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.
Not sure why this was here, so I removed it.
arguments: 'build.proj -t:restore' | ||
feedsToUse: 'select' | ||
|
||
- powershell: | |
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.
AKV .csproj already has conditional references depending on Project vs Package, so this wasn't necessary.
parameters: | ||
SNIVersion: ${{parameters.SNIVersion}} | ||
SNIValidationFeed: ${{parameters.SNIValidationFeed}} | ||
- template: common/templates/steps/ci-prebuild-step.yml@self |
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 and the else block are identical, so I pulled them out.
src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Abstractions.csproj
Outdated
Show resolved
Hide resolved
- Addressed my own review comments.
- Fixed parameter typo in YAML.
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, I think this is a good improvement over the OG PR. We'll mess with it some more as we get time for it 👍
- Fixed Abstractions package versioning in official builds. - Fixed broken assembly version checks in official builds. - Removed Abstractions solution file.
- Fixing invalid package/assembly versions due to build numbers containing dots.
- Fixed official build assembly versioning.
- Fixed incorrect variable dereference syntax in expression.
- Fixed assembly versions > 65534.
- Improving diagnostics for assembly version checks.
- Trying to fix PowerShell variable expansion and assembly version checks.
- Diagnostics to figure out why MDS assembly file versions aren't being set.
- 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. - Renamed Extensions package to Abstractions. - Updated README related to extensions design. - Changed MDS to reference Abstractions via project or package depending on build parameters. - Removed obsolete package reference files. - Updated CI pipelines to build Abstractions in parallel for Project-based builds. - Modularized Abstractions CI templates. - Cleaned up the project vs package pathways. - Replaced generic "NugetPackageVersion" with variables names specific to the package (i.e. MdsPackageVersion, AkvPackageVersion, etc). - Removed top-level Abstractions package version parameters in favour of a variable. - Fixes to Abstractions version number construction. - Fixes for some build.proj targets.
…/SqlClient into dev/paul/azure-split/abstractions
- Fixed duplicate parameters from git merge.
Abandoned in favour of #3626 and #3628.
Overview
As part of the Azure split work, a new Abstractions 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.
This supercedes the original draft PR #3471.
Issues
The first step towards #1108.
Testing