-
Notifications
You must be signed in to change notification settings - Fork 123
Harder pipeline rewire monitoring and error handling #1465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves error handling and monitoring for the pipeline rewiring process, with a focus on handling disabled repositories and pipelines more gracefully. The changes prevent API errors (404s and 400s) by checking repository and pipeline status before attempting operations, and fix misleading user feedback when operations are skipped.
Key Changes:
- Enhanced repository and pipeline status checking to skip operations on disabled resources with clear warnings instead of errors
- Improved branch policy checks to use repository ID directly when available, reducing API calls and preventing 404 errors
- Fixed misleading success message that appeared even when pipeline rewiring was skipped
- Refined monitor timeout logging to only display in dry-run mode
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs | Added conditional success logging based on rewire result; added monitor timeout logging in dry-run mode; removed trailing whitespace from comment |
| src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandArgs.cs | Added custom Log method override to exclude MonitorTimeoutMinutes and AdoPat from logging |
| src/ado2gh/Commands/RewirePipeline/RewirePipelineCommand.cs | Updated MonitorTimeoutMinutes option description to clarify it's for dry-run mode only; set explicit default value |
| src/OctoshiftCLI.Tests/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler_ErrorHandlingTests.cs | Updated mock to return boolean for RewirePipelineToGitHub and verify success logging |
| src/OctoshiftCLI.Tests/Services/PipelineTestServiceTests.cs | Added IsPipelineEnabled mocks to existing tests; added comprehensive test for disabled pipeline scenario |
| src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_ErrorHandlingTests.cs | Updated tests to verify boolean return value from RewirePipelineToGitHub; added isDisabled field to mock responses |
| src/OctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_BranchPolicyTests.cs | Added tests for disabled repository handling and repository ID direct usage; updated all branch policy tests with isDisabled field; added comprehensive tests for 404 scenarios |
| src/OctoshiftCLI.Tests/Octoshift/Services/AdoApiTests.cs | Added comprehensive tests for IsPipelineEnabled covering enabled, disabled, paused, and missing queueStatus scenarios |
| src/Octoshift/Services/PipelineTestService.cs | Added pipeline enabled check before attempting to queue test builds |
| src/Octoshift/Services/AdoPipelineTriggerService.cs | Changed RewirePipelineToGitHub to return bool; added repository disabled checks; enhanced caching to track disabled status; updated IsPipelineRequiredByBranchPolicy to accept and use repository ID directly |
| src/Octoshift/Services/AdoApi.cs | Added IsPipelineEnabled method to check pipeline queueStatus |
| src/Octoshift/Commands/CommandArgs.cs | Made Log method virtual to allow overrides |
| RELEASENOTES.md | Added user-facing descriptions of all fixes and improvements |
Comments suppressed due to low confidence (1)
src/OctoshiftCLI.Tests/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler_ErrorHandlingTests.cs:135
- Consider adding a test case for when
RewirePipelineToGitHubreturnsfalse(e.g., when the repository is disabled). This should verify that:
- No success message is logged
- No error is thrown (since it's a graceful skip, not an error)
This would ensure the conditional success logging on lines 121-124 of the handler works correctly.
public async Task HandleRegularRewire_Should_Succeed_When_Pipeline_Found()
{
// Arrange
var defaultBranch = "main";
var clean = "true";
var checkoutSubmodules = "false";
var triggers = new JArray();
_mockAdoApi.Setup(x => x.GetPipelineId(ADO_ORG, ADO_TEAM_PROJECT, ADO_PIPELINE))
.ReturnsAsync(PIPELINE_ID);
_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,
AdoTeamProject = ADO_TEAM_PROJECT,
AdoPipeline = ADO_PIPELINE,
GithubOrg = GITHUB_ORG,
GithubRepo = GITHUB_REPO,
ServiceConnectionId = SERVICE_CONNECTION_ID,
DryRun = false
};
// Act
await _handler.Handle(args);
// Assert
_mockAdoPipelineTriggerService.Verify(x => x.RewirePipelineToGitHub(
ADO_ORG, ADO_TEAM_PROJECT, PIPELINE_ID, defaultBranch, clean, checkoutSubmodules,
GITHUB_ORG, GITHUB_REPO, SERVICE_CONNECTION_ID, triggers, null), Times.Once);
// Verify success was logged
_mockOctoLogger.Verify(x => x.LogSuccess("Successfully rewired pipeline"), Times.Once);
// Verify no errors were logged
_mockOctoLogger.Verify(x => x.LogError(It.IsAny<string>()), Times.Never);
}
src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs
Outdated
Show resolved
Hide resolved
Unit Test Results 1 files 1 suites 10m 25s ⏱️ Results for commit 22e47b0. ♻️ This comment has been updated with latest results. |
Uh oh!
There was an error while loading. Please reload this page.