-
Notifications
You must be signed in to change notification settings - Fork 80
Add darc vmr resolve operation
#5391
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
Conversation
5f859f5 to
aeb22c3
Compare
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 the darc vmr resolve command to help users resolve codeflow conflicts encountered by Maestro. The command validates PR state, verifies local repository configurations, and attempts to flow the next build using the rebase strategy.
Key Changes:
- Added
HeadBranchandNextBuildsToApplyfields to the TrackedPullRequest API model for conflict resolution - Refactored forward flow and backflow implementations from child operations into the parent
CodeFlowOperationclass for reuse - Implemented the new
ResolveOperationwith comprehensive validations for subscription state, local repositories, and branch alignment
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
PullRequestController.cs |
Adds two new fields to TrackedPullRequest DTO and refactors duplicate code into a helper method |
LocalGitClient.cs |
Adds validation method to check for clean working directory (no conflicts or uncommitted changes) |
Local.cs |
Adds methods to retrieve source dependency and source manifest from local repositories |
ResolveCommandLineOptions.cs |
Defines command-line options for the new resolve command |
ResolveOperation.cs |
Implements the resolve operation with validation logic and codeflow execution |
ForwardFlowOperation.cs |
Removes FlowCodeAsync implementation, now uses parent class implementation |
CodeFlowOperation.cs |
Moves FlowForwardAsync and FlowBackAsync implementations from child classes |
BackflowOperation.cs |
Removes FlowCodeAsync implementation, now uses parent class implementation |
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
| private async Task ValidateLocalVmr(Subscription subscription) | ||
| { | ||
| var mappingName = string.IsNullOrEmpty(subscription.TargetDirectory) | ||
| ? subscription.SourceDirectory | ||
| : subscription.TargetDirectory; | ||
|
|
||
| var local = new Local(_options.GetRemoteTokenProvider(), _logger); | ||
|
|
||
| SourceManifest sourceManifest; | ||
| try | ||
| { | ||
| sourceManifest = await local.GetSourceManifestAsync(_vmrInfo.VmrPath); | ||
| } | ||
| catch (DependencyFileNotFoundException) | ||
| { | ||
| throw new DarcException("Could not find file `src/source-manifest.json` at the following" + | ||
| $"git repository: `{_vmrInfo.VmrPath}`. Please make sure it is a correct path to the VMR."); | ||
| } | ||
|
|
||
| if (!sourceManifest.Repositories.Any(repo => repo.Path.Equals(mappingName))) | ||
| { | ||
| throw new DarcException($"Could not find repo with name '{mappingName}' in the source-manifest.json" + | ||
| $" at the following git repository: `{_vmrInfo.VmrPath}. Please make sure it is a correct path to" + | ||
| " the VMR and that the mapping exists."); | ||
| } | ||
| } |
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 think this is a bit useless. If you call RefreshMetadata on the IDependencyTracker, it will do all this and throw an exception.
You need to call that one anyway to load the source mappings.
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.
Have a look at the CheckoutAsync in VmrCloneManager.
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.
The exception from RefreshMetaData() will likely be something like a "file not found" exception when trying to read source-manifest, right?
Instead, this validation provides a sensible message to the user that explains what the cause might be (maybe he is running the command from the wrong folder), and a suggestion for how to move next (make sure you run it from the correct target repo of the subscription)
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.
Probably, but why re-implement what it does instead of catching that exception and turning it into your own more approchable user-friendly one?
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Options/VirtualMonoRepo/ResolveCommandLineOptions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Options/VirtualMonoRepo/ResolveConflictCommandLineOptions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
670ddee to
96229d8
Compare
premun
left a comment
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.
@adamzip I fail to see where we clone the source repos - does that happen naturally inside the codeflowers?
src/Microsoft.DotNet.Darc/Darc/Options/VirtualMonoRepo/ResolveConflictCommandLineOptions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
…lveOperation.cs Co-authored-by: Copilot <[email protected]>
…lveOperation.cs Co-authored-by: Copilot <[email protected]>
…lveOperation.cs Co-authored-by: Copilot <[email protected]>
…lveOperation.cs Co-authored-by: Copilot <[email protected]>
1b2c320 to
75a3927
Compare
| excludedAssets: excludedAssets, | ||
| headBranch, | ||
| headBranch, | ||
| targetRepoUri ?? _vmrInfo.VmrPath, |
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.
Can you take this from my branch?
If targetRepoUri is null, we'd just put there the first remote we find in the VMR dir:
var vmr = _localGitRepoFactory.Create(_vmrInfo.VmrPath);
(await vmr.GetRemotesAsync()).First().Uri
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ForwardFlowOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/BackflowOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
premun
left a comment
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.
All looks good now, just some wording nits. Please give it a test locally and we can merge
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
…lveConflictOperation.cs Co-authored-by: Přemek Vysoký <[email protected]>
src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/ResolveConflictOperation.cs
Outdated
Show resolved
Hide resolved
premun
left a comment
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.
@adamzip please also change the rebase scenario tests to use the new command (and then please run them locally to make sure they go green).
Also when doing so, I think they account for the fact the codeflow commands don't return 0 when conflicts are hit so we have a try/catch there but with your command, we should not expect failures so the try/catch can go.
|
I tried that out and it went green |
Second part of #5312 (comment)
darc vmr resolvecommand that validates PR state, locally cloned repos, and attempts to flow the next build to flow using the rebase strategy.