From 697730df17e87b87c14d26594a29ce958b22d003 Mon Sep 17 00:00:00 2001 From: Georgy Gorelko Date: Wed, 3 Dec 2025 15:12:11 -0800 Subject: [PATCH 1/8] Implement pipeline status checks and enhance error handling in ADO to GitHub migration - Added IsPipelineEnabled method to AdoApi to check if a pipeline is enabled, disabled, or paused. - Updated AdoPipelineTriggerService to skip rewiring if the repository is disabled or returns a 404. - Enhanced error handling in RewirePipelineToGitHub to return false when skipping due to disabled repositories. - Added tests for IsPipelineEnabled to verify behavior for enabled, disabled, paused, and missing queue status. - Updated tests in AdoPipelineTriggerService to ensure correct handling of disabled repositories and 404 responses. - Improved logging in RewirePipelineCommandArgs to include relevant properties while excluding MonitorTimeoutMinutes. - Set default value for MonitorTimeoutMinutes in RewirePipelineCommandArgs. --- RELEASENOTES.md | 7 +- src/Octoshift/Commands/CommandArgs.cs | 2 +- src/Octoshift/Services/AdoApi.cs | 12 + .../Services/AdoPipelineTriggerService.cs | 110 +++++++-- src/Octoshift/Services/PipelineTestService.cs | 10 + .../Octoshift/Services/AdoApiTests.cs | 75 ++++++ ...ipelineTriggerService_BranchPolicyTests.cs | 232 ++++++++++++++++-- ...pelineTriggerService_ErrorHandlingTests.cs | 29 ++- .../Services/PipelineTestServiceTests.cs | 49 ++++ ...pelineCommandHandler_ErrorHandlingTests.cs | 5 + .../RewirePipeline/RewirePipelineCommand.cs | 5 +- .../RewirePipelineCommandArgs.cs | 43 +++- .../RewirePipelineCommandHandler.cs | 9 +- 13 files changed, 528 insertions(+), 60 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 8b1378917..7c02957d6 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -1 +1,6 @@ - +- Fixed branch policy check for classic pipelines with prefixed repository names to use repository ID directly when available, preventing 404 errors during rewiring +- Skip branch policy checks for disabled repositories with a warning instead of attempting API calls that may fail +- Skip pipeline rewiring entirely for disabled repositories, exiting early with an appropriate warning message +- Fixed misleading success message that appeared even when pipeline rewiring was skipped for disabled repositories +- Fixed monitor timeout minutes to only display when --dry-run mode is enabled, reducing confusion during regular pipeline rewiring operations +- Check if pipeline is disabled before attempting to queue a test build, preventing 400 Bad Request errors and providing clear warning messages diff --git a/src/Octoshift/Commands/CommandArgs.cs b/src/Octoshift/Commands/CommandArgs.cs index a647ca2c8..34f8a4542 100644 --- a/src/Octoshift/Commands/CommandArgs.cs +++ b/src/Octoshift/Commands/CommandArgs.cs @@ -13,7 +13,7 @@ public abstract class CommandArgs public virtual void Validate(OctoLogger log) { } - public void Log(OctoLogger log) + public virtual void Log(OctoLogger log) { if (log is null) { diff --git a/src/Octoshift/Services/AdoApi.cs b/src/Octoshift/Services/AdoApi.cs index c7fb98494..ae5ba1763 100644 --- a/src/Octoshift/Services/AdoApi.cs +++ b/src/Octoshift/Services/AdoApi.cs @@ -561,6 +561,18 @@ public virtual async Task ShareServiceConnection(string adoOrg, string adoTeamPr return (defaultBranch, clean, checkoutSubmodules, triggers); } + public virtual async Task IsPipelineEnabled(string org, string teamProject, int pipelineId) + { + var url = $"{_adoBaseUrl}/{org.EscapeDataString()}/{teamProject.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; + + var response = await _client.GetAsync(url); + var data = JObject.Parse(response); + + // Check the queueStatus field - it can be "enabled", "disabled", or "paused" + var queueStatus = (string)data["queueStatus"]; + return string.IsNullOrEmpty(queueStatus) || queueStatus.Equals("enabled", StringComparison.OrdinalIgnoreCase); + } + public virtual async Task GetBoardsGithubRepoId(string org, string teamProject, string teamProjectId, string endpointId, string githubOrg, string githubRepo) { var url = $"{_adoBaseUrl}/{org.EscapeDataString()}/_apis/Contribution/HierarchyQuery?api-version=5.0-preview.1"; diff --git a/src/Octoshift/Services/AdoPipelineTriggerService.cs b/src/Octoshift/Services/AdoPipelineTriggerService.cs index 7f8c87b68..53362bc07 100644 --- a/src/Octoshift/Services/AdoPipelineTriggerService.cs +++ b/src/Octoshift/Services/AdoPipelineTriggerService.cs @@ -22,7 +22,7 @@ public class AdoPipelineTriggerService private readonly string _adoBaseUrl; // Cache for repository IDs and branch policies to avoid redundant API calls - private readonly Dictionary _repositoryIdCache = []; + private readonly Dictionary _repositoryCache = []; private readonly Dictionary _branchPolicyCache = []; public AdoPipelineTriggerService(AdoApi adoApi, OctoLogger log, string adoBaseUrl) @@ -36,7 +36,8 @@ public AdoPipelineTriggerService(AdoApi adoApi, OctoLogger log, string adoBaseUr /// Changes a pipeline's repository configuration from ADO to GitHub, applying /// trigger configuration based on branch policy requirements and existing settings. /// - public virtual async Task RewirePipelineToGitHub( + /// True if the pipeline was successfully rewired, false if it was skipped. + public virtual async Task RewirePipelineToGitHub( string adoOrg, string teamProject, int pipelineId, @@ -56,52 +57,93 @@ public virtual async Task RewirePipelineToGitHub( var response = await _adoApi.GetAsync(url); var data = JObject.Parse(response); - var newRepo = CreateGitHubRepositoryConfiguration(githubOrg, githubRepo, defaultBranch, clean, checkoutSubmodules, connectedServiceId, targetApiUrl); var currentRepoName = data["repository"]?["name"]?.ToString(); - var isPipelineRequiredByBranchPolicy = await IsPipelineRequiredByBranchPolicy(adoOrg, teamProject, currentRepoName, pipelineId); + var currentRepoId = data["repository"]?["id"]?.ToString(); + + // Check if repository is disabled - skip rewiring if it is + if (!string.IsNullOrEmpty(currentRepoId) || !string.IsNullOrEmpty(currentRepoName)) + { + var identifier = !string.IsNullOrEmpty(currentRepoId) ? currentRepoId : currentRepoName; + var (_, isDisabled) = await GetRepositoryInfoWithCache(adoOrg, teamProject, currentRepoId, currentRepoName); + + if (isDisabled) + { + _log.LogWarning($"Repository {adoOrg}/{teamProject}/{identifier} is disabled. Skipping pipeline rewiring for pipeline {pipelineId}."); + return false; + } + } + + var newRepo = CreateGitHubRepositoryConfiguration(githubOrg, githubRepo, defaultBranch, clean, checkoutSubmodules, connectedServiceId, targetApiUrl); + var isPipelineRequiredByBranchPolicy = await IsPipelineRequiredByBranchPolicy(adoOrg, teamProject, currentRepoName, currentRepoId, pipelineId); LogBranchPolicyCheckResults(pipelineId, isPipelineRequiredByBranchPolicy); var payload = BuildPipelinePayload(data, newRepo, originalTriggers, isPipelineRequiredByBranchPolicy); await _adoApi.PutAsync(url, payload.ToObject(typeof(object))); + return true; } catch (HttpRequestException ex) when (ex.Message.Contains("404")) { // Pipeline not found - log warning and skip _log.LogWarning($"Pipeline {pipelineId} not found in {adoOrg}/{teamProject}. Skipping pipeline rewiring."); - return; + return false; } catch (HttpRequestException ex) { // Other HTTP errors during pipeline retrieval _log.LogWarning($"HTTP error retrieving pipeline {pipelineId} in {adoOrg}/{teamProject}: {ex.Message}. Skipping pipeline rewiring."); - return; + return false; } } /// /// Analyzes branch policies to determine if a pipeline is required for branch protection. /// - public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string teamProject, string repoName, int pipelineId) + public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string teamProject, string repoName, string repoId, int pipelineId) { ArgumentNullException.ThrowIfNull(adoOrg); ArgumentNullException.ThrowIfNull(teamProject); - if (string.IsNullOrEmpty(repoName)) + if (string.IsNullOrEmpty(repoName) && string.IsNullOrEmpty(repoId)) { - _log.LogWarning($"Branch policy check skipped for pipeline {pipelineId} - repository name not available. Pipeline trigger configuration may not preserve branch policy requirements."); + _log.LogWarning($"Branch policy check skipped for pipeline {pipelineId} - repository name and ID not available. Pipeline trigger configuration may not preserve branch policy requirements."); return false; } try { - // Get repository information first (with caching) - var repositoryId = await GetRepositoryIdWithCache(adoOrg, teamProject, repoName); + // Use repository ID directly if available, otherwise look it up by name + string repositoryId; + var isRepositoryDisabled = false; + + if (!string.IsNullOrEmpty(repoId)) + { + _log.LogVerbose($"Using repository ID from pipeline definition for branch policy check: {repoId}"); + repositoryId = repoId; - if (string.IsNullOrEmpty(repositoryId)) + // Check if repository is disabled by fetching its details + var (_, disabled) = await GetRepositoryInfoWithCache(adoOrg, teamProject, repoId, repoName); + isRepositoryDisabled = disabled; + } + else { - _log.LogWarning($"Repository ID not found for {adoOrg}/{teamProject}/{repoName}. Branch policy check cannot be performed for pipeline {pipelineId}."); + // Get repository information by name (with caching) + var (id, disabled) = await GetRepositoryInfoWithCache(adoOrg, teamProject, null, repoName); + repositoryId = id; + isRepositoryDisabled = disabled; + + if (string.IsNullOrEmpty(repositoryId)) + { + _log.LogWarning($"Repository ID not found for {adoOrg}/{teamProject}/{repoName}. Branch policy check cannot be performed for pipeline {pipelineId}."); + return false; + } + } + + // Skip branch policy check if repository is disabled + if (isRepositoryDisabled) + { + _log.LogWarning($"Repository {adoOrg}/{teamProject}/{repoName} is disabled. Branch policy check skipped for pipeline {pipelineId}. Pipeline trigger configuration may not preserve branch policy requirements."); return false; } @@ -451,51 +493,65 @@ private bool HasTriggerType(JToken originalTriggers, string triggerType) #region Private Helper Methods - Caching /// - /// Gets the repository ID with caching to avoid redundant API calls for the same repository. + /// Gets the repository information (ID and disabled status) with caching to avoid redundant API calls for the same repository. /// - private async Task GetRepositoryIdWithCache(string adoOrg, string teamProject, string repoName) + private async Task<(string id, bool isDisabled)> GetRepositoryInfoWithCache(string adoOrg, string teamProject, string repoId, string repoName) { - var cacheKey = $"{adoOrg.ToUpper()}/{teamProject.ToUpper()}/{repoName.ToUpper()}"; + var identifier = !string.IsNullOrEmpty(repoId) ? repoId : repoName; + var cacheKey = $"{adoOrg.ToUpper()}/{teamProject.ToUpper()}/{identifier.ToUpper()}"; - if (_repositoryIdCache.TryGetValue(cacheKey, out var cachedId)) + if (_repositoryCache.TryGetValue(cacheKey, out var cachedInfo)) { - _log.LogVerbose($"Using cached repository ID for {adoOrg}/{teamProject}/{repoName}"); - return cachedId; + _log.LogVerbose($"Using cached repository information for {adoOrg}/{teamProject}/{identifier}"); + return cachedInfo; } - _log.LogVerbose($"Fetching repository ID for {adoOrg}/{teamProject}/{repoName}"); + _log.LogVerbose($"Fetching repository information for {adoOrg}/{teamProject}/{identifier}"); try { - var repoUrl = $"{_adoBaseUrl}/{adoOrg.EscapeDataString()}/{teamProject.EscapeDataString()}/_apis/git/repositories/{repoName.EscapeDataString()}?api-version=6.0"; + var repoUrl = $"{_adoBaseUrl}/{adoOrg.EscapeDataString()}/{teamProject.EscapeDataString()}/_apis/git/repositories/{identifier.EscapeDataString()}?api-version=6.0"; var repoResponse = await _adoApi.GetAsync(repoUrl); var repoData = JObject.Parse(repoResponse); var repositoryId = repoData["id"]?.ToString(); + var isDisabled = repoData["isDisabled"]?.ToString().Equals("true", StringComparison.OrdinalIgnoreCase) ?? false; if (!string.IsNullOrEmpty(repositoryId)) { - _repositoryIdCache[cacheKey] = repositoryId; - _log.LogVerbose($"Cached repository ID {repositoryId} for {adoOrg}/{teamProject}/{repoName}"); + var info = (repositoryId, isDisabled); + _repositoryCache[cacheKey] = info; + _log.LogVerbose($"Cached repository information (ID: {repositoryId}, Disabled: {isDisabled}) for {adoOrg}/{teamProject}/{identifier}"); + return info; } - return repositoryId; + return (null, false); + } + catch (HttpRequestException ex) when (ex.Message.Contains("404")) + { + // 404 typically means the repository is disabled or doesn't exist + // Treat it as disabled to avoid further API calls + // Log as verbose since the caller will log a more specific warning about the disabled repository + _log.LogVerbose($"Repository {adoOrg}/{teamProject}/{identifier} returned 404 - likely disabled or not found."); + var info = (identifier, true); // Mark as disabled + _repositoryCache[cacheKey] = info; + return info; } catch (HttpRequestException ex) { // Don't cache failed requests - let the caller handle the error - _log.LogVerbose($"Failed to fetch repository ID for {adoOrg}/{teamProject}/{repoName}: {ex.Message}"); + _log.LogVerbose($"Failed to fetch repository information for {adoOrg}/{teamProject}/{identifier}: {ex.Message}"); throw; } catch (TaskCanceledException ex) { // Don't cache timeouts - let the caller handle the error - _log.LogVerbose($"Timeout fetching repository ID for {adoOrg}/{teamProject}/{repoName}: {ex.Message}"); + _log.LogVerbose($"Timeout fetching repository information for {adoOrg}/{teamProject}/{identifier}: {ex.Message}"); throw; } catch (JsonException ex) { // Don't cache JSON parsing errors - let the caller handle the error - _log.LogVerbose($"JSON parsing error for repository {adoOrg}/{teamProject}/{repoName}: {ex.Message}"); + _log.LogVerbose($"JSON parsing error for repository {adoOrg}/{teamProject}/{identifier}: {ex.Message}"); throw; } } diff --git a/src/Octoshift/Services/PipelineTestService.cs b/src/Octoshift/Services/PipelineTestService.cs index e67f24f93..1910ba57e 100644 --- a/src/Octoshift/Services/PipelineTestService.cs +++ b/src/Octoshift/Services/PipelineTestService.cs @@ -59,6 +59,16 @@ public async Task TestPipeline(PipelineTestArgs args) args.PipelineId = pipelineId; } + // Check if pipeline is disabled before attempting to queue a build + var isEnabled = await _adoApi.IsPipelineEnabled(args.AdoOrg, args.AdoTeamProject, args.PipelineId.Value); + if (!isEnabled) + { + _log.LogWarning($"Pipeline '{args.PipelineName}' (ID: {args.PipelineId.Value}) is disabled. Skipping pipeline test."); + testResult.ErrorMessage = "Pipeline is disabled"; + testResult.EndTime = DateTime.UtcNow; + return testResult; + } + // Get original repository information for restoration (originalRepoName, _, originalDefaultBranch, originalClean, originalCheckoutSubmodules) = await _adoApi.GetPipelineRepository(args.AdoOrg, args.AdoTeamProject, args.PipelineId.Value); diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/AdoApiTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/AdoApiTests.cs index e2d6eb9ab..c162807ef 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/AdoApiTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/AdoApiTests.cs @@ -985,6 +985,81 @@ public async Task GetPipeline_Should_Return_Pipeline() Triggers.Should().NotBeNull(); } + [Fact] + public async Task IsPipelineEnabled_Should_Return_True_For_Enabled_Pipeline() + { + var pipelineId = 826263; + + var endpoint = $"https://dev.azure.com/{ADO_ORG.EscapeDataString()}/{ADO_TEAM_PROJECT.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; + var response = new + { + id = pipelineId, + queueStatus = "enabled" + }; + + _mockAdoClient.Setup(x => x.GetAsync(endpoint).Result).Returns(response.ToJson()); + + var result = await sut.IsPipelineEnabled(ADO_ORG, ADO_TEAM_PROJECT, pipelineId); + + result.Should().BeTrue(); + } + + [Fact] + public async Task IsPipelineEnabled_Should_Return_False_For_Disabled_Pipeline() + { + var pipelineId = 826263; + + var endpoint = $"https://dev.azure.com/{ADO_ORG.EscapeDataString()}/{ADO_TEAM_PROJECT.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; + var response = new + { + id = pipelineId, + queueStatus = "disabled" + }; + + _mockAdoClient.Setup(x => x.GetAsync(endpoint).Result).Returns(response.ToJson()); + + var result = await sut.IsPipelineEnabled(ADO_ORG, ADO_TEAM_PROJECT, pipelineId); + + result.Should().BeFalse(); + } + + [Fact] + public async Task IsPipelineEnabled_Should_Return_False_For_Paused_Pipeline() + { + var pipelineId = 826263; + + var endpoint = $"https://dev.azure.com/{ADO_ORG.EscapeDataString()}/{ADO_TEAM_PROJECT.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; + var response = new + { + id = pipelineId, + queueStatus = "paused" + }; + + _mockAdoClient.Setup(x => x.GetAsync(endpoint).Result).Returns(response.ToJson()); + + var result = await sut.IsPipelineEnabled(ADO_ORG, ADO_TEAM_PROJECT, pipelineId); + + result.Should().BeFalse(); + } + + [Fact] + public async Task IsPipelineEnabled_Should_Return_True_For_Missing_QueueStatus() + { + var pipelineId = 826263; + + var endpoint = $"https://dev.azure.com/{ADO_ORG.EscapeDataString()}/{ADO_TEAM_PROJECT.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; + var response = new + { + id = pipelineId + }; + + _mockAdoClient.Setup(x => x.GetAsync(endpoint).Result).Returns(response.ToJson()); + + var result = await sut.IsPipelineEnabled(ADO_ORG, ADO_TEAM_PROJECT, pipelineId); + + result.Should().BeTrue(); + } + [Fact] public async Task GetBoardsGithubRepoId_Should_Return_RepoId() { diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_BranchPolicyTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_BranchPolicyTests.cs index 7b789d188..fbee8d727 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_BranchPolicyTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_BranchPolicyTests.cs @@ -1,5 +1,6 @@ using System; using System.Linq; +using System.Net.Http; using System.Threading.Tasks; using FluentAssertions; using Moq; @@ -36,7 +37,8 @@ public async Task IsPipelineRequiredByBranchPolicy_Should_Return_True_When_Pipel var repoResponse = new { id = repositoryId, - name = REPO_NAME + name = REPO_NAME, + isDisabled = "false" }.ToJson(); var policyResponse = new @@ -70,12 +72,67 @@ public async Task IsPipelineRequiredByBranchPolicy_Should_Return_True_When_Pipel _mockAdoApi.Setup(m => m.GetAsync(policyUrl)).ReturnsAsync(policyResponse); // Act - var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, PIPELINE_ID); + var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, null, PIPELINE_ID); // Assert result.Should().BeTrue(); } + [Fact] + public async Task IsPipelineRequiredByBranchPolicy_Should_Use_Repository_Id_Directly_When_Provided() + { + // Arrange + var repositoryId = Guid.NewGuid().ToString(); + + var repoResponse = new + { + id = repositoryId, + name = REPO_NAME, + isDisabled = "false" + }.ToJson(); + + var policyResponse = new + { + count = 1, + value = new[] + { + new + { + id = 1, + type = new + { + id = "0609b952-1397-4640-95ec-e00a01b2c241", + displayName = "Build" + }, + isEnabled = true, + settings = new + { + buildDefinitionId = PIPELINE_ID, + displayName = PIPELINE_NAME, + validDuration = 0 + } + } + } + }.ToJson(); + + // When repository ID is provided, we still need to fetch repo details to check if it's disabled + var repoByIdUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{repositoryId.EscapeDataString()}?api-version=6.0"; + var policyUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/policy/configurations?repositoryId={repositoryId}&api-version=6.0"; + + _mockAdoApi.Setup(m => m.GetAsync(repoByIdUrl)).ReturnsAsync(repoResponse); + _mockAdoApi.Setup(m => m.GetAsync(policyUrl)).ReturnsAsync(policyResponse); + + // Act - Pass repository ID directly instead of name + var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, repositoryId, PIPELINE_ID); + + // Assert + result.Should().BeTrue(); + + // Verify that repository lookup by name was NOT called since we provided the ID + var repoByNameUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{REPO_NAME.EscapeDataString()}?api-version=6.0"; + _mockAdoApi.Verify(m => m.GetAsync(repoByNameUrl), Times.Never); + } + [Fact] public async Task IsPipelineRequiredByBranchPolicy_Should_Return_False_When_Pipeline_Not_In_Policy() { @@ -84,7 +141,8 @@ public async Task IsPipelineRequiredByBranchPolicy_Should_Return_False_When_Pipe var repoResponse = new { id = repositoryId, - name = REPO_NAME + name = REPO_NAME, + isDisabled = "false" }.ToJson(); var policyResponse = new @@ -118,7 +176,7 @@ public async Task IsPipelineRequiredByBranchPolicy_Should_Return_False_When_Pipe _mockAdoApi.Setup(m => m.GetAsync(policyUrl)).ReturnsAsync(policyResponse); // Act - var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, PIPELINE_ID); + var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, null, PIPELINE_ID); // Assert result.Should().BeFalse(); @@ -132,7 +190,8 @@ public async Task IsPipelineRequiredByBranchPolicy_Should_Return_False_When_Poli var repoResponse = new { id = repositoryId, - name = REPO_NAME + name = REPO_NAME, + isDisabled = "false" }.ToJson(); var policyResponse = new @@ -166,7 +225,7 @@ public async Task IsPipelineRequiredByBranchPolicy_Should_Return_False_When_Poli _mockAdoApi.Setup(m => m.GetAsync(policyUrl)).ReturnsAsync(policyResponse); // Act - var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, PIPELINE_ID); + var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, null, PIPELINE_ID); // Assert result.Should().BeFalse(); @@ -180,7 +239,8 @@ public async Task IsPipelineRequiredByBranchPolicy_Should_Return_False_When_No_B var repoResponse = new { id = repositoryId, - name = REPO_NAME + name = REPO_NAME, + isDisabled = "false" }.ToJson(); var policyResponse = new @@ -196,10 +256,56 @@ public async Task IsPipelineRequiredByBranchPolicy_Should_Return_False_When_No_B _mockAdoApi.Setup(m => m.GetAsync(policyUrl)).ReturnsAsync(policyResponse); // Act - var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, PIPELINE_ID); + var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, null, PIPELINE_ID); + + // Assert + result.Should().BeFalse(); + } + + [Fact] + public async Task IsPipelineRequiredByBranchPolicy_Should_Return_False_When_Repository_Is_Disabled() + { + // Arrange + var repositoryId = Guid.NewGuid().ToString(); + var repoResponse = new + { + id = repositoryId, + name = REPO_NAME, + isDisabled = "true" + }.ToJson(); + + var repoUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{REPO_NAME.EscapeDataString()}?api-version=6.0"; + + _mockAdoApi.Setup(m => m.GetAsync(repoUrl)).ReturnsAsync(repoResponse); + + // Act + var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, null, PIPELINE_ID); + + // Assert + result.Should().BeFalse(); + + // Verify that branch policy check was NOT performed since repository is disabled + var policyUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/policy/configurations?repositoryId={repositoryId}&api-version=6.0"; + _mockAdoApi.Verify(m => m.GetAsync(policyUrl), Times.Never); + } + + [Fact] + public async Task IsPipelineRequiredByBranchPolicy_Should_Return_False_When_Repository_Returns_404() + { + // Arrange - Disabled repositories often return 404 when queried directly + var repoUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{REPO_NAME.EscapeDataString()}?api-version=6.0"; + + _mockAdoApi.Setup(m => m.GetAsync(repoUrl)) + .ThrowsAsync(new HttpRequestException("Response status code does not indicate success: 404 (Not Found).")); + + // Act + var result = await _triggerService.IsPipelineRequiredByBranchPolicy(ADO_ORG, TEAM_PROJECT, REPO_NAME, null, PIPELINE_ID); // Assert result.Should().BeFalse(); + + // Verify that branch policy check was NOT performed since repository returned 404 + _mockAdoApi.Verify(m => m.GetAsync(It.Is(url => url.Contains("policy/configurations"))), Times.Never); } [Fact] @@ -235,8 +341,8 @@ public async Task RewirePipelineToGitHub_WhenPipelineNotRequiredByBranchPolicy_S // Mock repository lookup - return valid repository var repositoryId = "repo-123"; - var repoResponse = new { id = repositoryId, name = REPO_NAME }.ToJson(); - var repoUrl = $"/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{REPO_NAME.EscapeDataString()}?api-version=6.0"; + var repoResponse = new { id = repositoryId, name = REPO_NAME, isDisabled = "false" }.ToJson(); + var repoUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{REPO_NAME.EscapeDataString()}?api-version=6.0"; // Mock branch policies - return empty policies (not required by branch policy) var policies = new @@ -244,7 +350,7 @@ public async Task RewirePipelineToGitHub_WhenPipelineNotRequiredByBranchPolicy_S count = 0, value = Array.Empty() }.ToJson(); - var policyUrl = $"/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/policy/configurations?repositoryId={repositoryId}&api-version=6.0"; + var policyUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/policy/configurations?repositoryId={repositoryId}&api-version=6.0"; var pipelineUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; @@ -253,9 +359,10 @@ public async Task RewirePipelineToGitHub_WhenPipelineNotRequiredByBranchPolicy_S _mockAdoApi.Setup(m => m.GetAsync(policyUrl)).ReturnsAsync(policies); // Act - await _triggerService.RewirePipelineToGitHub(ADO_ORG, TEAM_PROJECT, pipelineId, defaultBranch, clean, checkoutSubmodules, "github-org", githubRepo, serviceConnectionId, originalTriggers, null); + var result = await _triggerService.RewirePipelineToGitHub(ADO_ORG, TEAM_PROJECT, pipelineId, defaultBranch, clean, checkoutSubmodules, "github-org", githubRepo, serviceConnectionId, originalTriggers, null); // Assert - Should preserve original triggers (both CI and PR, with build status reporting) + result.Should().BeTrue(); _mockAdoApi.Verify(m => m.PutAsync(pipelineUrl, It.Is(payload => VerifyTriggersPreserved(payload, true, true) )), Times.Once); @@ -282,8 +389,8 @@ public async Task RewirePipelineToGitHub_WhenPipelineRequiredByBranchPolicy_Shou // Mock repository lookup - return valid repository var repositoryId = "repo-123"; - var repoResponse = new { id = repositoryId, name = REPO_NAME }.ToJson(); - var repoUrl = $"/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{REPO_NAME.EscapeDataString()}?api-version=6.0"; + var repoResponse = new { id = repositoryId, name = REPO_NAME, isDisabled = "false" }.ToJson(); + var repoUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{REPO_NAME.EscapeDataString()}?api-version=6.0"; // Mock branch policies - return policy that requires this pipeline var policies = new @@ -300,7 +407,7 @@ public async Task RewirePipelineToGitHub_WhenPipelineRequiredByBranchPolicy_Shou } } }.ToJson(); - var policyUrl = $"/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/policy/configurations?repositoryId={repositoryId}&api-version=6.0"; + var policyUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/policy/configurations?repositoryId={repositoryId}&api-version=6.0"; var pipelineUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; @@ -309,14 +416,107 @@ public async Task RewirePipelineToGitHub_WhenPipelineRequiredByBranchPolicy_Shou _mockAdoApi.Setup(m => m.GetAsync(policyUrl)).ReturnsAsync(policies); // Act - await _triggerService.RewirePipelineToGitHub(ADO_ORG, TEAM_PROJECT, pipelineId, defaultBranch, clean, checkoutSubmodules, "github-org", githubRepo, serviceConnectionId, null, null); + var result = await _triggerService.RewirePipelineToGitHub(ADO_ORG, TEAM_PROJECT, pipelineId, defaultBranch, clean, checkoutSubmodules, "github-org", githubRepo, serviceConnectionId, null, null); // Assert - Should enable both CI and PR triggers WITH build status reporting + result.Should().BeTrue(); _mockAdoApi.Verify(m => m.PutAsync(pipelineUrl, It.Is(payload => VerifyTriggersPreserved(payload, true, true) )), Times.Once); } + [Fact] + public async Task RewirePipelineToGitHub_Should_Skip_When_Repository_Is_Disabled() + { + // Arrange + var githubRepo = "test-repo"; + var serviceConnectionId = Guid.NewGuid().ToString(); + var defaultBranch = "main"; + var pipelineId = PIPELINE_ID; + var clean = "true"; + var checkoutSubmodules = "false"; + var repositoryId = Guid.NewGuid().ToString(); + + // Mock existing pipeline with disabled repository + var existingPipelineData = new + { + name = PIPELINE_NAME, + repository = new { name = REPO_NAME, id = repositoryId }, + someOtherProperty = "value" + }; + + // Mock repository lookup - return disabled repository + var repoResponse = new + { + id = repositoryId, + name = REPO_NAME, + isDisabled = "true" + }.ToJson(); + var repoUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{repositoryId.EscapeDataString()}?api-version=6.0"; + + var pipelineUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; + + _mockAdoApi.Setup(m => m.GetAsync(pipelineUrl)).ReturnsAsync(existingPipelineData.ToJson()); + _mockAdoApi.Setup(m => m.GetAsync(repoUrl)).ReturnsAsync(repoResponse); + + // Act + var result = await _triggerService.RewirePipelineToGitHub(ADO_ORG, TEAM_PROJECT, pipelineId, defaultBranch, clean, checkoutSubmodules, "github-org", githubRepo, serviceConnectionId, null, null); + + // Assert - Should NOT call PutAsync since repository is disabled + result.Should().BeFalse(); + _mockAdoApi.Verify(m => m.PutAsync(It.IsAny(), It.IsAny()), Times.Never); + + // Verify warning was logged + _mockOctoLogger.Verify(m => m.LogWarning(It.Is(s => + s.Contains("disabled") && + s.Contains("Skipping pipeline rewiring") && + s.Contains(pipelineId.ToString()) + )), Times.Once); + } + + [Fact] + public async Task RewirePipelineToGitHub_Should_Skip_When_Repository_Returns_404() + { + // Arrange + var githubRepo = "test-repo"; + var serviceConnectionId = Guid.NewGuid().ToString(); + var defaultBranch = "main"; + var pipelineId = PIPELINE_ID; + var clean = "true"; + var checkoutSubmodules = "false"; + var repositoryId = Guid.NewGuid().ToString(); + + // Mock existing pipeline + var existingPipelineData = new + { + name = PIPELINE_NAME, + repository = new { name = REPO_NAME, id = repositoryId }, + someOtherProperty = "value" + }; + + // Mock repository lookup - return 404 (likely disabled) + var repoUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/git/repositories/{repositoryId.EscapeDataString()}?api-version=6.0"; + + var pipelineUrl = $"{ADO_SERVICE_URL}/{ADO_ORG.EscapeDataString()}/{TEAM_PROJECT.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; + + _mockAdoApi.Setup(m => m.GetAsync(pipelineUrl)).ReturnsAsync(existingPipelineData.ToJson()); + _mockAdoApi.Setup(m => m.GetAsync(repoUrl)) + .ThrowsAsync(new HttpRequestException("Response status code does not indicate success: 404 (Not Found).")); + + // Act + var result = await _triggerService.RewirePipelineToGitHub(ADO_ORG, TEAM_PROJECT, pipelineId, defaultBranch, clean, checkoutSubmodules, "github-org", githubRepo, serviceConnectionId, null, null); + + // Assert - Should NOT call PutAsync since repository returned 404 + result.Should().BeFalse(); + _mockAdoApi.Verify(m => m.PutAsync(It.IsAny(), It.IsAny()), Times.Never); + + // Verify only one warning was logged about skipping pipeline rewiring (not the 404 warning) + _mockOctoLogger.Verify(m => m.LogWarning(It.Is(s => + s.Contains("Skipping pipeline rewiring") && + s.Contains(pipelineId.ToString()) + )), Times.Once); + } + private static bool VerifyTriggersPreserved(object payload, bool enablePullRequestValidation, bool enableBuildStatusReporting) { var json = payload.ToJson(); diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_ErrorHandlingTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_ErrorHandlingTests.cs index c4d2ed6a0..654c24650 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_ErrorHandlingTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_ErrorHandlingTests.cs @@ -45,11 +45,13 @@ public async Task RewirePipelineToGitHub_Should_Skip_When_Pipeline_Not_Found_404 _mockAdoApi.Setup(x => x.GetAsync(pipelineUrl)) .ThrowsAsync(new HttpRequestException("Response status code does not indicate success: 404 (Not Found).")); - // Act & Assert - Should not throw exception, should handle gracefully - await _triggerService.Invoking(x => x.RewirePipelineToGitHub( + // Act + var result = await _triggerService.RewirePipelineToGitHub( ADO_ORG, TEAM_PROJECT, PIPELINE_ID, defaultBranch, clean, checkoutSubmodules, - githubOrg, githubRepo, serviceConnectionId, null, null)) - .Should().NotThrowAsync(); + githubOrg, githubRepo, serviceConnectionId, null, null); + + // Assert - Should return false indicating skipped + result.Should().BeFalse(); // Verify that warning was logged _mockOctoLogger.Verify(x => x.LogWarning(It.Is(s => @@ -77,11 +79,13 @@ public async Task RewirePipelineToGitHub_Should_Skip_When_Pipeline_HTTP_Error() _mockAdoApi.Setup(x => x.GetAsync(pipelineUrl)) .ThrowsAsync(new HttpRequestException("Response status code does not indicate success: 500 (Internal Server Error).")); - // Act & Assert - Should not throw exception, should handle gracefully - await _triggerService.Invoking(x => x.RewirePipelineToGitHub( + // Act + var result = await _triggerService.RewirePipelineToGitHub( ADO_ORG, TEAM_PROJECT, PIPELINE_ID, defaultBranch, clean, checkoutSubmodules, - githubOrg, githubRepo, serviceConnectionId, null, null)) - .Should().NotThrowAsync(); + githubOrg, githubRepo, serviceConnectionId, null, null); + + // Assert - Should return false indicating skipped + result.Should().BeFalse(); // Verify that warning was logged _mockOctoLogger.Verify(x => x.LogWarning(It.Is(s => @@ -119,7 +123,7 @@ public async Task RewirePipelineToGitHub_Should_Continue_When_Pipeline_Found() // Mock repository lookup for branch policy check var repositoryId = "repo-123"; - var repoResponse = new { id = repositoryId, name = REPO_NAME }.ToJson(); + var repoResponse = new { id = repositoryId, name = REPO_NAME, isDisabled = "false" }.ToJson(); _mockAdoApi.Setup(x => x.GetAsync(repoUrl)) .ReturnsAsync(repoResponse); @@ -130,11 +134,14 @@ public async Task RewirePipelineToGitHub_Should_Continue_When_Pipeline_Found() .ReturnsAsync(policies); // Act - await _triggerService.RewirePipelineToGitHub( + var result = await _triggerService.RewirePipelineToGitHub( ADO_ORG, TEAM_PROJECT, PIPELINE_ID, defaultBranch, clean, checkoutSubmodules, githubOrg, githubRepo, serviceConnectionId, null, null); - // Assert - Verify that PutAsync was called (pipeline was successfully rewired) + // Assert - Should return true indicating successful rewiring + result.Should().BeTrue(); + + // Verify that PutAsync was called (pipeline was successfully rewired) _mockAdoApi.Verify(x => x.PutAsync(pipelineUrl, It.IsAny()), Times.Once); // Verify that no error warnings were logged diff --git a/src/OctoshiftCLI.Tests/Services/PipelineTestServiceTests.cs b/src/OctoshiftCLI.Tests/Services/PipelineTestServiceTests.cs index 714549dd3..d5a87b3e5 100644 --- a/src/OctoshiftCLI.Tests/Services/PipelineTestServiceTests.cs +++ b/src/OctoshiftCLI.Tests/Services/PipelineTestServiceTests.cs @@ -65,6 +65,9 @@ public async Task TestPipeline_Should_Perform_Complete_Test_Workflow() var buildId = 456; var buildUrl = "https://dev.azure.com/build/456"; + _mockAdoApi.Setup(x => x.IsPipelineEnabled(ADO_ORG, ADO_TEAM_PROJECT, PIPELINE_ID)) + .ReturnsAsync(true); + _mockAdoApi.Setup(x => x.GetPipelineRepository(ADO_ORG, ADO_TEAM_PROJECT, PIPELINE_ID)) .ReturnsAsync((originalRepoName, "repo-id", originalDefaultBranch, originalClean, originalCheckoutSubmodules)); @@ -126,6 +129,9 @@ public async Task TestPipeline_Should_Lookup_Pipeline_ID_When_Not_Provided() _mockAdoApi.Setup(x => x.GetPipelineId(ADO_ORG, ADO_TEAM_PROJECT, PIPELINE_NAME)) .ReturnsAsync(PIPELINE_ID); + _mockAdoApi.Setup(x => x.IsPipelineEnabled(ADO_ORG, ADO_TEAM_PROJECT, PIPELINE_ID)) + .ReturnsAsync(true); + _mockAdoApi.Setup(x => x.GetPipelineRepository(ADO_ORG, ADO_TEAM_PROJECT, PIPELINE_ID)) .ReturnsAsync(("repo", "id", "refs/heads/main", "true", "false")); @@ -159,6 +165,9 @@ public async Task TestPipeline_Should_Handle_Restoration_Failure() MonitorTimeoutMinutes = 1 }; + _mockAdoApi.Setup(x => x.IsPipelineEnabled(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(true); + _mockAdoApi.Setup(x => x.GetPipelineRepository(It.IsAny(), It.IsAny(), It.IsAny())) .ReturnsAsync(("repo", "id", "refs/heads/main", "true", "false")); @@ -183,5 +192,45 @@ public async Task TestPipeline_Should_Handle_Restoration_Failure() result.RestoredSuccessfully.Should().BeFalse(); result.ErrorMessage.Should().Contain("Failed to restore: Restore failed"); } + + [Fact] + public async Task TestPipeline_Should_Skip_Test_For_Disabled_Pipeline() + { + // Arrange + var args = new PipelineTestArgs + { + AdoOrg = ADO_ORG, + AdoTeamProject = ADO_TEAM_PROJECT, + PipelineName = PIPELINE_NAME, + PipelineId = PIPELINE_ID, + GithubOrg = GITHUB_ORG, + GithubRepo = GITHUB_REPO, + ServiceConnectionId = SERVICE_CONNECTION_ID, + MonitorTimeoutMinutes = 1 + }; + + _mockAdoApi.Setup(x => x.IsPipelineEnabled(ADO_ORG, ADO_TEAM_PROJECT, PIPELINE_ID)) + .ReturnsAsync(false); + + // Act + var result = await _service.TestPipeline(args); + + // Assert + result.Should().NotBeNull(); + result.ErrorMessage.Should().Be("Pipeline is disabled"); + result.EndTime.Should().NotBeNull(); + + // Verify that no pipeline operations were attempted + _mockAdoApi.Verify(x => x.GetPipelineRepository(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + _mockAdoApi.Verify(x => x.GetPipeline(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + _mockPipelineTriggerService.Verify(x => x.RewirePipelineToGitHub(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + _mockAdoApi.Verify(x => x.QueueBuild(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + _mockAdoApi.Verify(x => x.RestorePipelineToAdoRepo(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); + + _mockOctoLogger.Verify(x => x.LogWarning(It.Is(s => s.Contains("is disabled"))), Times.Once); + } } } diff --git a/src/OctoshiftCLI.Tests/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler_ErrorHandlingTests.cs b/src/OctoshiftCLI.Tests/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler_ErrorHandlingTests.cs index d03731df2..faf2a937b 100644 --- a/src/OctoshiftCLI.Tests/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler_ErrorHandlingTests.cs +++ b/src/OctoshiftCLI.Tests/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler_ErrorHandlingTests.cs @@ -103,6 +103,11 @@ public async Task HandleRegularRewire_Should_Succeed_When_Pipeline_Found() _mockAdoApi.Setup(x => x.GetPipeline(ADO_ORG, ADO_TEAM_PROJECT, PIPELINE_ID)) .ReturnsAsync((defaultBranch, clean, checkoutSubmodules, triggers)); + _mockAdoPipelineTriggerService.Setup(x => x.RewirePipelineToGitHub( + ADO_ORG, ADO_TEAM_PROJECT, PIPELINE_ID, defaultBranch, clean, checkoutSubmodules, + GITHUB_ORG, GITHUB_REPO, SERVICE_CONNECTION_ID, triggers, null)) + .ReturnsAsync(true); + var args = new RewirePipelineCommandArgs { AdoOrg = ADO_ORG, diff --git a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommand.cs b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommand.cs index ec0735664..8e7335096 100644 --- a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommand.cs +++ b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommand.cs @@ -27,6 +27,9 @@ public RewirePipelineCommand() : base( AddOption(TargetApiUrl); AddOption(DryRun); AddOption(MonitorTimeoutMinutes); + + // Set default value for MonitorTimeoutMinutes since System.CommandLine doesn't use property defaults + MonitorTimeoutMinutes.SetDefaultValue(30); } public Option AdoOrg { get; } = new("--ado-org") @@ -71,7 +74,7 @@ public RewirePipelineCommand() : base( }; public Option MonitorTimeoutMinutes { get; } = new("--monitor-timeout-minutes") { - Description = "Timeout in minutes for monitoring build completion in dry-run mode. Defaults to 30 minutes." + Description = "(Dry-run mode only) Timeout in minutes for monitoring build completion. Defaults to 30 minutes." }; public override RewirePipelineCommandHandler BuildHandler(RewirePipelineCommandArgs args, IServiceProvider sp) diff --git a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandArgs.cs b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandArgs.cs index 0564227b3..677f81831 100644 --- a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandArgs.cs +++ b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandArgs.cs @@ -1,4 +1,7 @@ -using OctoshiftCLI.Commands; +using System; +using OctoshiftCLI.Commands; +using OctoshiftCLI.Extensions; +using OctoshiftCLI.Services; namespace OctoshiftCLI.AdoToGithub.Commands.RewirePipeline { @@ -16,5 +19,43 @@ public class RewirePipelineCommandArgs : CommandArgs public string TargetApiUrl { get; set; } public bool DryRun { get; set; } public int MonitorTimeoutMinutes { get; set; } = 30; + + public override void Log(OctoLogger log) + { + if (log is null) + { + throw new ArgumentNullException(nameof(log)); + } + + log.Verbose = Verbose; + + // Log all properties except MonitorTimeoutMinutes + log.LogInformation($"ADO ORG: {AdoOrg}"); + log.LogInformation($"ADO TEAM PROJECT: {AdoTeamProject}"); + + if (AdoPipeline.HasValue()) + { + log.LogInformation($"ADO PIPELINE: {AdoPipeline}"); + } + + if (AdoPipelineId.HasValue) + { + log.LogInformation($"ADO PIPELINE ID: {AdoPipelineId}"); + } + + log.LogInformation($"GITHUB ORG: {GithubOrg}"); + log.LogInformation($"GITHUB REPO: {GithubRepo}"); + log.LogInformation($"SERVICE CONNECTION ID: {ServiceConnectionId}"); + + if (TargetApiUrl.HasValue()) + { + log.LogInformation($"TARGET API URL: {TargetApiUrl}"); + } + + if (DryRun) + { + log.LogInformation($"DRY RUN: true"); + } + } } } diff --git a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs index 47ea3b121..a50ee2e6a 100644 --- a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs +++ b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs @@ -52,6 +52,7 @@ public async Task Handle(RewirePipelineCommandArgs args) private async Task HandleDryRun(RewirePipelineCommandArgs args) { _log.LogInformation("Starting dry-run mode: Testing pipeline rewiring to GitHub..."); + _log.LogInformation($"Monitor timeout: {args.MonitorTimeoutMinutes} minutes"); var pipelineTestArgs = new PipelineTestArgs { @@ -73,6 +74,7 @@ private async Task HandleDryRun(RewirePipelineCommandArgs args) _log.LogInformation($"ADO Organization: {testResult.AdoOrg}"); _log.LogInformation($"ADO Team Project: {testResult.AdoTeamProject}"); _log.LogInformation($"Pipeline Name: {testResult.PipelineName}"); + _log.LogInformation($"Monitor Timeout: {args.MonitorTimeoutMinutes} minutes"); _log.LogInformation($"Build Result: {testResult.Result ?? "not completed"}"); if (testResult.Result == "succeeded") @@ -103,7 +105,7 @@ private async Task HandleRegularRewire(RewirePipelineCommandArgs args) var (defaultBranch, clean, checkoutSubmodules, triggers) = await _adoApi.GetPipeline(args.AdoOrg, args.AdoTeamProject, adoPipelineId); // Use the specialized service for complex trigger logic - await _pipelineTriggerService.RewirePipelineToGitHub( + var rewired = await _pipelineTriggerService.RewirePipelineToGitHub( args.AdoOrg, args.AdoTeamProject, adoPipelineId, @@ -116,7 +118,10 @@ await _pipelineTriggerService.RewirePipelineToGitHub( triggers, args.TargetApiUrl); - _log.LogSuccess("Successfully rewired pipeline"); + if (rewired) + { + _log.LogSuccess("Successfully rewired pipeline"); + } } catch (HttpRequestException ex) when (ex.Message.Contains("404")) { From 479110caf8cd7f176d9605c16cd249b0a1b1ed6c Mon Sep 17 00:00:00 2001 From: Georgy Gorelko Date: Wed, 3 Dec 2025 15:12:39 -0800 Subject: [PATCH 2/8] Update RewirePipelineCommandHandler.cs --- .../Commands/RewirePipeline/RewirePipelineCommandHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs index a50ee2e6a..edbd83162 100644 --- a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs +++ b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs @@ -131,7 +131,7 @@ private async Task HandleRegularRewire(RewirePipelineCommandArgs args) } catch (ArgumentException ex) when (ex.ParamName == "pipeline") { - // Pipeline lookup failed - log error and fail gracefully + // Pipeline lookup failed - log error and fail gracefully _log.LogError($"Pipeline lookup failed: {ex.Message}"); throw new OctoshiftCliException($"Unable to find the specified pipeline. Please verify the pipeline name and try again."); } From ba47a86f5336df48aaa046283590bce03921fe60 Mon Sep 17 00:00:00 2001 From: Georgy Gorelko Date: Wed, 3 Dec 2025 15:42:10 -0800 Subject: [PATCH 3/8] Fix SDK version in global.json and enhance null argument checks in AdoPipelineTriggerService --- src/Octoshift/Services/AdoPipelineTriggerService.cs | 9 +++++++-- .../RewirePipeline/RewirePipelineCommandHandler.cs | 1 - 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Octoshift/Services/AdoPipelineTriggerService.cs b/src/Octoshift/Services/AdoPipelineTriggerService.cs index 53362bc07..ac3cd81c5 100644 --- a/src/Octoshift/Services/AdoPipelineTriggerService.cs +++ b/src/Octoshift/Services/AdoPipelineTriggerService.cs @@ -50,6 +50,9 @@ public virtual async Task RewirePipelineToGitHub( JToken originalTriggers = null, string targetApiUrl = null) { + ArgumentNullException.ThrowIfNull(adoOrg); + ArgumentNullException.ThrowIfNull(teamProject); + var url = $"{_adoBaseUrl}/{adoOrg.EscapeDataString()}/{teamProject.EscapeDataString()}/_apis/build/definitions/{pipelineId}?api-version=6.0"; try @@ -143,7 +146,8 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t // Skip branch policy check if repository is disabled if (isRepositoryDisabled) { - _log.LogWarning($"Repository {adoOrg}/{teamProject}/{repoName} is disabled. Branch policy check skipped for pipeline {pipelineId}. Pipeline trigger configuration may not preserve branch policy requirements."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogWarning($"Repository {adoOrg}/{teamProject}/{repoIdentifier} is disabled. Branch policy check skipped for pipeline {pipelineId}. Pipeline trigger configuration may not preserve branch policy requirements."); return false; } @@ -531,8 +535,9 @@ private bool HasTriggerType(JToken originalTriggers, string triggerType) // 404 typically means the repository is disabled or doesn't exist // Treat it as disabled to avoid further API calls // Log as verbose since the caller will log a more specific warning about the disabled repository + // Return (null, true) to indicate repository ID is unknown but repository is disabled _log.LogVerbose($"Repository {adoOrg}/{teamProject}/{identifier} returned 404 - likely disabled or not found."); - var info = (identifier, true); // Mark as disabled + var info = ((string)null, true); // Mark as disabled with null ID since identifier may be a name _repositoryCache[cacheKey] = info; return info; } diff --git a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs index edbd83162..92a76f754 100644 --- a/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs +++ b/src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs @@ -74,7 +74,6 @@ private async Task HandleDryRun(RewirePipelineCommandArgs args) _log.LogInformation($"ADO Organization: {testResult.AdoOrg}"); _log.LogInformation($"ADO Team Project: {testResult.AdoTeamProject}"); _log.LogInformation($"Pipeline Name: {testResult.PipelineName}"); - _log.LogInformation($"Monitor Timeout: {args.MonitorTimeoutMinutes} minutes"); _log.LogInformation($"Build Result: {testResult.Result ?? "not completed"}"); if (testResult.Result == "succeeded") From 39357d9566dc1d2b17fa7d7bab61e2e39775e15f Mon Sep 17 00:00:00 2001 From: georgy-gorelko <218141981+georgy-gorelko@users.noreply.github.com> Date: Thu, 18 Dec 2025 17:10:33 -0800 Subject: [PATCH 4/8] Update release notes for recent pipeline improvements Updated release notes to reflect recent fixes and improvements in the pipeline rewiring process, including handling for disabled repositories and clearer error messages. --- RELEASENOTES.md | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index b83efaf07..2199f5f22 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -1,9 +1,5 @@ -- Fixed branch policy check for classic pipelines with prefixed repository names to use repository ID directly when available, preventing 404 errors during rewiring -- Skip branch policy checks for disabled repositories with a warning instead of attempting API calls that may fail -- Skip pipeline rewiring entirely for disabled repositories, exiting early with an appropriate warning message -- Fixed misleading success message that appeared even when pipeline rewiring was skipped for disabled repositories -- Fixed monitor timeout minutes to only display when --dry-run mode is enabled, reducing confusion during regular pipeline rewiring operations -- Check if pipeline is disabled before attempting to queue a test build, preventing 400 Bad Request errors and providing clear warning messages -- bbs2gh : Added validation for `--archive-path` and `--bbs-shared-home` options to fail fast with clear error messages if the provided paths do not exist or are not accessible. Archive path is now logged before upload operations to help with troubleshooting -- Added validation to detect and return clear error messages when a URL is provided instead of a name for organization, repository, or enterprise arguments (e.g., `--github-org`, `--github-target-org`, `--source-repo`, `--github-target-enterprise`) -- Added `--target-api-url` as an optional arg to the `add-team-to-repo` command +- **ado2gh**: Improved `rewire-pipeline` command to gracefully handle disabled repositories and pipelines with clear warnings instead of errors +- **ado2gh**: Fixed 404 errors when checking branch policies for classic pipelines with prefixed repository names +- **ado2gh**: Fixed 400 errors when running `rewire-pipeline --dry-run` on disabled pipelines +- **ado2gh**: Fixed misleading success messages when pipeline rewiring was skipped +- **ado2gh**: Fixed monitor timeout minutes to only display when --dry-run mode is enabled, reducing confusion during regular pipeline rewiring operations From a5f87d3886f082102c8d305ebdc8ab6c5d50026e Mon Sep 17 00:00:00 2001 From: Georgy Gorelko Date: Fri, 19 Dec 2025 10:53:01 -0800 Subject: [PATCH 5/8] Improve logging for branch policy checks by including repository identifier --- .../Services/AdoPipelineTriggerService.cs | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/Octoshift/Services/AdoPipelineTriggerService.cs b/src/Octoshift/Services/AdoPipelineTriggerService.cs index ac3cd81c5..7b30cb769 100644 --- a/src/Octoshift/Services/AdoPipelineTriggerService.cs +++ b/src/Octoshift/Services/AdoPipelineTriggerService.cs @@ -138,7 +138,8 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t if (string.IsNullOrEmpty(repositoryId)) { - _log.LogWarning($"Repository ID not found for {adoOrg}/{teamProject}/{repoName}. Branch policy check cannot be performed for pipeline {pipelineId}."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogWarning($"Repository ID not found for {adoOrg}/{teamProject}/{repoIdentifier}. Branch policy check cannot be performed for pipeline {pipelineId}."); return false; } } @@ -156,7 +157,8 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t if (policyData?.Value == null || policyData.Value.Count == 0) { - _log.LogVerbose($"No branch policies found for repository {adoOrg}/{teamProject}/{repoName}. ADO Pipeline ID = {pipelineId} is not required by branch policy."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogVerbose($"No branch policies found for repository {adoOrg}/{teamProject}/{repoIdentifier}. ADO Pipeline ID = {pipelineId} is not required by branch policy."); return false; } @@ -168,11 +170,13 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t if (isPipelineRequired) { - _log.LogVerbose($"ADO Pipeline ID = {pipelineId} is required by branch policy in {adoOrg}/{teamProject}/{repoName}. Build status reporting will be enabled to support branch protection."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogVerbose($"ADO Pipeline ID = {pipelineId} is required by branch policy in {adoOrg}/{teamProject}/{repoIdentifier}. Build status reporting will be enabled to support branch protection."); } else { - _log.LogVerbose($"ADO Pipeline ID = {pipelineId} is not required by any branch policies in {adoOrg}/{teamProject}/{repoName}."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogVerbose($"ADO Pipeline ID = {pipelineId} is not required by any branch policies in {adoOrg}/{teamProject}/{repoIdentifier}."); } return isPipelineRequired; @@ -180,31 +184,36 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t catch (HttpRequestException ex) { // If we can't determine branch policy status due to network issues, default to false - _log.LogWarning($"HTTP error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoName}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogWarning($"HTTP error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); return false; } catch (TaskCanceledException ex) { // If branch policy checking times out, consider check failed - _log.LogWarning($"Branch policy check timed out for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoName}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogWarning($"Branch policy check timed out for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); return false; } catch (JsonException ex) { // If we can't determine branch policy status due to JSON parsing issues, default to false - _log.LogWarning($"JSON parsing error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoName}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogWarning($"JSON parsing error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); return false; } catch (ArgumentException ex) { // If we can't determine branch policy status due to invalid arguments, default to false - _log.LogWarning($"Invalid argument error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoName}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogWarning($"Invalid argument error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); return false; } catch (InvalidOperationException ex) { // If branch policy checking fails due to invalid state, consider check failed - _log.LogWarning($"Invalid operation error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoName}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); + var repoIdentifier = repoName ?? repoId ?? "unknown"; + _log.LogWarning($"Invalid operation error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); return false; } } From ea2b2be00ce133754ecbc608d8fe3d675132954c Mon Sep 17 00:00:00 2001 From: Georgy Gorelko Date: Fri, 19 Dec 2025 11:00:43 -0800 Subject: [PATCH 6/8] Refactor logging for branch policy checks to improve clarity and remove redundant identifiers --- src/Octoshift/Services/AdoPipelineTriggerService.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Octoshift/Services/AdoPipelineTriggerService.cs b/src/Octoshift/Services/AdoPipelineTriggerService.cs index 7b30cb769..ff88af9b5 100644 --- a/src/Octoshift/Services/AdoPipelineTriggerService.cs +++ b/src/Octoshift/Services/AdoPipelineTriggerService.cs @@ -138,8 +138,7 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t if (string.IsNullOrEmpty(repositoryId)) { - var repoIdentifier = repoName ?? repoId ?? "unknown"; - _log.LogWarning($"Repository ID not found for {adoOrg}/{teamProject}/{repoIdentifier}. Branch policy check cannot be performed for pipeline {pipelineId}."); + _log.LogWarning($"Repository ID not found for {adoOrg}/{teamProject}/{repoName}. Branch policy check cannot be performed for pipeline {pipelineId}."); return false; } } @@ -147,7 +146,7 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t // Skip branch policy check if repository is disabled if (isRepositoryDisabled) { - var repoIdentifier = repoName ?? repoId ?? "unknown"; + var repoIdentifier = repoName ?? repoId; _log.LogWarning($"Repository {adoOrg}/{teamProject}/{repoIdentifier} is disabled. Branch policy check skipped for pipeline {pipelineId}. Pipeline trigger configuration may not preserve branch policy requirements."); return false; } @@ -546,7 +545,7 @@ private bool HasTriggerType(JToken originalTriggers, string triggerType) // Log as verbose since the caller will log a more specific warning about the disabled repository // Return (null, true) to indicate repository ID is unknown but repository is disabled _log.LogVerbose($"Repository {adoOrg}/{teamProject}/{identifier} returned 404 - likely disabled or not found."); - var info = ((string)null, true); // Mark as disabled with null ID since identifier may be a name + var info = (null, true); // Mark as disabled with null ID since identifier may be a name _repositoryCache[cacheKey] = info; return info; } From 9ea32af8e09c181ae0737017f612c7e37587ead7 Mon Sep 17 00:00:00 2001 From: Georgy Gorelko Date: Fri, 19 Dec 2025 11:32:19 -0800 Subject: [PATCH 7/8] Refactor branch policy check logic to improve clarity and reduce redundancy --- .../Services/AdoPipelineTriggerService.cs | 141 ++++++++---------- 1 file changed, 66 insertions(+), 75 deletions(-) diff --git a/src/Octoshift/Services/AdoPipelineTriggerService.cs b/src/Octoshift/Services/AdoPipelineTriggerService.cs index ff88af9b5..7176bac4c 100644 --- a/src/Octoshift/Services/AdoPipelineTriggerService.cs +++ b/src/Octoshift/Services/AdoPipelineTriggerService.cs @@ -116,34 +116,13 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t try { - // Use repository ID directly if available, otherwise look it up by name - string repositoryId; - var isRepositoryDisabled = false; + var (repositoryId, isRepositoryDisabled) = await GetRepositoryIdAndStatus(adoOrg, teamProject, repoName, repoId, pipelineId); - if (!string.IsNullOrEmpty(repoId)) + if (string.IsNullOrEmpty(repositoryId)) { - _log.LogVerbose($"Using repository ID from pipeline definition for branch policy check: {repoId}"); - repositoryId = repoId; - - // Check if repository is disabled by fetching its details - var (_, disabled) = await GetRepositoryInfoWithCache(adoOrg, teamProject, repoId, repoName); - isRepositoryDisabled = disabled; - } - else - { - // Get repository information by name (with caching) - var (id, disabled) = await GetRepositoryInfoWithCache(adoOrg, teamProject, null, repoName); - repositoryId = id; - isRepositoryDisabled = disabled; - - if (string.IsNullOrEmpty(repositoryId)) - { - _log.LogWarning($"Repository ID not found for {adoOrg}/{teamProject}/{repoName}. Branch policy check cannot be performed for pipeline {pipelineId}."); - return false; - } + return false; } - // Skip branch policy check if repository is disabled if (isRepositoryDisabled) { var repoIdentifier = repoName ?? repoId; @@ -151,72 +130,84 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t return false; } - // Get branch policies for the repository (with caching) - var policyData = await GetBranchPoliciesWithCache(adoOrg, teamProject, repositoryId); - - if (policyData?.Value == null || policyData.Value.Count == 0) - { - var repoIdentifier = repoName ?? repoId ?? "unknown"; - _log.LogVerbose($"No branch policies found for repository {adoOrg}/{teamProject}/{repoIdentifier}. ADO Pipeline ID = {pipelineId} is not required by branch policy."); - return false; - } - - // Look for enabled build validation policies that reference our pipeline - var isPipelineRequired = policyData.Value.Any(policy => - policy.Type?.DisplayName == "Build" && - policy.IsEnabled && - policy.Settings?.BuildDefinitionId == pipelineId.ToString()); - - if (isPipelineRequired) - { - var repoIdentifier = repoName ?? repoId ?? "unknown"; - _log.LogVerbose($"ADO Pipeline ID = {pipelineId} is required by branch policy in {adoOrg}/{teamProject}/{repoIdentifier}. Build status reporting will be enabled to support branch protection."); - } - else - { - var repoIdentifier = repoName ?? repoId ?? "unknown"; - _log.LogVerbose($"ADO Pipeline ID = {pipelineId} is not required by any branch policies in {adoOrg}/{teamProject}/{repoIdentifier}."); - } - - return isPipelineRequired; + return await CheckBranchPoliciesForPipeline(adoOrg, teamProject, repositoryId, repoName, repoId, pipelineId); } - catch (HttpRequestException ex) + catch (Exception ex) when (ex is HttpRequestException or TaskCanceledException or JsonException or ArgumentException or InvalidOperationException) { - // If we can't determine branch policy status due to network issues, default to false - var repoIdentifier = repoName ?? repoId ?? "unknown"; - _log.LogWarning($"HTTP error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); + LogBranchPolicyCheckError(ex, adoOrg, teamProject, repoName, repoId, pipelineId); return false; } - catch (TaskCanceledException ex) + } + + private async Task<(string repositoryId, bool isDisabled)> GetRepositoryIdAndStatus(string adoOrg, string teamProject, string repoName, string repoId, int pipelineId) + { + if (!string.IsNullOrEmpty(repoId)) { - // If branch policy checking times out, consider check failed - var repoIdentifier = repoName ?? repoId ?? "unknown"; - _log.LogWarning($"Branch policy check timed out for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); - return false; + _log.LogVerbose($"Using repository ID from pipeline definition for branch policy check: {repoId}"); + var (_, disabled) = await GetRepositoryInfoWithCache(adoOrg, teamProject, repoId, repoName); + return (repoId, disabled); } - catch (JsonException ex) + + var (id, disabledStatus) = await GetRepositoryInfoWithCache(adoOrg, teamProject, null, repoName); + if (string.IsNullOrEmpty(id)) { - // If we can't determine branch policy status due to JSON parsing issues, default to false - var repoIdentifier = repoName ?? repoId ?? "unknown"; - _log.LogWarning($"JSON parsing error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); - return false; + _log.LogWarning($"Repository ID not found for {adoOrg}/{teamProject}/{repoName}. Branch policy check cannot be performed for pipeline {pipelineId}."); + return (null, false); } - catch (ArgumentException ex) + + return (id, disabledStatus); + } + + private async Task CheckBranchPoliciesForPipeline(string adoOrg, string teamProject, string repositoryId, string repoName, string repoId, int pipelineId) + { + var policyData = await GetBranchPoliciesWithCache(adoOrg, teamProject, repositoryId); + + if (policyData?.Value == null || policyData.Value.Count == 0) { - // If we can't determine branch policy status due to invalid arguments, default to false var repoIdentifier = repoName ?? repoId ?? "unknown"; - _log.LogWarning($"Invalid argument error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); + _log.LogVerbose($"No branch policies found for repository {adoOrg}/{teamProject}/{repoIdentifier}. ADO Pipeline ID = {pipelineId} is not required by branch policy."); return false; } - catch (InvalidOperationException ex) + + var isPipelineRequired = policyData.Value.Any(policy => + policy.Type?.DisplayName == "Build" && + policy.IsEnabled && + policy.Settings?.BuildDefinitionId == pipelineId.ToString()); + + LogBranchPolicyCheckResult(isPipelineRequired, adoOrg, teamProject, repoName, repoId, pipelineId); + return isPipelineRequired; + } + + private void LogBranchPolicyCheckResult(bool isPipelineRequired, string adoOrg, string teamProject, string repoName, string repoId, int pipelineId) + { + var repoIdentifier = repoName ?? repoId ?? "unknown"; + + if (isPipelineRequired) + { + _log.LogVerbose($"ADO Pipeline ID = {pipelineId} is required by branch policy in {adoOrg}/{teamProject}/{repoIdentifier}. Build status reporting will be enabled to support branch protection."); + } + else { - // If branch policy checking fails due to invalid state, consider check failed - var repoIdentifier = repoName ?? repoId ?? "unknown"; - _log.LogWarning($"Invalid operation error during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); - return false; + _log.LogVerbose($"ADO Pipeline ID = {pipelineId} is not required by any branch policies in {adoOrg}/{teamProject}/{repoIdentifier}."); } } + private void LogBranchPolicyCheckError(Exception ex, string adoOrg, string teamProject, string repoName, string repoId, int pipelineId) + { + var repoIdentifier = repoName ?? repoId ?? "unknown"; + var errorType = ex switch + { + HttpRequestException => "HTTP error", + TaskCanceledException => "Branch policy check timed out", + JsonException => "JSON parsing error", + ArgumentException => "Invalid argument error", + InvalidOperationException => "Invalid operation error", + _ => "Error" + }; + + _log.LogWarning($"{errorType} during branch policy check for pipeline {pipelineId} in {adoOrg}/{teamProject}/{repoIdentifier}: {ex.Message}. Pipeline trigger configuration may not preserve branch policy requirements."); + } + #region Private Helper Methods - Trigger Configuration Logic private void LogBranchPolicyCheckResults(int pipelineId, bool isPipelineRequiredByBranchPolicy) @@ -545,7 +536,7 @@ private bool HasTriggerType(JToken originalTriggers, string triggerType) // Log as verbose since the caller will log a more specific warning about the disabled repository // Return (null, true) to indicate repository ID is unknown but repository is disabled _log.LogVerbose($"Repository {adoOrg}/{teamProject}/{identifier} returned 404 - likely disabled or not found."); - var info = (null, true); // Mark as disabled with null ID since identifier may be a name + (string id, bool isDisabled) info = (null, true); // Mark as disabled with null ID since identifier may be a name _repositoryCache[cacheKey] = info; return info; } From e31624abe257ff88d4a906aad89c4866ca4e4a20 Mon Sep 17 00:00:00 2001 From: Georgy Gorelko Date: Mon, 5 Jan 2026 15:05:47 -0800 Subject: [PATCH 8/8] Refactor AdoPipelineTriggerService to log information instead of warnings for disabled repositories and adjust related tests. Removed the check that prevented rewiring disabled repositories. Branch policy checks are skipped for disabled repos --- .../Services/AdoPipelineTriggerService.cs | 15 +---------- ...ipelineTriggerService_BranchPolicyTests.cs | 25 ++++++++++--------- 2 files changed, 14 insertions(+), 26 deletions(-) diff --git a/src/Octoshift/Services/AdoPipelineTriggerService.cs b/src/Octoshift/Services/AdoPipelineTriggerService.cs index 7176bac4c..5a419a5c1 100644 --- a/src/Octoshift/Services/AdoPipelineTriggerService.cs +++ b/src/Octoshift/Services/AdoPipelineTriggerService.cs @@ -63,19 +63,6 @@ public virtual async Task RewirePipelineToGitHub( var currentRepoName = data["repository"]?["name"]?.ToString(); var currentRepoId = data["repository"]?["id"]?.ToString(); - // Check if repository is disabled - skip rewiring if it is - if (!string.IsNullOrEmpty(currentRepoId) || !string.IsNullOrEmpty(currentRepoName)) - { - var identifier = !string.IsNullOrEmpty(currentRepoId) ? currentRepoId : currentRepoName; - var (_, isDisabled) = await GetRepositoryInfoWithCache(adoOrg, teamProject, currentRepoId, currentRepoName); - - if (isDisabled) - { - _log.LogWarning($"Repository {adoOrg}/{teamProject}/{identifier} is disabled. Skipping pipeline rewiring for pipeline {pipelineId}."); - return false; - } - } - var newRepo = CreateGitHubRepositoryConfiguration(githubOrg, githubRepo, defaultBranch, clean, checkoutSubmodules, connectedServiceId, targetApiUrl); var isPipelineRequiredByBranchPolicy = await IsPipelineRequiredByBranchPolicy(adoOrg, teamProject, currentRepoName, currentRepoId, pipelineId); @@ -126,7 +113,7 @@ public async Task IsPipelineRequiredByBranchPolicy(string adoOrg, string t if (isRepositoryDisabled) { var repoIdentifier = repoName ?? repoId; - _log.LogWarning($"Repository {adoOrg}/{teamProject}/{repoIdentifier} is disabled. Branch policy check skipped for pipeline {pipelineId}. Pipeline trigger configuration may not preserve branch policy requirements."); + _log.LogInformation($"Repository {adoOrg}/{teamProject}/{repoIdentifier} is disabled. Branch policy check skipped for pipeline {pipelineId} - will use default trigger configuration."); return false; } diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_BranchPolicyTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_BranchPolicyTests.cs index fbee8d727..0800b3150 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_BranchPolicyTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_BranchPolicyTests.cs @@ -462,14 +462,14 @@ public async Task RewirePipelineToGitHub_Should_Skip_When_Repository_Is_Disabled // Act var result = await _triggerService.RewirePipelineToGitHub(ADO_ORG, TEAM_PROJECT, pipelineId, defaultBranch, clean, checkoutSubmodules, "github-org", githubRepo, serviceConnectionId, null, null); - // Assert - Should NOT call PutAsync since repository is disabled - result.Should().BeFalse(); - _mockAdoApi.Verify(m => m.PutAsync(It.IsAny(), It.IsAny()), Times.Never); + // Assert - Should succeed and call PutAsync even though repository is disabled + result.Should().BeTrue(); + _mockAdoApi.Verify(m => m.PutAsync(It.IsAny(), It.IsAny()), Times.Once); - // Verify warning was logged - _mockOctoLogger.Verify(m => m.LogWarning(It.Is(s => + // Verify info message was logged about skipping branch policy check + _mockOctoLogger.Verify(m => m.LogInformation(It.Is(s => s.Contains("disabled") && - s.Contains("Skipping pipeline rewiring") && + s.Contains("Branch policy check skipped") && s.Contains(pipelineId.ToString()) )), Times.Once); } @@ -506,13 +506,14 @@ public async Task RewirePipelineToGitHub_Should_Skip_When_Repository_Returns_404 // Act var result = await _triggerService.RewirePipelineToGitHub(ADO_ORG, TEAM_PROJECT, pipelineId, defaultBranch, clean, checkoutSubmodules, "github-org", githubRepo, serviceConnectionId, null, null); - // Assert - Should NOT call PutAsync since repository returned 404 - result.Should().BeFalse(); - _mockAdoApi.Verify(m => m.PutAsync(It.IsAny(), It.IsAny()), Times.Never); + // Assert - Should succeed and call PutAsync even though repository returned 404 + result.Should().BeTrue(); + _mockAdoApi.Verify(m => m.PutAsync(It.IsAny(), It.IsAny()), Times.Once); - // Verify only one warning was logged about skipping pipeline rewiring (not the 404 warning) - _mockOctoLogger.Verify(m => m.LogWarning(It.Is(s => - s.Contains("Skipping pipeline rewiring") && + // Verify info message was logged about skipping branch policy check (repo treated as disabled) + _mockOctoLogger.Verify(m => m.LogInformation(It.Is(s => + s.Contains("disabled") && + s.Contains("Branch policy check skipped") && s.Contains(pipelineId.ToString()) )), Times.Once); }