[Eng] Improve typespec devexp with New-DevTSPModule
#28954
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
VeryEarly
changed the title
Update-DevTSPModule
There was a problem hiding this comment.
Pull request overview
This PR introduces a new cmdlet Update-DevTSPModule to the AzDev tooling that automates TypeSpec-based Azure PowerShell module generation. The cmdlet handles downloading TypeSpec configurations from remote GitHub repositories or local paths, merges them with Azure PowerShell-specific configurations, and generates PowerShell modules using the TypeSpec PowerShell emitter.
Key changes:
- Adds comprehensive TypeSpec workflow automation cmdlet with support for remote and local TSP configurations
- Implements YAML serialization support in the YamlHelper service
- Updates build script to clean artifacts directory before rebuilding
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/AzDev/src/Typespec/UpdateTSPModuleCmdlet.cs | New cmdlet implementation handling TSP config resolution, git operations, npm dependency installation, directory copying, and TypeSpec compilation - 759 lines of core logic |
| tools/AzDev/src/Typespec/README.md | Documentation covering cmdlet usage, terminology, prerequisites, and various usage scenarios including local/remote TSP sources |
| tools/AzDev/src/Services/YamlHelper.cs | Implements Serialize method to support YAML output generation required by the cmdlet |
| tools/AzDev/build.ps1 | Adds cleanup of artifacts directory before build to ensure clean builds |
| tools/AzDev/AzDev/AzDev.psd1 | Exports the new Update-DevTSPModule cmdlet for module consumers |
tools/AzDev/src/Typespec/README.md
Outdated
| - ***Execute this cmdlet anywhere under $RootDirectory without explicitly providing it***: Run this cmdlet under any subdirectory of azure-powershell, `RepoRoot` will be calculated. | ||
| - ***Pass by parameter***: `Update-DevTSPModule -RepoRoot $RootDirectory` | ||
| - ***AzDev Cmdlet***: `Set-DevContext -RepoRoot $RootDirectory` | ||
| - **Emitter**: The tool responsible for generating PowerShell code from the TypeSpec definition. This cmdlet specifically works witapprovedh the `@azure-tools/typespec-powershell` emitter. |
There was a problem hiding this comment.
Typo in the word "with" - contains extra characters "approved".
| - **Emitter**: The tool responsible for generating PowerShell code from the TypeSpec definition. This cmdlet specifically works witapprovedh the `@azure-tools/typespec-powershell` emitter. | |
| - **Emitter**: The tool responsible for generating PowerShell code from the TypeSpec definition. This cmdlet specifically works with the `@azure-tools/typespec-powershell` emitter. |
| Directory.Delete(path, true); | ||
| } | ||
|
|
||
| private async Task installDependencies(string workingDirectory) |
There was a problem hiding this comment.
Method name installDependencies doesn't follow C# naming conventions - methods should use PascalCase. Should be InstallDependencies.
| private async Task installDependencies(string workingDirectory) | |
| private async Task InstallDependencies(string workingDirectory) |
| Dictionary<string, object> tspLocationPWDContent = YamlHelper.Deserialize<Dictionary<string, object>>(File.ReadAllText(tspLocationPath)); | ||
| //if tspconfig emitted previously was from local, only record the absolute directory name | ||
| if (File.Exists((string)tspLocationPWDContent["directory"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["repo"]) && string.IsNullOrEmpty((string)tspLocationPWDContent["commit"])) | ||
| { | ||
| if (remoteInfo != (null, null, null, null)) | ||
| { | ||
| throw new ArgumentException("Emitted by local TSP last time, cannot update by remote info. Please provide remote `-TSPLocation`."); | ||
| } | ||
| return (string)tspLocationPWDContent["directory"]; | ||
| } | ||
| (string RemoteDirectory, string RemoteCommit, string RemoteRepositoryName, string RemoteForkName) = remoteInfo; | ||
| //otherwise it was from remote, construct its url | ||
| string repo = !string.IsNullOrEmpty(RemoteForkName) ? $"{RemoteForkName}/azure-rest-api-specs" : (!string.IsNullOrEmpty(RemoteRepositoryName) ? RemoteRepositoryName : (string)tspLocationPWDContent["repo"]); | ||
| string commit = !string.IsNullOrEmpty(RemoteCommit) ? RemoteCommit : (string)tspLocationPWDContent["commit"]; | ||
| string directory = !string.IsNullOrEmpty(RemoteDirectory) ? RemoteDirectory : (string)tspLocationPWDContent["directory"]; |
There was a problem hiding this comment.
The code doesn't validate that the YAML file contains the expected keys before accessing them with dictionary indexing. If "directory", "repo", or "commit" keys are missing, this will throw a KeyNotFoundException which is less clear than a proper validation error.
Recommendation: Add validation:
Dictionary<string, object> tspLocationPWDContent = YamlHelper.Deserialize<Dictionary<string, object>>(File.ReadAllText(tspLocationPath));
if (!tspLocationPWDContent.ContainsKey("directory") ||
!tspLocationPWDContent.ContainsKey("repo") ||
!tspLocationPWDContent.ContainsKey("commit"))
{
throw new ArgumentException($"Invalid tsp-location.yaml format at {tspLocationPath}. Required keys: directory, repo, commit");
}| } | ||
| if (string.IsNullOrEmpty(potentialRoot)) | ||
| { | ||
| throw new ArgumentException("Unable to determine Azure PowerShell repository root. Please execute this cmdlet in Azure-PowerShell repository, Or please provide `-RepoRoot` or set it through `Set-DevContext -RepoRoot`."); |
There was a problem hiding this comment.
The error message has inconsistent punctuation and grammar. "Azure-PowerShell repository, Or" should not have a comma before "Or", and "Or" should not be capitalized mid-sentence.
Recommendation:
throw new ArgumentException("Unable to determine Azure PowerShell repository root. Please execute this cmdlet in the Azure PowerShell repository, or provide `-RepoRoot` or set it through `Set-DevContext -RepoRoot`.");| throw new ArgumentException("Unable to determine Azure PowerShell repository root. Please execute this cmdlet in Azure-PowerShell repository, Or please provide `-RepoRoot` or set it through `Set-DevContext -RepoRoot`."); | |
| throw new ArgumentException("Unable to determine Azure PowerShell repository root. Please execute this cmdlet in the Azure PowerShell repository, or provide `-RepoRoot` or set it through `Set-DevContext -RepoRoot`."); |
| try | ||
| { | ||
| installDependencies(Path.GetDirectoryName(tempTSPLocation)).Wait(); | ||
| RunCommand(FindNPMCommandFromPath("tsp.cmd"), $"compile ./ --emit {EmitterPath ?? emitterName} --output-dir {emitterOutputDir}", Path.GetDirectoryName(tempTSPLocation)).Wait(); |
There was a problem hiding this comment.
The hardcoded .cmd extension makes this code Windows-specific and violates cross-platform compatibility requirements. On Unix-like systems (Linux, macOS), npm and tsp commands don't have the .cmd extension.
Recommendation: Detect the platform and use the appropriate command:
private string FindNPMCommandFromPath(string command)
{
// Remove .cmd extension for cross-platform compatibility
string baseCommand = command.Replace(".cmd", "");
if (_npmPath == "" || !File.Exists(_npmPath))
{
string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty;
string npmPath = pathEnv.Split(Path.PathSeparator)
.FirstOrDefault(path => path.EndsWith("npm"));
_npmPath = npmPath;
}
// On Windows, check for .cmd, .bat, or .exe; on Unix, use the command directly
string commandPath;
if (Environment.OSVersion.Platform == PlatformID.Win32NT)
{
commandPath = Path.Combine(_npmPath, $"{baseCommand}.cmd");
if (!File.Exists(commandPath))
commandPath = Path.Combine(_npmPath, $"{baseCommand}.bat");
if (!File.Exists(commandPath))
commandPath = Path.Combine(_npmPath, $"{baseCommand}.exe");
}
else
{
commandPath = Path.Combine(_npmPath, baseCommand);
}
if (!File.Exists(commandPath))
{
throw new FileNotFoundException($"Command '{baseCommand}' not found in system PATH.");
}
return commandPath;
}| return tempTspLocation; | ||
| } | ||
|
|
||
| //copy sourcDir and put it under destinationDir |
There was a problem hiding this comment.
The comment has a typo: "sourcDir" should be "sourceDir". Also, the comment doesn't accurately describe what the method does - it copies the source directory's contents under a subdirectory (with the source directory's name) within the destination directory, not just "put it under" the destination.
| //copy sourcDir and put it under destinationDir | |
| // Copies the contents of sourceDir into a subdirectory (named after sourceDir) within destinationDir, | |
| // excluding any files or directories specified in the 'exclude' array. |
| await RunCommand("git", $"clone {cloneRepo} {tempDirPath} --no-checkout --filter=tree:0", outDir); | ||
| await RunCommand("git", $"sparse-checkout set {path}", tempDirPath); | ||
| await RunCommand("git", $"sparse-checkout add {path}", tempDirPath); | ||
| await RunCommand("git", $"checkout {commit}", tempDirPath); |
There was a problem hiding this comment.
The git clone URL and path parameters are constructed using string interpolation without validation or sanitization. This could allow command injection if the repo, path, or commit variables contain malicious characters (e.g., shell metacharacters, quotes).
Recommendation: Validate or escape the parameters before using them in shell commands, or use a library that handles command execution safely. At minimum:
- Validate
repomatches expected format (e.g., "owner/repo-name") - Validate
commitis a valid 40-character hex SHA - Validate
pathdoesn't contain dangerous characters like;,|,&, etc.
| string cloneRepo = $"https://github.com/{repo}.git"; | ||
| await RunCommand("git", $"clone {cloneRepo} {tempDirPath} --no-checkout --filter=tree:0", outDir); | ||
| await RunCommand("git", $"sparse-checkout set {path}", tempDirPath); | ||
| await RunCommand("git", $"sparse-checkout add {path}", tempDirPath); |
There was a problem hiding this comment.
The git operations use separate commands which is inefficient. Line 362 and 363 both call sparse-checkout - line 363's sparse-checkout add is redundant since line 362's sparse-checkout set already sets the path.
Recommendation: Remove line 363 (sparse-checkout add) as it duplicates line 362's functionality:
await RunCommand("git", $"clone {cloneRepo} {tempDirPath} --no-checkout --filter=tree:0", outDir);
await RunCommand("git", $"sparse-checkout set {path}", tempDirPath);
await RunCommand("git", $"checkout {commit}", tempDirPath);| await RunCommand("git", $"sparse-checkout add {path}", tempDirPath); |
| throw new InvalidOperationException($"Failed to prepare temporary directory [{tempDirPath}]: {ex.Message}", ex); | ||
| } | ||
| CopyDirectory(tspLocation, tempDirPath, ["tsp-output", "node_modules"]); | ||
| string tempTspLocation = Path.Combine(tempDirPath, tspLocation, "tspconfig.yaml"); |
There was a problem hiding this comment.
The path construction is incorrect. After calling CopyDirectory(tspLocation, tempDirPath, ...) which copies the directory and its contents under tempDirPath, line 386 attempts to combine tempDirPath, the full tspLocation path, and "tspconfig.yaml". This will create an invalid path.
Looking at the CopyDirectory method (line 398), it creates currentDir = Path.Combine(destinationDir, dir.Name), which means the source directory is copied as a subdirectory with its name, not its full path.
Recommendation: Use only the directory name, not the full path:
string dirName = Path.GetFileName(tspLocation);
string tempTspLocation = Path.Combine(tempDirPath, dirName, "tspconfig.yaml");| string tempTspLocation = Path.Combine(tempDirPath, tspLocation, "tspconfig.yaml"); | |
| string dirName = Path.GetFileName(tspLocation); | |
| string tempTspLocation = Path.Combine(tempDirPath, dirName, "tspconfig.yaml"); |
| file.CopyTo(tempPath, false); | ||
| } | ||
| foreach (DirectoryInfo subdir in dirs) | ||
| { |
There was a problem hiding this comment.
The CopyDirectory method only excludes files based on their names (line 404), but doesn't check directory names against the exclude list. This means directories named "tsp-output" or "node_modules" will still be copied recursively.
Recommendation: Add directory exclusion check:
foreach (DirectoryInfo subdir in dirs)
{
if (exclude != null && Array.Exists(exclude, e => e.Equals(subdir.Name, StringComparison.OrdinalIgnoreCase)))
{
continue;
}
CopyDirectory(subdir.FullName, currentDir, exclude);
}| { | |
| { | |
| if (exclude != null && Array.Exists(exclude, e => e.Equals(subdir.Name, StringComparison.OrdinalIgnoreCase))) | |
| { | |
| continue; | |
| } |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| for (int i = 0; i < segments.Length; i++) | ||
| { | ||
| string segment = segments[i]; | ||
| if (segment[0] == '{' && segment[^1] == '}') |
There was a problem hiding this comment.
Potential index range error: Using segment[^1] (index from end) requires C# 8.0+ and .NET Standard 2.1+. If this project targets .NET Standard 2.0 or lower, this will cause a compilation error. Verify the target framework or use:
if (segment[0] == '{' && segment[segment.Length - 1] == '}')
{
string key = segment.Substring(1, segment.Length - 2);This is already done correctly on line 466, so line 464 should use the same pattern.
| if (segment[0] == '{' && segment[^1] == '}') | |
| if (segment[0] == '{' && segment[segment.Length - 1] == '}') |
|
|
||
| private string FindNPMCommandFromPath(string command) | ||
| { | ||
| if (_npmPath == "" || !File.Exists(_npmPath)) |
There was a problem hiding this comment.
The comparison _npmPath == "" should use string.IsNullOrEmpty(_npmPath) for consistency with the rest of the codebase and to handle null cases properly.
| if (_npmPath == "" || !File.Exists(_npmPath)) | |
| if (string.IsNullOrEmpty(_npmPath) || !File.Exists(_npmPath)) |
| throw new FileNotFoundException($"package.json not found in {workingDirectory}"); | ||
| } | ||
| string args = File.Exists(Path.Combine(workingDirectory, "package-lock.json")) ? "ci" : "install"; | ||
| await RunCommand(FindNPMCommandFromPath("npm.cmd"), args, workingDirectory); |
There was a problem hiding this comment.
The hardcoded npm.cmd command is Windows-specific and will fail on Unix-like systems. See related comment on line 262 for the same issue with tsp.cmd.
| { | ||
| throw new ArgumentException($"No emitter-output-dir configured in {TSPLocation}"); | ||
| } | ||
| // if emitter-outout-dir is not absolute, assume it's relative to RepoRoot |
There was a problem hiding this comment.
Typo in the comment: "emitter-outout-dir" should be "emitter-output-dir" (missing the 'p').
| // if emitter-outout-dir is not absolute, assume it's relative to RepoRoot | |
| // if emitter-output-dir is not absolute, assume it's relative to RepoRoot |
| private const string UriRegex = "^https://(?<urlRoot>github|raw.githubusercontent).com/(?<repo>[^/]*/azure-rest-api-specs(-pr)?)/(tree/|blob/)?(?<commit>[0-9a-f]{40})/(?<path>.*)/tspconfig.yaml$"; | ||
|
|
||
| private const string emitterName = "@azure-tools/typespec-powershell"; | ||
|
|
There was a problem hiding this comment.
Magic string: The tempDirName constant "TempTypeSpecFiles" should be documented with its purpose. Also consider if this name could conflict with user directories or cause issues on case-sensitive file systems.
| /// <summary> | |
| /// Name of the temporary directory used to store intermediate TypeSpec files during module update operations. | |
| /// This directory is created in a controlled location and is intended to be cleaned up after use. | |
| /// The name "TempTypeSpecFiles" was chosen for clarity, but developers should be aware of potential | |
| /// conflicts if a directory with the same name exists in the target location, especially on case-sensitive file systems. | |
| /// Consider updating the logic to use a more unique name if this becomes an issue. | |
| /// </summary> |
|
|
||
| ## Prerequisite | ||
| - **node version >= 20** | ||
| - **typespec compiler installed?**: `npm install -g @typespec/compiler` |
There was a problem hiding this comment.
The question mark makes the documentation unclear. This should be a statement, not a question. Consider: "- typespec compiler installed: npm install -g @typespec/compiler"
| - **typespec compiler installed?**: `npm install -g @typespec/compiler` | |
| - **typespec compiler installed**: `npm install -g @typespec/compiler` |
| this.MyInvocation.BoundParameters.ContainsKey(nameof(RemoteRepositoryName)) || | ||
| this.MyInvocation.BoundParameters.ContainsKey(nameof(RemoteForkName))) | ||
| { | ||
| throw new ArgumentException("Please do not provide `-RemoteDirectory`, `-RemoteCommit`, `-RemoteRepository` or `-RemoteForkName` along with `-TSPLocation`."); |
There was a problem hiding this comment.
Error message refers to -RemoteRepository but the actual parameter name is -RemoteRepositoryName (line 73). This inconsistency will confuse users trying to follow the error message guidance.
| throw new ArgumentException("Please do not provide `-RemoteDirectory`, `-RemoteCommit`, `-RemoteRepository` or `-RemoteForkName` along with `-TSPLocation`."); | |
| throw new ArgumentException("Please do not provide `-RemoteDirectory`, `-RemoteCommit`, `-RemoteRepositoryName` or `-RemoteForkName` along with `-TSPLocation`."); |
| { | ||
| private static readonly HttpClient httpClient = new HttpClient(); | ||
|
|
||
| private const string UriRegex = "^https://(?<urlRoot>github|raw.githubusercontent).com/(?<repo>[^/]*/azure-rest-api-specs(-pr)?)/(tree/|blob/)?(?<commit>[0-9a-f]{40})/(?<path>.*)/tspconfig.yaml$"; |
There was a problem hiding this comment.
Magic string: The URI regex pattern is complex and hardcoded. Consider extracting it as a documented constant with an explanation of what it matches. The pattern expects exactly 40 character commit hashes (SHA-1), which could become outdated if GitHub moves to SHA-256 hashes.
Consider:
// Matches GitHub URLs pointing to tspconfig.yaml files with 40-char commit hashes
// Examples:
// - https://github.com/Azure/azure-rest-api-specs/blob/abc123.../path/tspconfig.yaml
// - https://raw.githubusercontent.com/Azure/azure-rest-api-specs/abc123.../path/tspconfig.yaml
private const string UriRegex = "^https://(?<urlRoot>github|raw.githubusercontent).com/(?<repo>[^/]*/azure-rest-api-specs(-pr)?)/(tree/|blob/)?(?<commit>[0-9a-f]{40})/(?<path>.*)/tspconfig.yaml$";| private const string UriRegex = "^https://(?<urlRoot>github|raw.githubusercontent).com/(?<repo>[^/]*/azure-rest-api-specs(-pr)?)/(tree/|blob/)?(?<commit>[0-9a-f]{40})/(?<path>.*)/tspconfig.yaml$"; | |
| /// <summary> | |
| /// Regex pattern to match GitHub URLs pointing to tspconfig.yaml files with 40-character commit hashes (SHA-1). | |
| /// | |
| /// Examples: | |
| /// - https://github.com/Azure/azure-rest-api-specs/blob/abc123.../path/tspconfig.yaml | |
| /// - https://raw.githubusercontent.com/Azure/azure-rest-api-specs/abc123.../path/tspconfig.yaml | |
| /// | |
| /// Note: The pattern expects exactly 40-character commit hashes (SHA-1). If GitHub moves to SHA-256 hashes (64 characters), | |
| /// this pattern will need to be updated. | |
| /// </summary> | |
| private const string UriRegex = @"^https://(?<urlRoot>github|raw\.githubusercontent)\.com/(?<repo>[^/]*/azure-rest-api-specs(-pr)?)/(tree/|blob/)?(?<commit>[0-9a-f]{40})/(?<path>.*)/tspconfig\.yaml$"; |
| [Cmdlet("Update", "DevTSPModule")] | ||
| public class UpdateTSPModuleCmdlet : DevCmdletBase | ||
| { | ||
| private static readonly HttpClient httpClient = new HttpClient(); |
There was a problem hiding this comment.
The static HttpClient instance is good for performance, but it's not being disposed properly. While HttpClient is designed to be long-lived and reused, in a cmdlet scenario where the module may be unloaded, there's no cleanup mechanism. Consider implementing IDisposable or using a different pattern that ensures proper resource cleanup when the module is unloaded.
There was a problem hiding this comment.
@copilot see line "using var response = await httpClient.SendAsync(request, cts.Token);" isn't it properly handle?
tools/AzDev/build.ps1
Outdated
| $module = 'AzDev' | ||
| $artifacts = "$PSScriptRoot/../../artifacts" | ||
|
|
||
| rm -r "$artifacts/$module" |
There was a problem hiding this comment.
The command rm is an alias that may not be available on all systems. Per the repository's cross-platform compatibility guidelines, use the explicit Remove-Item cmdlet for PowerShell 5.1+ compatibility:
Remove-Item -Path "$artifacts/$module" -Recurse -Force -ErrorAction SilentlyContinue
| 3. no -tsplocation and there is a tsp-location.yaml, use tsp-location.yaml as -tsplocation | ||
| 4. if -tspconfig provided, merge it with -tsplocation | ||
| */ | ||
| [Cmdlet("Update", "DevTSPModule")] |
There was a problem hiding this comment.
I prefer to use the verb "set" instead of "update", since this cmdlet is used for both initializing and updating a PS module.
| [Parameter(HelpMessage = "The fork name of the remote repository where the TSP config is located. Do not use this parameter along with `-TSPLocation` and `-RemoteRepository`.")] | ||
| public string RemoteForkName { get; set; } | ||
|
|
||
| [Parameter(HelpMessage = "The path to an additional TSP config file to merge with the main TSP config. Will look for `tspconfig.yaml` in current directory if not provided.")] |
There was a problem hiding this comment.
Make sure the changes to protect tspconfig.yaml are merged into the autorest.powershell repository before merging this PR.
dolauli
left a comment
dolauli
left a comment
There was a problem hiding this comment.
Overall, the PR looks good to me, aside from the two minor issues I mentioned in the comments.
isra-fel
left a comment
isra-fel
left a comment
There was a problem hiding this comment.
Do we want to name TSP modules ***.TSP like ***.AutoRest? Could reduce a bit confusion but needs to update lots of scripts
There was a problem hiding this comment.
Consider checking in sample configs to AzDev
There was a problem hiding this comment.
have one remote sample config, please find links in README.md
tools/AzDev/src/Typespec/README.md
Outdated
There was a problem hiding this comment.
From which directory should I run the command?
There was a problem hiding this comment.
any directory under azure-powershell should work, or anywhere under your file system with reporoot provided
tools/AzDev/build.ps1
Outdated
| $module = 'AzDev' | ||
| $artifacts = "$PSScriptRoot/../../artifacts" | ||
|
|
||
| rm -r "$artifacts/$module" |
Yes, update scripts/pipelines could be a heavy work, we shall plan it separately. |
VeryEarly
changed the title
Update-DevTSPModuleNew-DevTSPModule
| private string FindNPMCommandFromPath(string command) | ||
| { | ||
| 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."); | ||
| } | ||
| return commandPath; | ||
| } |
There was a problem hiding this comment.
Windows-specific path handling. On line 304, the code checks for Windows platform but uses hardcoded empty string for non-Windows platforms. This may not work correctly on all Unix-like systems. Consider using proper cross-platform path resolution for npm commands.
| string pathEnv = Environment.GetEnvironmentVariable("PATH") ?? string.Empty; | ||
| string npmPath = pathEnv.Split(Path.PathSeparator).FirstOrDefault(path => path.EndsWith("npm")); | ||
| _npmPath = npmPath; | ||
| } |
There was a problem hiding this comment.
The logic for finding npm in PATH is fragile. Line 308 searches for paths ending with "npm", but this may incorrectly match directories that happen to end with "npm" rather than the actual npm directory. Consider using a more robust approach like checking for the actual npm executable.
| if (!string.IsNullOrEmpty(RemoteRepositoryName) && !string.IsNullOrEmpty(RemoteCommit) && !string.IsNullOrEmpty(RemoteDirectory)) | ||
| { | ||
| TSPLocation = new UriBuilder | ||
| { | ||
| Scheme = "https", | ||
| Host = "raw.githubusercontent.com", | ||
| Path = $"{RemoteRepositoryName ?? "Azure/azure-rest-api-specs"}/{RemoteCommit ?? "main"}/{RemoteDirectory ?? ""}/tspconfig.yaml" | ||
| }.ToString(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential argument validation issue. The code builds a URI on lines 134-139 but doesn't validate that all three required parameters (RemoteRepositoryName, RemoteCommit, RemoteDirectory) are non-null before constructing it. If any one is missing, the condition on line 132 won't be met, but the null coalescing operators inside UriBuilder will use fallback values that may not be intended. Consider validating all three are provided together or none at all.
| try | ||
| { | ||
| context = ContextProvider.LoadContext(); | ||
| } | ||
| catch | ||
| { | ||
| context = null; | ||
| } |
There was a problem hiding this comment.
Broad exception catch without logging. Lines 197-204 catch all exceptions when loading context but don't log the exception details. This makes debugging difficult. Consider at least logging the exception with WriteWarning or WriteVerbose so developers can understand why context loading failed.
| [Cmdlet("New", "DevTSPModule")] | ||
| public class NewTSPModuleCmdlet : DevCmdletBase | ||
| { | ||
| private static readonly HttpClient httpClient = new HttpClient(); |
There was a problem hiding this comment.
Static HttpClient instance without proper lifecycle management. While this is generally acceptable for console/cmdlet applications with short lifecycles, consider adding proper disposal if the cmdlet might be used in long-running PowerShell sessions to prevent socket exhaustion.
| cd "D:/workspace/azure-powershell/src/DependencyMap/DependencyMap.Autorest" | ||
| New-DevTSPModule | ||
| ``` | ||
|
|
||
| ### Use tsp-location.yaml with updated commit and fork When last time generated from remote | ||
| ```powershell | ||
| cd "D:/workspace/azure-powershell/src/DependencyMap/DependencyMap.Autorest" | ||
| New-DevTSPModule -RemoteForkName "VeryEarly" -RemoteCommit "e952eed8b787d99d10ba9a5ea3789ed0a9877214" | ||
| ``` | ||
|
|
||
| ### Use local `-AzPSConfig` to override or extend `tspconfig.yaml` from `-TSPLocation` | ||
| ```powershell | ||
| New-DevTSPModule -TSPLocation "https://raw.githubusercontent.com/Azure/azure-rest-api-specs/29e9e3ca1a1bccba66a6cf092dbc317c639989b1/specification/azuredependencymap/DependencyMap.Management/tspconfig.yaml" -AzPSConfig "D:/workspace/azure-powershell/src/DependencyMap/DependencyMap.Autorest/tspconfig.yaml" | ||
| ``` | ||
|
|
||
| ### Use local `-AzPSConfig` to override or extend `tspconfig.yaml` from `tsp-location.yaml` both under current directory | ||
| ```powershell | ||
| cd "D:/workspace/azure-powershell/src/DependencyMap/DependencyMap.Autorest" | ||
| New-DevTSPModule | ||
| ``` | ||
|
|
||
| ### Execute `New-DevTSPModule` from directories outside of azure-powershell | ||
| ```powershell | ||
| cd "C:/" | ||
| New-DevTSPModule -TSPLocation "https://raw.githubusercontent.com/Azure/azure-rest-api-specs/29e9e3ca1a1bccba66a6cf092dbc317c639989b1/specification/azuredependencymap/DependencyMap.Management/tspconfig.yaml" -RepoRoot "D:/workspace/azure-powershell" | ||
| ``` | ||
| Or | ||
|
|
||
| ```powershell | ||
| cd "C:/" | ||
| Set-DevContext -RepoRoot "D:/workspace/azure-powershell" | ||
| New-DevTSPModule -TSPLocation "https://raw.githubusercontent.com/Azure/azure-rest-api-specs/29e9e3ca1a1bccba66a6cf092dbc317c639989b1/specification/azuredependencymap/DependencyMap.Management/tspconfig.yaml" | ||
| ``` |
There was a problem hiding this comment.
Examples use Windows-style paths with D: drives and backslashes. For better cross-platform documentation (per CodingGuidelineID 1000002), consider using forward slashes which work on all platforms, or provide both Windows and Unix examples.
| process.ErrorDataReceived += (sender, e) => | ||
| { | ||
| if (!string.IsNullOrEmpty(e.Data)) | ||
| Console.Error.WriteLine(e.Data); |
There was a problem hiding this comment.
Console.Error.WriteLine is used instead of PowerShell's WriteError or WriteWarning. For PowerShell cmdlets, use WriteError to properly integrate with PowerShell's error handling pipeline and allow proper error stream redirection.
| if (string.IsNullOrEmpty(segment)) | ||
| { | ||
| continue; | ||
| } | ||
| resolvedDir.Append(segment); | ||
| if (i < segments.Length - 1) | ||
| { | ||
| resolvedDir.Append(Path.DirectorySeparatorChar); | ||
| } | ||
|
|
There was a problem hiding this comment.
Incomplete path resolution logic. When a segment is empty after resolution (line 494-497), the code continues without appending the separator, but then on line 499-502 it still checks if it's not the last segment to add a separator. This could result in double separators or missing separators in edge cases. Consider consolidating the logic or adding the separator only when a non-empty segment is appended.
| if (Directory.Exists(tempDirPath)) | ||
| { | ||
| ForceDeleteDir(tempDirPath); | ||
| } | ||
| Directory.CreateDirectory(tempDirPath); |
There was a problem hiding this comment.
Race condition in temporary directory handling. Between checking Directory.Exists on line 368 and calling ForceDeleteDir on line 370, another process could create the directory. While unlikely in practice, consider using a try-catch around the deletion or using a unique temporary directory name to avoid conflicts.
| await RunCommand("git", $"clone {cloneRepo} {tempDirPath} --no-checkout --filter=tree:0", outDir); | ||
| await RunCommand("git", $"sparse-checkout set {path}", tempDirPath); | ||
| await RunCommand("git", $"sparse-checkout add {path}", tempDirPath); | ||
| await RunCommand("git", $"checkout {commit}", tempDirPath); |
There was a problem hiding this comment.
Security concern: Command injection vulnerability. Line 379 directly interpolates user-controlled variables into git command arguments without sanitization. An attacker could potentially inject malicious git commands through RemoteRepositoryName or other parameters. Consider using Process.ArgumentList or properly escaping/validating the parameters.
3 participants
Description
instruction: https://github.com/VeryEarly/azure-powershell/blob/yabo/tsp-client/tools/AzDev/src/Typespec/README.md
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.