Fix conflict message when flows are blocked#5473
Conversation
|
Here's a PR showing correct behavior maestro-auth-test/dotnet#219 |
|
and also a backflow maestro-auth-test/maestro-test1#4280 (note that the repo link says not found because the file is deleted on one side to cause the conflict) |
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies the ConflictInPrBranchException constructor by removing parameters that performed path transformations, and changes the ReproTool to force subscription triggers. The changes address issue #5471 related to fixing conflict messages when flows are blocked.
Key Changes
- Simplified
ConflictInPrBranchExceptionconstructor by removingmappingNameandisForwardFlowparameters - Removed path transformation logic in
ParseResultmethod that was converting VMR paths to repo paths for forward flows and vice versa for backflows - Changed
TriggerSubscriptionAsyncin ReproTool to useforce: trueinstead offorce: false
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tools/ProductConstructionService.ReproTool/Operations/Operation.cs |
Changed subscription trigger to use force parameter set to true |
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCodeflower.cs |
Updated ConflictInPrBranchException constructor call to match simplified signature |
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs |
Removed mappingName and isForwardFlow parameters from constructor and simplified ParseResult to return raw file paths without transformation |
| public ConflictInPrBranchException( | ||
| string failedMergeMessage, | ||
| string targetBranch, | ||
| string mappingName, | ||
| bool isForwardFlow) | ||
| : this(ParseResult(failedMergeMessage, mappingName, isForwardFlow), targetBranch) | ||
| string targetBranch) | ||
| : this(ParseResult(failedMergeMessage), targetBranch) |
There was a problem hiding this comment.
The constructor signature for ConflictInPrBranchException was changed to remove the mappingName and isForwardFlow parameters, but there's a test file that still uses the old 4-parameter constructor: repo/test/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs at lines 587-591.
This will cause a compilation error:
new ConflictInPrBranchException(
"error: patch failed: " + string.Join(Environment.NewLine + "error: patch failed: ", conflictedFiles),
"branch",
"repo",
isForwardFlow: true)The constructor call should be updated to match the new signature with only 2 parameters:
new ConflictInPrBranchException(
"error: patch failed: " + string.Join(Environment.NewLine + "error: patch failed: ", conflictedFiles),
"branch")
premun
left a comment
premun
left a comment
There was a problem hiding this comment.
Is the solution that the exception now strips the prefix correctly already?
No, the Comment Builder makes sure that the file paths are good, so e.g. if the conflict was on a ff, we get |
#5471