Sync eng/common directory with azure-sdk-tools for PR 12410 #3144
Conversation
azure-sdk
added
EngSys
There was a problem hiding this comment.
Pull Request Overview
Adds optional ArtifactsJson input (alongside or instead of PackageNames) to set test pipeline package versions and updates the YAML template to use strongly-typed parameters. Core change introduces a new code path for deriving package names and version tags from artifact metadata.
- Added ArtifactsJson parameter and logic to parse and process artifacts (including Java groupId handling)
- Made PackageNames optional and updated YAML parameter schema to typed form
- Introduced parallel (duplicated) version resolution logic for the new artifacts path
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| eng/common/scripts/SetTestPipelineVersion.ps1 | Adds ArtifactsJson handling, optional PackageNames, and artifact-aware version/tag computation. |
| eng/common/pipelines/templates/steps/set-test-pipeline-version.yml | Converts parameters to typed form and plumbs through new ArtifactsJson argument. |
Comments suppressed due to low confidence (1)
eng/common/scripts/SetTestPipelineVersion.ps1:108
- [nitpick] The version/tag resolution logic (initializing newVersion, computing prefix, fetching tags, sorting, setting prerelease fields, and calling SetPackageVersion) is duplicated between the artifacts branch and the fallback loop; this increases maintenance overhead and risk of divergence. Extract the shared logic into a helper function (e.g., Resolve-And-SetPackageVersion -PackageName [-GroupId]) and invoke it from both branches.
foreach ($packageName in $packageNamesArray) {
Write-Host "Processing $packageName"
$newVersion = [AzureEngSemanticVersion]::new("1.0.0")
$prefix = "$packageName$TagSeparator"
| default: '' | ||
|
|
||
| steps: | ||
| - ${{ if eq(parameters.TestPipeline, 'true') }}: |
There was a problem hiding this comment.
The parameter TestPipeline is now declared as a boolean (type: boolean) but the condition compares it to the string 'true'; this can lead to unintended evaluation or brittleness. Change to - ${{ if eq(parameters.TestPipeline, true) }}: or simply - ${{ if parameters.TestPipeline }}: for a proper boolean check.
| - ${{ if eq(parameters.TestPipeline, 'true') }}: | |
| - ${{ if parameters.TestPipeline }}: |
Copilot uses AI. Check for mistakes.
| Write-Host "Get Latest Tag : git tag -l $prefix*" | ||
| $latestTags = git tag -l "$prefix*" | ||
|
|
||
| $semVars = @() | ||
|
|
||
| $semVarsSorted = [AzureEngSemanticVersion]::SortVersionStrings($semVars) | ||
| Write-Host "Last Published Version $($semVarsSorted[0])" | ||
| $newVersion = [AzureEngSemanticVersion]::new($semVarsSorted[0]) | ||
| if ($latestTags -and ($latestTags.Length -gt 0)) { | ||
| foreach ($tag in $latestTags) { | ||
| $semVars += $tag.Substring($prefix.Length) | ||
| } |
There was a problem hiding this comment.
When git returns a single matching tag, $latestTags is a [string]; iterating it with foreach enumerates characters, producing incorrect substrings. Wrap the command in an array cast to normalize: $latestTags = @(git tag -l "$prefix*") and iterate over @($latestTags) to ensure tag-level enumeration.
Copilot uses AI. Check for mistakes.
| if ($artifacts) { | ||
| # When using ArtifactsJson, process each artifact with its name and groupId (if applicable) | ||
| try { | ||
| foreach ($artifact in $artifacts) { | ||
| $packageName = $artifact.name | ||
| $newVersion = [AzureEngSemanticVersion]::new("1.0.0") | ||
| $prefix = "$packageName$TagSeparator" |
There was a problem hiding this comment.
[nitpick] The version/tag resolution logic (initializing newVersion, computing prefix, fetching tags, sorting, setting prerelease fields, and calling SetPackageVersion) is duplicated between the artifacts branch and the fallback loop; this increases maintenance overhead and risk of divergence. Extract the shared logic into a helper function (e.g., Resolve-And-SetPackageVersion -PackageName [-GroupId]) and invoke it from both branches.
Copilot uses AI. Check for mistakes.
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#12410 See eng/common workflow