[CI] Fix PR detection in api-diff configure#24626
Conversation
The api-diff pipeline is triggered via ResourceTrigger, so BUILD_REASON is not PullRequest and PR metadata may be absent. The configure step in VSTS.psm1 treated some runs as PR builds using BuildReason and a branch check, then unconditionally called GitHub PR APIs, which failed when no PR id was resolved. Align PR detection with parse_pr_labels.ps1: use the parent build reason (fallback to current BUILD_REASON) and only treat Manual/IndividualCI builds as PRs when the parent branch name is 'merge'. This avoids misclassifying pipeline-triggered CI runs, prevents empty ChangeId lookups, and keeps CI behavior (tags/labels) consistent with the intended non‑PR path.
There was a problem hiding this comment.
Pull request overview
Fixes PR-build detection in the api-diff configure step so resource-triggered runs aren’t misclassified as PR builds, and prevents GitHub PR API calls when no PR id is available.
Changes:
- Derive an effective
$buildReason(preferring the parent build reason, falling back toBUILD_REASON) for tagging/PR detection. - Refine PR detection rules (Manual/IndividualCI only treated as PR when parent branch name is
merge). - Guard GitHub PR info retrieval so it only runs when a non-empty PR id was resolved; otherwise fall back to CI labeling/tagging.
| Write-Host "Configuring build from PR." | ||
|
|
||
| # retrieve the PR data to be able to fwd the labels from github | ||
| $prId = $this.ExportPRId($configuration) | ||
| } | ||
|
|
||
| if ($prId) { |
There was a problem hiding this comment.
The log line "Configuring build from PR." can be misleading now that PR metadata fetch is gated on successfully resolving a PR id. If ExportPRId returns null, the build is treated as CI, but the logs still claim it was configured from a PR. Consider moving/adjusting this message so it only prints after a non-empty PR id is found (or log that PR detection was attempted).
| Write-Host "Configuring build from PR." | |
| # retrieve the PR data to be able to fwd the labels from github | |
| $prId = $this.ExportPRId($configuration) | |
| } | |
| if ($prId) { | |
| # retrieve the PR data to be able to fwd the labels from github | |
| $prId = $this.ExportPRId($configuration) | |
| } | |
| if ($prId) { | |
| Write-Host "Configuring build from PR." |
| $buildReason = $configuration.PARENT_BUILD_BUILD_REASON | ||
| if (-not $buildReason) { | ||
| $buildReason = $Env:BUILD_REASON | ||
| } | ||
|
|
||
| if ($buildReason -eq "Schedule") { | ||
| $tags.Add("cronjob") | ||
| } | ||
|
|
||
| if ($Env:IS_PR -eq "true" -or $configuration.BuildReason -eq "PullRequest" -or (($configuration.BuildReason -eq "Manual") -and ($configuration.PARENT_BUILD_BUILD_SOURCEBRANCH -eq "merge")) ) { | ||
| $prId = $null | ||
| if ($Env:IS_PR -eq "true" -or $buildReason -eq "PullRequest" -or (($buildReason -eq "Manual" -or $buildReason -eq "IndividualCI") -and ($configuration.PARENT_BUILD_BUILD_SOURCEBRANCHNAME -eq "merge")) ) { | ||
| Write-Host "Configuring build from PR." | ||
|
|
||
| # retrieve the PR data to be able to fwd the labels from github | ||
| $prId = $this.ExportPRId($configuration) | ||
| } | ||
|
|
||
| if ($prId) { | ||
| $prInfo = Get-GitHubPRInfo -ChangeId $prId |
There was a problem hiding this comment.
PR detection/tagging behavior was changed (build reason selection, Manual/IndividualCI handling, and skipping GitHub PR API calls when no PR id is resolved), but there are no Pester tests covering these PR/non-PR branches. Add unit tests for New-BuildConfiguration covering at least: (1) IS_PR/buildReason indicates PR but ExportPRId returns null => no Get-GitHubPRInfo call and ciBuild tagging, and (2) Manual/IndividualCI + SourceBranchName=merge => PR path.
✅ [CI Build #df0d7d9] Build passed (Build packages) ✅Pipeline on Agent |
✅ [CI Build #df0d7d9] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #df0d7d9] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS arm64 - Mac Tahoe (26) passed 💻✅ All tests on macOS arm64 - Mac Tahoe (26) passed. Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
✅ [CI Build #df0d7d9] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #df0d7d9] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #df0d7d9] Build passed (Build macOS tests) ✅Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS M1 - Mac Monterey (12) passed 💻✅ All tests on macOS M1 - Mac Monterey (12) passed. Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS arm64 - Mac Sequoia (15) passed 💻✅ All tests on macOS arm64 - Mac Sequoia (15) passed. Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS M1 - Mac Ventura (13) passed 💻✅ All tests on macOS M1 - Mac Ventura (13) passed. Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS arm64 - Mac Tahoe (26) passed 💻✅ All tests on macOS arm64 - Mac Tahoe (26) passed. Pipeline on Agent |
💻 [CI Build #df0d7d9] Tests on macOS X64 - Mac Sonoma (14) passed 💻✅ All tests on macOS X64 - Mac Sonoma (14) passed. Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #df0d7d9] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 117 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
The api-diff pipeline is triggered via ResourceTrigger, so BUILD_REASON is not PullRequest and PR metadata may be absent. The configure step in VSTS.psm1 treated some runs as PR builds using BuildReason and a branch check, then unconditionally called GitHub PR APIs, which failed when no PR id was resolved.
Align PR detection with parse_pr_labels.ps1: use the parent build reason (fallback to current BUILD_REASON) and only treat Manual/IndividualCI builds as PRs when the parent branch name is 'merge'. This avoids misclassifying pipeline-triggered CI runs, prevents empty ChangeId lookups, and keeps CI behavior (tags/labels) consistent with the intended non‑PR path.