[DO NOT REVIEW] Test pipeline for new TSP flow #28987
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
|
|
There was a problem hiding this comment.
Pull request overview
This pull request introduces a new TypeSpec (TSP) flow for generating Azure PowerShell modules directly from TypeSpec definitions. The PR is marked as "[DO NOT REVIEW]" and labeled as a pipeline test, indicating this is experimental infrastructure work to support TypeSpec-based code generation alongside the existing AutoRest flow.
Key changes:
- Adds a new
New-DevTSPModulecmdlet to the AzDev tooling that handles TypeSpec-based module generation, including fetching TSP configurations from remote or local sources, merging configurations, and invoking the TypeSpec compiler - Integrates the new TSP flow into the existing build pipeline by modifying
PrepareAutorestModule.ps1andBuildScripts.psm1to detect and use TSP-based generation whentsp-location.yamlis present - Implements YAML serialization support in
YamlHelper.csto enable configuration file generation
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 27 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/AzDev/src/Typespec/NewTSPModuleCmdlet.cs | New 784-line cmdlet implementing TypeSpec module generation with remote/local TSP fetching, configuration merging, and compilation orchestration |
| tools/AzDev/src/Typespec/README.md | Comprehensive documentation for the new cmdlet including terminology, prerequisites, usage examples, and migration guidance |
| tools/AzDev/src/Services/YamlHelper.cs | Implements YAML serialization by adding Serialize method and ISerializer instance |
| tools/AzDev/AzDev/AzDev.psd1 | Exports the new New-DevTSPModule cmdlet from the AzDev module |
| tools/AzDev/build.ps1 | Refactors build script to use Join-Path for cross-platform compatibility and adds clean build logic |
| tools/BuildScripts/PrepareAutorestModule.ps1 | Adds AzDev module build and import logic to make New-DevTSPModule available during module preparation |
| tools/BuildScripts/BuildScripts.psm1 | Integrates TSP flow by calling New-DevTSPModule when tsp-location.yaml is detected instead of AutoRest |
| src/DependencyMap/DependencyMap/ChangeLog.md | Adds test changelog entry |
| src/DependencyMap/DependencyMap.Autorest/tspconfig.yaml | New TypeSpec configuration file with PowerShell emitter options and directives for the DependencyMap module |
| src/DependencyMap/DependencyMap.Autorest/tsp-location.yaml | Updates remote repository reference from Azure to VeryEarly fork with new commit hash |
| src/DependencyMap/DependencyMap.Autorest/generate-info.json | Updates generation ID to track this new generation |
| Copy-Item "$PSScriptRoot/$module/*" "$artifacts/$module" -Recurse -Force | ||
| if (Test-Path $moduleOut) { Remove-Item $moduleOut -Recurse -Force } | ||
| dotnet publish (Join-Path $PSScriptRoot "src") --sc -o (Join-Path $moduleOut "bin") | ||
| Copy-Item (Join-Path $PSScriptRoot $module "*") $moduleOut -Recurse -Force |
There was a problem hiding this comment.
The script uses Join-Path correctly for cross-platform compatibility, which is good. However, verify that the dotnet publish and Copy-Item commands work correctly on Unix-like systems (Linux, macOS), especially the wildcard pattern in Copy-Item on line 7. Consider testing this script on non-Windows platforms to ensure full compatibility.
| Copy-Item (Join-Path $PSScriptRoot $module "*") $moduleOut -Recurse -Force | |
| Get-ChildItem -Path (Join-Path $PSScriptRoot $module) | Copy-Item -Destination $moduleOut -Recurse -Force |
| // ---------------------------------------------------------------------------------- | ||
|
|
||
| using AzDev.Models; | ||
| using NuGet.Protocol.Plugins; |
There was a problem hiding this comment.
The using statement 'NuGet.Protocol.Plugins' is imported but doesn't appear to be used in this file. Consider removing unused imports to keep the code clean and reduce compilation dependencies.
| using NuGet.Protocol.Plugins; |
| Console.WriteLine(e.Data); | ||
| }; | ||
| process.ErrorDataReceived += (sender, e) => | ||
| { | ||
| if (!string.IsNullOrEmpty(e.Data)) | ||
| Console.Error.WriteLine(e.Data); |
There was a problem hiding this comment.
The cmdlet uses Console.WriteLine and Console.Error.WriteLine directly instead of PowerShell's standard WriteVerbose, WriteWarning, or WriteError methods. This bypasses PowerShell's standard stream handling and preference variables. Replace Console.WriteLine with WriteVerbose or WriteInformation, and Console.Error.WriteLine with WriteError for consistency with PowerShell conventions.
| Console.WriteLine(e.Data); | |
| }; | |
| process.ErrorDataReceived += (sender, e) => | |
| { | |
| if (!string.IsNullOrEmpty(e.Data)) | |
| Console.Error.WriteLine(e.Data); | |
| { | |
| // Route standard output through PowerShell Information stream | |
| WriteInformation(e.Data, new[] { "NewTSPModuleCmdlet" }); | |
| } | |
| }; | |
| process.ErrorDataReceived += (sender, e) => | |
| { | |
| if (!string.IsNullOrEmpty(e.Data)) | |
| { | |
| // Route standard error through PowerShell Error stream | |
| var errorRecord = new ErrorRecord( | |
| new InvalidOperationException(e.Data), | |
| "ExternalCommandError", | |
| ErrorCategory.NotSpecified, | |
| targetObject: null); | |
| WriteError(errorRecord); | |
| } |
| catch (Exception ex) | ||
| { | ||
| throw new Exception($"Failed to download tspconfig from {uri}, {ex.Message}", ex); |
There was a problem hiding this comment.
The code catches a general Exception and wraps it, but the inner exception message is redundant since it's already included in the outer exception. Consider either removing the inner exception message from the outer message string, or simplifying the error handling to avoid duplicate information in the exception chain.
| string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":""; | ||
| if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath)) | ||
| { | ||
| string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty; | ||
| string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm")); | ||
| _npmPath = npmPath; | ||
| } | ||
| string commandPath = Path.Combine(_npmPath, command+commandSuffix); | ||
| if (!File.Exists(commandPath)) | ||
| { | ||
|
|
||
| throw new FileNotFoundException($"Command '{command}' not found in system PATH."); | ||
| } |
There was a problem hiding this comment.
The condition checks for string.IsNullOrEmpty(_npmPath) but _npmPath is assigned a value inside the if block. If the FirstOrDefault returns null, _npmPath will be null, and on the next call, the condition will still be true, causing the PATH to be searched again unnecessarily. The logic should check if _npmPath is null or empty after the assignment and only proceed if a valid path was found. Additionally, if no npm directory is found, this should throw an appropriate error rather than continuing with a null _npmPath.
| string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":""; | |
| if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath)) | |
| { | |
| string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty; | |
| string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm")); | |
| _npmPath = npmPath; | |
| } | |
| string commandPath = Path.Combine(_npmPath, command+commandSuffix); | |
| if (!File.Exists(commandPath)) | |
| { | |
| throw new FileNotFoundException($"Command '{command}' not found in system PATH."); | |
| } | |
| string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd" : ""; | |
| if (string.IsNullOrEmpty(_npmPath) || !Directory.Exists(_npmPath)) | |
| { | |
| string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty; | |
| string npmPath = pathEnv | |
| .Split(Path.PathSeparator) | |
| .FirstOrDefault(path => path.EndsWith("npm", StringComparison.OrdinalIgnoreCase) && Directory.Exists(path)); | |
| if (string.IsNullOrEmpty(npmPath)) | |
| { | |
| throw new DirectoryNotFoundException("Unable to locate an npm directory in the system PATH."); | |
| } | |
| _npmPath = npmPath; | |
| } | |
| string commandPath = Path.Combine(_npmPath, command + commandSuffix); | |
| if (!File.Exists(commandPath)) | |
| { | |
| throw new FileNotFoundException($"Command '{command}' not found in system PATH."); | |
| } |
| - **Sample remote TSP Location without PowerShell emitter option**: https://github.com/VeryEarly/azure-rest-api-specs/blob/yabo/test-tspconfig/specification/azuredependencymap/DependencyMap.Management/tspconfig.yaml | ||
| - **Sample AsPSConfig with PowerShell emitter option**: https://github.com/VeryEarly/azure-powershell/blob/yabo/tsp-client-bugbash/src/DependencyMap/DependencyMap.Autorest/tspconfig.yaml |
There was a problem hiding this comment.
The sample URLs point to personal forks (VeryEarly) rather than official Azure repositories. For documentation intended for broader use, consider using examples from the official Azure repositories, or clearly label these as example/test URLs that users should replace with their own.
| - **Sample remote TSP Location without PowerShell emitter option**: https://github.com/VeryEarly/azure-rest-api-specs/blob/yabo/test-tspconfig/specification/azuredependencymap/DependencyMap.Management/tspconfig.yaml | |
| - **Sample AsPSConfig with PowerShell emitter option**: https://github.com/VeryEarly/azure-powershell/blob/yabo/tsp-client-bugbash/src/DependencyMap/DependencyMap.Autorest/tspconfig.yaml | |
| - **Sample remote TSP Location without PowerShell emitter option**: https://github.com/Azure/azure-rest-api-specs/blob/29e9e3ca1a1bccba66a6cf092dbc317c639989b1/specification/azuredependencymap/DependencyMap.Management/tspconfig.yaml | |
| - **Sample AzPSConfig with PowerShell emitter option**: https://github.com/Azure/azure-powershell/blob/main/path-to-your-module/tspconfig.yaml (replace with the path to your own tspconfig.yaml) |
| - Additional information about change #1 | ||
| --> | ||
| ## Upcoming Release | ||
| * Pipeline test |
There was a problem hiding this comment.
The ChangeLog entry "Pipeline test" is not user-facing and doesn't follow the required format. According to the ChangeLog.md guidelines, entries should describe changes from the user's perspective and explain what changed and how it affects their usage. This appears to be a test entry that should be removed or replaced with a proper description of the changes being introduced.
| Console.WriteLine($"No ChildConfig provided, use {parentConfigPath}"); | ||
| return parent; | ||
| } | ||
| string childConfig = GetTSPConfig(childConfigPath); | ||
| // Validate and deserialize child config | ||
| if (string.IsNullOrWhiteSpace(childConfig) || !YamlHelper.TryDeserialize<IDictionary<object, object>>(childConfig, out IDictionary<object, object> child)) | ||
| { | ||
| throw new ArgumentException("Invalid child TSP config: " + childConfig, nameof(childConfig)); | ||
| } | ||
|
|
||
| Console.WriteLine("Performing deep merge for parent: " + parentConfigPath + " and child: " + childConfigPath); | ||
| var mergedConfig = MergeNestedObjectIteratively(parent, child); | ||
| Console.WriteLine("TSP config merge completed successfully"); |
There was a problem hiding this comment.
Similar to other Console.WriteLine usage in this file, use PowerShell's WriteVerbose or WriteInformation methods instead for consistency with PowerShell cmdlet conventions.
| string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":""; | ||
| if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath)) | ||
| { | ||
| string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty; | ||
| string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm")); | ||
| _npmPath = npmPath; | ||
| } | ||
| string commandPath = Path.Combine(_npmPath, command+commandSuffix); |
There was a problem hiding this comment.
The variable name 'commandSuffix' could be more descriptive. Consider renaming it to 'commandFileExtension' or 'platformCommandExtension' to make it clearer that this represents the file extension for commands on different platforms.
| string commandSuffix = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd":""; | |
| if ( string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath)) | |
| { | |
| string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty; | |
| string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm")); | |
| _npmPath = npmPath; | |
| } | |
| string commandPath = Path.Combine(_npmPath, command+commandSuffix); | |
| string commandFileExtension = Environment.OSVersion.Platform == PlatformID.Win32NT ? ".cmd" : ""; | |
| if (string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath)) | |
| { | |
| string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty; | |
| string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm")); | |
| _npmPath = npmPath; | |
| } | |
| string commandPath = Path.Combine(_npmPath, command + commandFileExtension); |
| $tspLocationPath = Join-Path $GenerateDirectory "tsp-location.yaml" | ||
| if (Test-Path $tspLocationPath) { | ||
| tsp-client update >> $GenerateLog | ||
| # Not good practice to do this, this requires 'PrepareAutorestModule.ps1' to prepare AzDev and pipeline to install tsp compiler' | ||
| New-DevTSPModule >> $GenerateLog |
There was a problem hiding this comment.
The new call to New-DevTSPModule in the pipeline context processes tsp-location.yaml, whose directory field can point to any local file path and is fully attacker-controlled via the PR. Inside New-DevTSPModule, this path is read and, if the file is not a valid TSP YAML dictionary, its entire contents are concatenated into an exception message (Invalid parent TSP config: ...), which will be written into the build logs, allowing a malicious contributor to exfiltrate arbitrary readable files (including potential secrets) from the build agent. To mitigate this, strictly validate or restrict the allowed paths from tsp-location.yaml (e.g., only under the repo root), and avoid echoing raw file contents in error messages for invalid configs.
1 participant
Description
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.