Stop ForwardFlow updates if they'd overwrite manual PR changes#5043
Stop ForwardFlow updates if they'd overwrite manual PR changes#5043dkurepa merged 16 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR prevents forward-flow operations from overwriting manual changes on a pull request by detecting non‐bot commits in the VMR, throwing a dedicated exception, and commenting on the PR before retrying after merge. It also extends the remote Git abstractions to fetch existing PR comments and adds a full scenario test.
- Introduces
ManualChangesWouldGetOverwrittenExceptionandValidateNothingGetsOverwrittenin the VMR flow. - Updates
PullRequestUpdaterto catch the exception, comment the PR, and schedule a retry. - Adds
GetPullRequestCommentsAsyncto remote interfaces and clients, plus a scenario test verifying manual-change protection.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ProductConstructionService.ScenarioTests/ScenarioTests_CodeFlow.cs | Added a new NUnit scenario test for manual‐change protection in forward flow. |
| test/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs | Introduced FastForwardAsync helper to fetch and pull the latest in tests. |
| src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs | Catches the new exception, implements HandleOverwrittingChanges, and schedules reminders. |
| src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrPatchHandler.cs | Added --no-color to Git diff commands. |
| src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs | Calls ValidateNothingGetsOverwritten before applying forward flows. |
| src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs | Defines ManualChangesWouldGetOverwrittenException. |
| src/Microsoft.DotNet.Darc/DarcLib/Remote.cs | Added GetPullRequestCommentsAsync to fetch existing PR comments. |
| src/Microsoft.DotNet.Darc/DarcLib/IRemoteGitRepo.cs | Declared GetPullRequestCommentsAsync in the remote Git interface. |
| src/Microsoft.DotNet.Darc/DarcLib/IRemote.cs | Added GetPullRequestCommentsAsync to the IRemote interface. |
| src/Microsoft.DotNet.Darc/DarcLib/GitHubClient.cs | Implemented PR comment retrieval via Octokit. |
| src/Microsoft.DotNet.Darc/DarcLib/Constants.cs | Added DefaultCommitAuthor constant for bot commit filtering. |
| src/Microsoft.DotNet.Darc/DarcLib/AzureDevOpsClient.cs | Implemented PR comment retrieval for Azure DevOps. |
Comments suppressed due to low confidence (1)
src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs:1383
- The method name
HandleOverwrittingChangeshas a typo. Consider renaming it toHandleOverwritingChanges(single 't') orHandleOverwrittenChangesfor clarity.
private async Task HandleOverwrittingChanges(SubscriptionDTO subscription, List<string> commits, InProgressPullRequest pr, SubscriptionUpdateWorkItem update)
…enarioTests_CodeFlow.cs Co-authored-by: Copilot <[email protected]>
adamzip
left a comment
adamzip
left a comment
There was a problem hiding this comment.
LGTM, just a couple comments
|
|
||
| // if the first commit in the head branch wasn't made by the bot don't check, we might be in a test | ||
| if (headBranchCommits[0].commiter != Constants.DefaultCommitAuthor) | ||
| { |
There was a problem hiding this comment.
Add a log here to explain the situation?
There was a problem hiding this comment.
I added logging, but not here. I don't think we need it here, we're just gonna continue in this case. I added logs in the error handling method tho
There was a problem hiding this comment.
I was thinking if there's somehow a codeflow PR where the first commit is not maestro bot (for example if someone tried to fix an issue in a codeflow PR by doing a rebase), then it would be nice to know about in the logs. Is it too much of an implausible situation?
Co-authored-by: Adam <[email protected]>
…ower.cs Co-authored-by: Adam <[email protected]>
…ptions.cs" This reverts commit 9df1120.
…orwardFlower.cs" This reverts commit da082fd.
|
|
||
| // if the first commit in the head branch wasn't made by the bot don't check, we might be in a test | ||
| if (headBranchCommits[0].commiter != Constants.DefaultCommitAuthor) | ||
| { |
There was a problem hiding this comment.
I was thinking if there's somehow a codeflow PR where the first commit is not maestro bot (for example if someone tried to fix an issue in a codeflow PR by doing a rebase), then it would be nice to know about in the logs. Is it too much of an implausible situation?
Ah I get it, but I don't think that should ever happen? especially with the codeflow PRs, might bring something else along with it. I'll merge this as is for now, we can maybe explore that option when we work on the real implementation |
…t#5043) <!-- Link the GitHub or AzDO issue this pull request is associated with. Please copy and paste the full URL rather than using the dotnet/arcade-services# syntax --> dotnet#5030 --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Adam <[email protected]>
dotnet#5043)" This reverts commit 8db78ae.
…t#5043) <!-- Link the GitHub or AzDO issue this pull request is associated with. Please copy and paste the full URL rather than using the dotnet/arcade-services# syntax --> dotnet#5030 --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: Adam <[email protected]>
Stop ForwardFlow updates if they'd overwrite manual PR changes (dotnet#5043)
dotnet#5043)" This reverts commit 2e55418.
#5030