Add support for --target-api-url to rewire-pipeline command
#1366
Conversation
|
The code for this was written for me by Copilot Agent mode in VS but I was able to test and verify that it worked as exptected. |
There was a problem hiding this comment.
Pull Request Overview
Adds a new --target-api-url option to the rewire-pipeline command, passes it through the handler into the ADO API call, and updates AdoApi.ChangePipelineRepo to construct repo URLs against a custom API endpoint when provided.
- Introduce
TargetApiUrloption in CLI command and args model - Wire
args.TargetApiUrlthroughRewirePipelineCommandHandlerinto the API call - Extend
AdoApi.ChangePipelineRepoto branch ontargetApiUrlfor URL construction and update tests
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ado2gh/Commands/RewirePipeline/RewirePipelineCommand.cs | Add --target-api-url option without a default value |
| src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandArgs.cs | Add TargetApiUrl property |
| src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs | Pass args.TargetApiUrl to ChangePipelineRepo |
| src/OctoshiftCLI.Tests/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandlerTests.cs | Update happy-path to expect null and add a test for non-null URL |
| src/Octoshift/Services/AdoApi.cs | Implement targetApiUrl logic for building repo URLs |
Comments suppressed due to low confidence (1)
src/ado2gh/Commands/RewirePipeline/RewirePipelineCommand.cs:56
- [nitpick] The CLI option description mentions a default of https://api.github.com, but no default is actually set on the option. Consider specifying a default value on the
Option<string>itself to ensure consistent behavior when the flag is omitted.
public Option<string> TargetApiUrl { get; } = new("--target-api-url")
| string apiUrl, webUrl, cloneUrl, branchesUrl, refsUrl, manageUrl; | ||
| if (!string.IsNullOrWhiteSpace(targetApiUrl)) | ||
| { | ||
| var apiUri = new Uri(targetApiUrl.TrimEnd('/')); |
There was a problem hiding this comment.
Consider validating the targetApiUrl string (e.g. with Uri.TryCreate) before constructing a Uri, and provide a clear error message if it's invalid, to avoid runtime UriFormatException.
| var apiUri = new Uri(targetApiUrl.TrimEnd('/')); | |
| if (!Uri.TryCreate(targetApiUrl.TrimEnd('/'), UriKind.Absolute, out var apiUri)) | |
| { | |
| throw new ArgumentException($"The provided targetApiUrl '{targetApiUrl}' is not a valid absolute URI.", nameof(targetApiUrl)); | |
| } |
Copilot uses AI. Check for mistakes.
Unit Test Results 1 files 1 suites 20s ⏱️ Results for commit e1646a1. ♻️ This comment has been updated with latest results. |
| orgName = githubOrg, | ||
| refsUrl = $"https://api.github.com/repos/{githubOrg.EscapeDataString()}/{githubRepo.EscapeDataString()}/git/refs", | ||
| refsUrl, | ||
| safeRepository = $"{githubOrg.EscapeDataString()}/{githubRepo.EscapeDataString()}", | ||
| shortName = githubRepo, | ||
| reportBuildStatus = true | ||
| }, | ||
| id = $"{githubOrg}/{githubRepo}", | ||
| type = "GitHub", |
There was a problem hiding this comment.
Even though this worked for me, I am not 100% certain this does not need to be a different value if the repository is on GHES or GHEC with Data Residency
src/Octoshift/Services/AdoApi.cs
Outdated
| { | ||
| var url = $"{_adoBaseUrl}/{adoOrg.EscapeDataString()}/{teamProject.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; | ||
|
|
||
| var response = await _client.GetAsync(url); | ||
| var data = JObject.Parse(response); | ||
|
|
||
| // Determine base URLs | ||
| string apiUrl, webUrl, cloneUrl, branchesUrl, refsUrl, manageUrl; | ||
| if (!string.IsNullOrWhiteSpace(targetApiUrl)) |
There was a problem hiding this comment.
Use our extension method HasValue() to do this check
| if (!string.IsNullOrWhiteSpace(targetApiUrl)) | |
| if (targetApiUrl.HasValue()) |
|
There'll need to be some unit tests for the changes in |
|
Add support for
--target-api-urlto therewire-pipelinecommand so that the command can be used when migrating repositories to GHEC with Data ResidencyThirdPartyNotices.txt(if applicable)Fixes Issue #1365