Sync eng/common directory with azure-sdk-tools for PR 13005 #25653
Conversation
azure-sdk
added
EngSys
There was a problem hiding this comment.
Pull request overview
This PR syncs the eng/common directory with azure-sdk-tools PR #13005, adding support for tracking TypeSpec spec project paths throughout the release workflow. The changes introduce a new SpecProjectPath property that is read from tsp-location.yaml files and propagated to Azure DevOps work items.
Key changes:
- Added
SpecProjectPathproperty to thePackagePropsclass to store the path from the azure-rest-api-specs repository - Modified work item helper functions to include the spec project path when creating or updating package work items
- Updated logging to display the spec project path during package validation
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| eng/common/scripts/Package-Properties.ps1 | Added SpecProjectPath property to PackageProps class and logic to read from tsp-location.yaml |
| eng/common/scripts/Save-Package-Properties.ps1 | Added logging output for the spec project path |
| eng/common/scripts/Validate-All-Packages.ps1 | Updated function call to include specProjectPath parameter |
| eng/common/scripts/Helpers/DevOps-WorkItem-Helpers.ps1 | Enhanced work item functions to store and update the Custom.SpecProjectPath field in Azure DevOps |
| } | ||
|
|
||
| if (Test-Path (Join-Path $directoryPath 'tsp-location.yaml')) { | ||
| $this.SpecProjectPath = (LoadFrom-Yaml (Join-Path $directoryPath 'tsp-location.yaml')).directory |
There was a problem hiding this comment.
The LoadFrom-Yaml function can return $null if the YAML file parsing fails or if the file doesn't exist (even though it's already checked). Accessing the .directory property on a $null object will cause an error. This should be wrapped in error handling or null-checking logic.
Consider:
if (Test-Path (Join-Path $directoryPath 'tsp-location.yaml')) {
$tspLocation = LoadFrom-Yaml (Join-Path $directoryPath 'tsp-location.yaml')
if ($tspLocation -and $tspLocation.directory) {
$this.SpecProjectPath = $tspLocation.directory
}
}| $this.SpecProjectPath = (LoadFrom-Yaml (Join-Path $directoryPath 'tsp-location.yaml')).directory | |
| $tspLocation = LoadFrom-Yaml (Join-Path $directoryPath 'tsp-location.yaml') | |
| if ($tspLocation -and $tspLocation.directory) { | |
| $this.SpecProjectPath = $tspLocation.directory | |
| } |
| if ($pkgDisplayName -ne $existingItem.fields["Custom.PackageDisplayName"]) { $changedField = "Custom.PackageDisplayName" } | ||
| if ($pkgType -ne [string]$existingItem.fields["Custom.PackageType"]) { $changedField = "Custom.PackageType" } | ||
| if ($pkgNewLibrary -ne $existingItem.fields["Custom.PackageTypeNewLibrary"]) { $changedField = "Custom.PackageTypeNewLibrary" } | ||
| if ($pkgRepoPath -ne $existingItem.fields["Custom.PackageRepoPath"]) { $changedField = "Custom.PackageRepoPath" } |
There was a problem hiding this comment.
The SpecProjectPath field is conditionally added to the work item but there's no check in the $changedField validation logic (lines 546-553) to detect when SpecProjectPath changes. This means that if only SpecProjectPath changes in an existing work item, the update won't be triggered.
Consider adding a check similar to the other fields:
if ($specProjectPath -ne $existingItem.fields["Custom.SpecProjectPath"]) { $changedField = "Custom.SpecProjectPath" }| if ($pkgRepoPath -ne $existingItem.fields["Custom.PackageRepoPath"]) { $changedField = "Custom.PackageRepoPath" } | |
| if ($pkgRepoPath -ne $existingItem.fields["Custom.PackageRepoPath"]) { $changedField = "Custom.PackageRepoPath" } | |
| if ($specProjectPath -ne $existingItem.fields["Custom.SpecProjectPath"]) { $changedField = "Custom.SpecProjectPath" } |
| if ((!$workItem.fields.ContainsKey('Custom.SpecProjectPath') -and $packageInfo.SpecProjectPath) -or | ||
| ($workItem.fields.ContainsKey('Custom.SpecProjectPath') -and ($workItem.fields['Custom.SpecProjectPath'] -ne $packageInfo.SpecProjectPath)) | ||
| ) { | ||
| Write-Host "Updating SpecProjectPath to '$($packageInfo.SpecProjectPath)' for work item [$($workItem.id)]" | ||
| UpdateWorkItem ` | ||
| -id $workItem.id ` | ||
| -fields "`"Custom.SpecProjectPath=$($packageInfo.SpecProjectPath)`"" ` | ||
| -outputCommand $false | ||
| } |
There was a problem hiding this comment.
The logic for updating SpecProjectPath has a potential issue. When $packageInfo.SpecProjectPath is an empty string (the default value from the parameter), the condition (!$workItem.fields.ContainsKey('Custom.SpecProjectPath') -and $packageInfo.SpecProjectPath) will evaluate to false because an empty string is falsy. However, the second condition will still try to update if the field exists and doesn't match the empty string, which could clear the field unintentionally.
Consider adding an explicit check for non-empty strings:
if ($packageInfo.SpecProjectPath -and
((!$workItem.fields.ContainsKey('Custom.SpecProjectPath')) -or
($workItem.fields['Custom.SpecProjectPath'] -ne $packageInfo.SpecProjectPath))
) {
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#13005 See eng/common workflow