-
Notifications
You must be signed in to change notification settings - Fork 4.2k
In PR validation, test sign contents of nupkg, but not the nupkg itself #79643
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
4990ffc to
8edc5e4
Compare
ec2e56a to
6130770
Compare
eng/build-utils.ps1
Outdated
| } | ||
|
|
||
| function GetPublishData() { | ||
| function GetPublishData([string]$branchName = "main") { |
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 now pulls publish data from the branch we're on. this does mean that release branch configurations will have to be updated on the release branch, but that doesn't seem unreasonable to me.
this makes it 10x easier to test changes that require test branches.
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.
Would this also allow us to run the official build pipeline on any branch? In other words, will we be able to deprecate the "PR validation" pipeline and have one pipeline for both (like we have in Razor) after this PR?
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.
Would this also allow us to run the official build pipeline on any branch? In other words, will we be able to deprecate the "PR validation" pipeline and have one pipeline for both (like we have in Razor) after this PR?
Yes and no. We can now pull the publishdata from the branch itself, which allows us to test official build changes (particularly publishing) on other branches. However incoming signing requirements mean that there are special requirements to queue builds on the official pipeline.
So general VS PR validations must be test signed and must be done from a different pipeline. We will have to split the Razor build into two separate pipelines soon.
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 actually going to revert this change. there is more that is needed to actually pull the publishdata from the branch, and I'd rather do it all together (insertion still pulls from main).
This reverts commit 0721c3b.
This allows us to support test signing in PR validation builds.
VS cloudbuild will not build with test signed packages (nuget reports certificate errors related to expiration), and we have no way in an insertion to specify that the PR should be queued with specific parameters to disable the errors.
Instead, we can simply not sign the nuget packages (while still signing the contents to make sure subsequent steps like ngen see strong named dlls).
val build - https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/657144