-
Notifications
You must be signed in to change notification settings - Fork 80
Include json version file updates in hasUpdates
#5543
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
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrVersionFileMerger.cs
Outdated
Show resolved
Hide resolved
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 fixes a bug where JSON version file updates (global.json and dotnet-tools.json) were not being properly tracked in the hasUpdates flag during backflow operations. The changes ensure that toolset updates are correctly reported as changes.
Key changes:
- Modified
MergeJsonsAsyncto return a boolean indicating whether changes were made - Extracted toolset update logic into a separate
BackflowToolsetsmethod that tracks changes - Updated
VersionFileUpdateResultto include aHasToolsetUpdatesfield - Fixed a documentation comment that incorrectly described conflict resolution behavior
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
VmrVersionFileMerger.cs |
Removed unused parameter and simplified deletion logic; corrected conflict resolution comment |
VmrBackflower.cs |
Added HasToolsetUpdates to the hasChanges calculation |
VersionFileUpdateResult.cs |
Added HasToolsetUpdates field to track toolset changes |
JsonFileMerger.cs |
Changed return type to bool and added logic to detect and return whether changes were made |
BackflowConflictResolver.cs |
Split toolset and dependency updates into separate methods to track toolset changes independently |
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/JsonFileMerger.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.
LGTM
Can we add a test? There probably already are tests for this class so maybe AI can cook up one really fast?
Co-authored-by: Přemek Vysoký <[email protected]>
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrVersionFileMerger.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/JsonFileMerger.cs
Outdated
Show resolved
Hide resolved
| if (jsonDeletedInTarget | ||
| || (jsonDeletedInSource && targetRepoCurrentJson == EmptyJsonString)) | ||
| { | ||
| // the target file is already deleted, nothing more to do | ||
| return false; | ||
| } |
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 like this much better
| { | ||
| bool hasChanges = false; | ||
|
|
||
| //todo - could this be head branch instead of target? |
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.
A leftover?
| var message = | ||
| $""" | ||
| There was a conflict when merging version properties. In file {fileTargetRepoPath}, property '{propertyNameTransformer(property)}' | ||
| was added in the target branch but removed in the source repo. |
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.
Here it also says source repo
#5406
Changes: