Move PR validation logic from the official build to a separate yml #54053
Conversation
@genlu I like the suggestion of moving main validation out of the official build. I wouldn't want to add it to the pr validation yml, so maybe there are multiple which can share a common build template. |
| Write-Host ($remoteRef | Out-String) | ||
|
|
||
| $prHeadSHA = $remoteRef.Split()[0] | ||
| if (!$prHeadSHA.StartsWith($commitSHA)) { |
There was a problem hiding this comment.
I think it might be reasonable to simply check if the $commitSHA is an ancestor of the $prHeadSHA because there may be a scenario where the user wants to validate one of the earlier commits in their PR.
There was a problem hiding this comment.
Yeah, I thought over this, but wasn't sure what the scenario would be. We obviously couldn't take half a PR. Also, It isn't the same as pulling pull/##/merge, since it wouldn't contain changes from the target branch.
Happy to defer this as a future enhancement though, once we work out how it should work.
There was a problem hiding this comment.
The scenario is that my PR has an RPS regression and I suspect a specific commit in the PR introduced the regression. So I may want to validate the specific commit as well as the one before it. That said, there is no need to introduce this capability, since it would be new.
| Write-Host "##vso[task.LogIssue type=error;]The PR's Head SHA ($prHeadSHA) does not begin with the specified commit SHA ($commitSHA). Unreviewed changes may have been pushed to the PR." | ||
| exit 1 | ||
| } | ||
|
|
||
| Write-Host "Setting up the build for PR validation by merging refs/pull/$prNumber/merge into $sourceBranchName..." | ||
| git pull origin refs/pull/$prNumber/merge |
There was a problem hiding this comment.
This seems like a race condition. The remote SHA may have changed in between ls-remote and pull. Consider pulling the $commitSHA instead.
There was a problem hiding this comment.
I mean technically true, but would be very tight.
Happy to defer this as a future enhancement where we also include merging the target branch in with a specific commit sha from the PR branch.
There was a problem hiding this comment.
It feels like the risk of this may be less if we fetch all the refs we want in one invocation. However, don't feel a need to address this in this PR.
It looks like this build failed. It's not clear to me if it could be related to the official yml changes. |
Failed due to missing optprof. Will rerun specifying a "potentially" good optprof drop https://dev.azure.com/dnceng/internal/_build/results?buildId=1188427&view=results Edit: The build with good optprof completed. |
| } | ||
|
|
||
| Write-Host "Getting the hash of refs/pull/$prNumber/head..." | ||
| $remoteRef = git ls-remote origin refs/pull/$prNumber/head |
There was a problem hiding this comment.
refs/pull/$prNumber/head [](start = 38, length = 24)
Would it be an issue that we check for the SHA of refs/pull/$prNumber/head but later do a separate pull from another branch refs/pull/$prNumber/merge
There was a problem hiding this comment.
Is this the same concern as #54053 (comment)?
There was a problem hiding this comment.
We've been using the merge branch since it will bring along changes from the PR's target branch and give us the most accurate representation of the PR merging and flowing into the release branch. The commit sha is intended to ensure that the PR branch is still in the same state as it was when reviewed by the user starting the validation.
cc: @genlu, @RikkiGibson
Test PR validation with too short SHA: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4865094&view=results
Test PR validation with invalid SHA: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4865097&view=results
Test PR validation with correct SHA: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4865173&view=results
Test official build: https://dev.azure.com/dnceng/internal/_build/results?buildId=1184266&view=results