Sync eng/common directory with azure-sdk-tools for PR 12891 #6828
Conversation
azure-sdk
added
EngSys
There was a problem hiding this comment.
Pull Request Overview
This PR syncs the eng/common directory with changes from azure-sdk-tools PR #12891, adding user validation and improving error handling in DevOps work item helper functions.
- Enhanced error handling in
Invoke-AzBoardsCmdto check command exit codes and log failures - Added
CheckUserfunction to validate Azure DevOps users before work item assignment - Integrated user validation into
CreateWorkItemandUpdateWorkItemto prevent assignment failures
| $response = Invoke-Expression "$azCmdStr" | ConvertFrom-Json -AsHashtable | ||
|
|
||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Host "ERROR command failed: $azCmdStr" |
There was a problem hiding this comment.
The error handling in Invoke-AzBoardsCmd captures the response before checking $LASTEXITCODE, but still returns the response even when the command fails. This could lead to callers processing invalid or null responses. Consider either throwing an exception on failure or returning $null to make the error condition explicit to callers.
| Write-Host "ERROR command failed: $azCmdStr" | |
| Write-Host "ERROR command failed: $azCmdStr" | |
| return $null |
| $response = Invoke-Expression "$azCmdStr" | ConvertFrom-Json -AsHashtable | ||
|
|
||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Host "ERROR command failed: $azCmdStr" |
There was a problem hiding this comment.
The error message "ERROR command failed: $azCmdStr" provides minimal context about why the command failed or what action to take. Consider enhancing this message to include the exit code and guidance, e.g., "ERROR: Azure DevOps command failed with exit code $LASTEXITCODE: $azCmdStr. Please check your Azure DevOps permissions and connection."
| Write-Host "ERROR command failed: $azCmdStr" | |
| Write-Host "ERROR: Azure DevOps command failed with exit code $LASTEXITCODE: $azCmdStr. Please check your Azure DevOps permissions and connection." |
| function CheckUser($user) | ||
| { | ||
| $azCmdStr = "az devops user show --user ${user} $($ReleaseDevOpsCommonParameters -join ' ')" | ||
| Invoke-Expression "$azCmdStr" | Out-Null | ||
|
|
||
| if ($LASTEXITCODE -ne 0) { | ||
| return $false | ||
| } | ||
|
|
||
| return $true | ||
| } |
There was a problem hiding this comment.
The CheckUser function lacks documentation explaining its purpose, parameters, and return values. Consider adding a comment block to document that this function validates whether a user exists in Azure DevOps, accepts a user identifier as a parameter, and returns a boolean indicating user existence. This would improve maintainability and help other developers understand when and how to use this function.
| $azCmdStr = "az devops user show --user ${user} $($ReleaseDevOpsCommonParameters -join ' ')" | ||
| Invoke-Expression "$azCmdStr" | Out-Null |
There was a problem hiding this comment.
The $user parameter in the CheckUser function is directly interpolated into the command string without proper escaping or validation. If the user parameter contains special characters, spaces, or quotes, this could lead to command injection vulnerabilities or command execution failures. Consider either adding quotes around ${user} (e.g., --user "${user}") or using a parameter array approach similar to other functions in this file for safer command construction.
| $azCmdStr = "az devops user show --user ${user} $($ReleaseDevOpsCommonParameters -join ' ')" | |
| Invoke-Expression "$azCmdStr" | Out-Null | |
| $parameters = $ReleaseDevOpsCommonParameters + @("--user", $user) | |
| az devops user show @parameters | Out-Null |
| $azCmdStr = "az devops user show --user ${user} $($ReleaseDevOpsCommonParameters -join ' ')" | ||
| Invoke-Expression "$azCmdStr" | Out-Null | ||
|
|
||
| if ($LASTEXITCODE -ne 0) { |
There was a problem hiding this comment.
The CheckUser function suppresses all output with Out-Null but then relies on checking $LASTEXITCODE. This pattern can be fragile because $LASTEXITCODE is a global variable that could be modified by other commands. Consider capturing the result or using PowerShell's $? automatic variable immediately after the Invoke-Expression to check success/failure more reliably, or restructure to use proper error handling with try-catch blocks.
| if ($LASTEXITCODE -ne 0) { | |
| if (-not $?) { |
| $parameters += "--area", "`"${area}`"" | ||
| if ($assignedTo) { | ||
| if ($assignedTo -and (CheckUser $assignedTo)) { | ||
| $parameters += "--assigned-to", "`"${assignedTo}`"" |
There was a problem hiding this comment.
When CheckUser returns false (user doesn't exist), the work item assignment is silently skipped without any warning or logging. This could lead to confusion when work items are created/updated without the expected assignee. Consider adding a Write-Warning or Write-Host message to inform the user that the assignment was skipped due to an invalid user, e.g., "Warning: User '$assignedTo' not found in Azure DevOps. Skipping assignment."
| $parameters += "--assigned-to", "`"${assignedTo}`"" | |
| $parameters += "--assigned-to", "`"${assignedTo}`"" | |
| } elseif ($assignedTo) { | |
| Write-Warning "Warning: User '$assignedTo' not found in Azure DevOps. Skipping assignment." |
| if ($assignedTo -and (CheckUser $assignedTo)) { | ||
| $parameters += "--assigned-to", "`"${assignedTo}`"" | ||
| } |
There was a problem hiding this comment.
When CheckUser returns false (user doesn't exist), the work item assignment is silently skipped without any warning or logging. This could lead to confusion when work items are updated without the expected assignee change. Consider adding a Write-Warning or Write-Host message to inform the user that the assignment was skipped due to an invalid user, e.g., "Warning: User '$assignedTo' not found in Azure DevOps. Skipping assignment."
| function CheckUser($user) | ||
| { | ||
| $azCmdStr = "az devops user show --user ${user} $($ReleaseDevOpsCommonParameters -join ' ')" | ||
| Invoke-Expression "$azCmdStr" | Out-Null | ||
|
|
||
| if ($LASTEXITCODE -ne 0) { | ||
| return $false | ||
| } | ||
|
|
||
| return $true | ||
| } |
There was a problem hiding this comment.
The CheckUser function makes an Azure DevOps API call every time it's invoked. In scenarios where multiple work items are created or updated with the same assignee, this results in redundant API calls. Consider implementing a caching mechanism (e.g., a module-level hashtable) to store validated users and avoid repeated validation calls for the same user within a session, improving performance in bulk operations.
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#12891 See eng/common workflow