-
Notifications
You must be signed in to change notification settings - Fork 119
rewire-pipelines: set PR trigger configuration after rewiring to GitHub based on Azure DevOps branch policy #1412
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
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 implements automatic preservation of Azure DevOps pipeline trigger configurations during migration to GitHub, with intelligent detection of branch policy requirements to determine when pull request triggers should be enabled.
- Adds branch policy checking to determine if pipelines are required by build validation policies
- Modifies trigger handling to preserve original configurations while enabling proper GitHub integration
- Implements comprehensive test coverage for the new branch policy detection functionality
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler.cs |
Updated to pass trigger data from pipeline retrieval to pipeline modification |
src/OctoshiftCLI.Tests/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandler_TriggerPreservationTests.cs |
New test file covering trigger preservation scenarios |
src/OctoshiftCLI.Tests/ado2gh/Commands/RewirePipeline/RewirePipelineCommandHandlerTests.cs |
Updated existing tests to handle new trigger parameter |
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs |
New comprehensive tests for pull request validation enhancement logic |
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_BranchPolicyTests.cs |
New test suite for branch policy detection functionality |
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApiTests.cs |
Updated existing API tests to accommodate new trigger handling |
src/Octoshift/Services/AdoApi.cs |
Core implementation of branch policy checking and enhanced trigger management |
global.json |
Updated .NET SDK version |
RELEASENOTES.md |
Added release note for the new trigger preservation feature |
Unit Test Results 1 files 1 suites 10m 24s ⏱️ Results for commit 97e5c39. ♻️ This comment has been updated with latest results. |
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
src/OctoshiftCLI.Tests/Octoshift/Services/AdoApi_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
… the migration process. 1. Enhanced Branch Policy Failure Logging ✅ Added detailed logging to CheckBranchPolicyRequirement method: Logs when repository name is not available Logs specific error messages for each type of failure: Network/HTTP errors Timeout errors JSON parsing errors Invalid argument errors Invalid operation errors Each log includes the pipeline ID, organization, team project, and repository for easy identification ✅ Enhanced IsPipelineRequiredByBranchPolicy method with verbose logging: Logs when repository ID cannot be found Logs when no branch policies exist (verbose) Logs when a pipeline IS required by branch policy (verbose) Logs when a pipeline is NOT required by branch policy (verbose) Logs specific error details for HTTP, JSON, and argument errors ✅ Added migration summary logging to ChangePipelineRepo method: Provides clear, informational messages about branch policy check results Explains the trigger configuration decision being made Helps users understand whether their branch policies were preserved.
@boylejj anyone can assist with review? |
I might consider extracting all that pipeline trigger logic out of AdoApi (which is intended to be relatively thin wrapper around calling ADO Api's, and not behavior-rich) and into a separate service which sits on top of AdoApi. |
PR description confused me a bit because there are so many meanings of trigger across ADO+GH+yml+pipelines: Yours says: My now understanding is: |
…equest validation - Implemented tests for IsPipelineRequiredByBranchPolicy method to verify behavior when pipeline is required, not in policy, policy is disabled, and no build policies exist. - Added tests for RewirePipelineToGitHub method to ensure triggers are preserved or enabled based on branch policy requirements. - Created AdoPipelineTriggerServiceFactory to facilitate the creation of AdoPipelineTriggerService instances with dependency injection. - Enhanced test coverage for CreateYamlControlledTriggers and HasPullRequestTrigger methods to validate trigger creation logic.
...ctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
… tests for improved readability and error handling
...ctoshiftCLI.Tests/Octoshift/Services/AdoPipelineTriggerService_PullRequestValidationTests.cs
Fixed
Show fixed
Hide fixed
Did code refactoring as per suggestion |
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.
I have concerns about API call performance for repeated invocations.
…gerService to reduce redundant API calls
@georgy-gorelko the logic you have in place all seems sound to rewire pipeline triggers post migration, do you have any mannual testing you've done of these changes? I am not sure why our ADO int tests havent run either I would feel more confident if we can show those passing |
Yes, I have run CLI locally to rewire pipelines upon migration. Here's example before/after CLI changes @begonaguereca I don't know what's the process of triggering remaining tests would be and why those not firing. Hopefully DRI team can review and assist with the needful runs |
@georgy-gorelko why are you merging into |
Im running the int tests manually here https://github.com/github/gh-gei/actions/runs/17305685659 |
@georgy-gorelko the ADO token was expired, I just updated it |
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.
Hey! Once the int tests pass this is good to go 👍🏽
@georgy-gorelko I am working on fixing the CI issue, but the migrations are passing in the end. I think we can merge without the remaining ones passing |
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.
Good to ship pending testing
I've got the CI working for everythign less BBS working on getting the server up again for BBS |
Ok all the CI issues have been resolved @georgy-gorelko if you would like to merge this in so we can review the other PR's that depend on this |
@begonaguereca Geogry is on vacation and I'm his team mate and stand-in. I was on vacation while this was proposed and i am not sure why we would turn of the override of yaml triggers? was that something you/your team discussed with Georgy? A lot of ADo pipelines has branch policies and ultimately the tool (probably?) needs tro translated that in to yaml triggers? |
Pipeline Trigger Preservation Feature
Overview
The
gh ado2gh rewire-pipeline
command now automatically preserves Azure DevOps pipeline trigger configurations when migrating to GitHub.What Changed
✅ Core Feature Implementation
Added
IsPipelineRequiredByBranchPolicy
methodThis method:
Integrated branch policy checking into
ChangePipelineRepo
The method now:
Enhanced trigger creation logic
The system now:
✅ Comprehensive Test Coverage
Created
AdoApi_BranchPolicyTests.cs
with full test coverage for:Business Impact
Benefits Delivered
Validation Results
Test Coverage
dotnet test --filter AdoApi_BranchPolicyTests
Passed! - Failed: 0, Passed: 6
Build Quality
dotnet build --verbosity minimal
Build succeeded. 0 Warning(s) 0 Error(s)
Usage
No changes required for end users - trigger preservation happens automatically:
The command will now automatically preserve all original pipeline trigger configurations during the migration process.