-
Notifications
You must be signed in to change notification settings - Fork 315
Abstractions Package - Pipeline Changes #3628
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: feat/azure-split
Are you sure you want to change the base?
Abstractions Package - Pipeline Changes #3628
Conversation
96b8487
to
383a354
Compare
b02a214
to
49ec317
Compare
/azp run |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/azure-split #3628 +/- ##
===================================================
Coverage ? 90.82%
===================================================
Files ? 6
Lines ? 316
Branches ? 0
===================================================
Hits ? 287
Misses ? 29
Partials ? 0
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:
|
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 some actions for myself.
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.
The identifier NugetPackageVersion
is ambiguous, so I replaced it with a name specific to the NuGet package (i.e. Abstractions, MDS, SqlServer, AKV, etc).
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 also eschewed using $(variables)
deep in nested templates, since it's difficult to know where they are defined and/or set. I added template parameters to replace a lot of them. Some deep $(variable)
use remains though.
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.
Thank you! The variable use at such a deep nesting level was one of my biggest complaints about how the pipelines were written before.
configuration: ${{ parameters.configuration }} | ||
operatingSystem: Windows | ||
build: all | ||
abstractionsPackageVersion: ${{parameters.abstractionsPackageVersion}} |
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 didn't bother making these Abstractions package version parameters conditional on Project vs Package reference type. IMO those two CI pipelines are almost different enough that sharing templates may be more trouble than it's worth. We will do better with the fancy new pipelines.
|
||
- template: ../steps/generate-nuget-package-step.yml@self | ||
parameters: | ||
NugetPackageVersion: $(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.
MDS and AKV package versions are now independent in CI builds.
|
||
- task: PublishBuildArtifacts@1 | ||
displayName: 'Publish Artifact: Artifacts' | ||
- task: PublishPipelineArtifact@1 |
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.
PublishPipelineArtifact
is the preferred way to publish things.
# See the LICENSE file in the project root for more information. # | ||
################################################################################# | ||
parameters: | ||
- name: abstractionsArtifactName |
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 tried to alphabetically order some template parameters, but decided it wasn't worth it given how many files I have touched. So you will see a few like this that have been re-ordered before I gave up.
- name: SQLTarget | ||
value: 'localhost' | ||
- name: NugetPackageVersion | ||
value: $(Major).$(Minor)$(Patch)-pull.1$(buildnumber) |
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 don't know why we were prepending 1
to the build number here.
- name: AssemblyFileVersion | ||
value: '$(Major).$(Minor)$(Patch).$(Build.BuildNumber)' | ||
- name: mdsAssemblyFileVersion | ||
value: $(Major).$(Minor).$(Patch).$(assemblyBuildNumber) |
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.
Note the tweak here. We used to concatenate the Minor and Patch numbers together because $(Build.BuildNumber)
already has a .
in it, and C# assembly file versions must not have more than 4 parts. Now we strip the second part (the attempt # ?) from the build number, since we can always find the correct build from just the main build number.
msbuildArguments: >- | ||
-t:BuildAkv | ||
-p:AssemblyFileVersion=${{ parameters.assemblyFileVersion }} | ||
-p:NugetPackageVersion=${{ parameters.mdsPackageVersion }} |
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.
@paulmedynski Verify that this change is correct - it may not be...
$(REPO_ROOT)/build.proj | ||
-t:BuildAkv | ||
-p:Configuration=${{ parameters.buildConfiguration }} | ||
-p:NugetPackageVersion=${{ parameters.mdsPackageVersion }} |
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.
@paulmedynski Same here - is this change correct?
# 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) |
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.
Re-arranged to avoid multiple identical if-statements.
/azp run |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
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.
targetPath: $(packagePath) | ||
# Note that packages/ will have been created by the above step, which is a | ||
# pre-requisite for configuring NuGet. | ||
- template: ../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.
It turns out that CI-SqlClient-Package runs weren't building the driver in Package mode. They were building the driver in Project mode, and then building the tests in Package mode. I fixed that to use Package mode everywhere. This helps find problems with Package mode before they hit dotnet-sqlclient-official. You will see referenceType
parameters added here and in downstream templates.
referenceType: ${{ parameters.referenceType }} | ||
operatingSystem: Windows | ||
build: all | ||
build: MDS |
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.
We now build/package MDS separately from AKV, since AKV needs an MDS package in orderr to build in Package mode.
parameters: | ||
analyzeType: all | ||
|
||
- template: ../steps/build-all-configurations-signed-dlls-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.
The analysis step clobbers the versioned DLLs from the build step, so I re-ordered those steps.
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.
@paulmedynski - Verify that the Roslyn analysis results aren't clobbered by the build.
Exit -1 | ||
} | ||
Get-ChildItem *.dll -Path $(extractedNugetPath) -Recurse | ForEach-Object VersionInfo | Format-List |
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 prints a detailed list of each DLL and its version information.
packageType: 'sdk' | ||
version: '9.0' | ||
|
||
- template: ./ensure-dotnet-version.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.
Nothing here uses .NET 8 runtime.
Get-ChildItem -recurse "$software\*.dll" | ||
Get-ChildItem -recurse "$symbols\*.pdb" | ||
Get-ChildItem -recurse "$software\*.dll" | ForEach-Object VersionInfo | Format-List |
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.
Full version information for all DLLs we build.
<RemoveDir Directories='$([System.IO.Directory]::GetDirectories(".", "bin", SearchOption.AllDirectories))' /> | ||
<RemoveDir Directories='$([System.IO.Directory]::GetDirectories(".", ".nuget", SearchOption.AllDirectories))' /> | ||
<RemoveDir Directories='$([System.IO.Directory]::GetDirectories(".", "obj", SearchOption.AllDirectories))' /> | ||
<RemoveDir Directories='$([System.IO.Directory]::GetDirectories(".", "packages", SearchOption.AllDirectories))' /> |
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.
CI runs that use the MSBuild@1
task perform an implicit clean, which would delete the pipeline artifacts we download. So I removed packages/ from clean.
93049ff
to
75eb084
Compare
- Fixed expansion of split expression in version numbers. - Removed addition of PDBs from <AllowedOutputExtensionsInPackageBuildOutputFolder>. - Removed MDS package ref dependency on Abstractions until pipelines are ready. - Renamed AbstractionsPackage to Abstractions in targets. - Updated BUILDGUIDE based on project ref behaviour. - Added feature branches to CI pipeline triggers. - Added missing/incomplete paths to the trigger. - Added dev/* branches to the CI triggers so PRs that target other dev/ branches can run CI. - Added missing MdsPackageVersion property to signed build pipeline job. - Commented-out failing NuGet tool installer task. - Re-ordered Guardian analysis step _before_ build step to avoid clobbering versioned DLLs. - Added MDS package version to AKV build/package steps. - Restored AKV nuspec ReferenceType property for AKV Official builds. - Explicitly building tooling before analysis. - Fixing validation steps to match XML props files. - Added PR automation triggers, and documented other pipeline sections. - Added ReferenceType throughout the MDS/AKV CI build steps. - Added NuGet.config update to main CI build step for Package reference mode. - Swapped Abstractions download and NuGet.config update to ensure packages/ exists before attempting to re-configure NuGet. - Clean target no longer removes packages/ - Uncommented package refs to Abstractions. - Added separate MDS and AKV project builds to support Package mode. - Added $ReferenceType$ property to MDS .nuspec like it is for AKV.
75eb084
to
181aa0c
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.
Overall, things look good to me. I think it would be helpful to have a walkthrough of a pipeline run to show us the different build artifacts, their version numbers, etc. so that we can see how it all fits together in the end.
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.
Thank you! The variable use at such a deep nesting level was one of my biggest complaints about how the pipelines were written before.
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 contains the YAML pipeline changes for CI and Official builds.
This supercedes the original PRs #3471 and #3567.
This depends on PR #3626.
Issues
The first step towards #1108.
Testing