Skip to content

Commit ee1c036

Browse files
adamzipadamzip
authored
Opposite flow merged during open pr (#5230)
#5148
1 parent 4d379ca commit ee1c036

13 files changed

Lines changed: 221 additions & 107 deletions

File tree

‎src/Microsoft.DotNet.Darc/Darc/Operations/VirtualMonoRepo/CodeFlowOperation.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ protected async Task FlowCodeLocallyAsync(
126126
tmpTargetBranch,
127127
tmpHeadBranch,
128128
headBranchExisted: false,
129+
forceUpdate: false,
129130
cancellationToken);
130131
}
131132
catch (Exception e)

‎src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/Exceptions.cs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,3 +99,11 @@ public class NonLinearCodeflowException(string currentSha, string previousSha)
9999
: DarcException($"Cannot flow commit {currentSha} as it's not a descendant of previously flown commit {previousSha}")
100100
{
101101
}
102+
103+
/// <summary>
104+
/// This exception is used when the current codeflow cannot be applied, and if a codeflow PR already exists, then it
105+
/// is blocked from receiving new flows.
106+
/// </summary>
107+
public class BlockingCodeflowException(string msg) : Exception(msg)
108+
{
109+
}

‎src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrBackflower.cs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ Task<CodeFlowResult> FlowBackAsync(
3535
IReadOnlyCollection<string>? excludedAssets,
3636
string targetBranch,
3737
string headBranch,
38+
bool forceUpdate,
3839
CancellationToken cancellationToken = default);
3940
}
4041

@@ -67,8 +68,9 @@ public VmrBackFlower(
6768
IBackflowConflictResolver versionFileConflictResolver,
6869
IFileSystem fileSystem,
6970
IBasicBarClient barClient,
71+
ICommentCollector commentCollector,
7072
ILogger<VmrCodeFlower> logger)
71-
: base(vmrInfo, sourceManifest, dependencyTracker, localGitClient, localGitRepoFactory, versionDetailsParser, fileSystem, logger)
73+
: base(vmrInfo, sourceManifest, dependencyTracker, localGitClient, localGitRepoFactory, versionDetailsParser, fileSystem, commentCollector, logger)
7274
{
7375
_vmrInfo = vmrInfo;
7476
_sourceManifest = sourceManifest;
@@ -91,6 +93,7 @@ public async Task<CodeFlowResult> FlowBackAsync(
9193
IReadOnlyCollection<string>? excludedAssets,
9294
string targetBranch,
9395
string headBranch,
96+
bool forceUpdate,
9497
CancellationToken cancellationToken = default)
9598
{
9699
var targetRepo = _localGitRepoFactory.Create(targetRepoPath);
@@ -111,6 +114,7 @@ public async Task<CodeFlowResult> FlowBackAsync(
111114
targetBranch,
112115
headBranch,
113116
headBranchExisted,
117+
forceUpdate,
114118
cancellationToken);
115119
}
116120

@@ -123,6 +127,7 @@ protected async Task<CodeFlowResult> FlowBackAsync(
123127
string targetBranch,
124128
string headBranch,
125129
bool headBranchExisted,
130+
bool forceUpdate,
126131
CancellationToken cancellationToken)
127132
{
128133
var currentFlow = new Backflow(build.Commit, lastFlows.LastFlow.RepoSha);
@@ -136,6 +141,7 @@ protected async Task<CodeFlowResult> FlowBackAsync(
136141
targetBranch,
137142
headBranch,
138143
headBranchExisted,
144+
forceUpdate,
139145
cancellationToken);
140146

141147
// We try to merge the target branch and we apply dependency updates
@@ -168,6 +174,7 @@ protected override async Task<bool> SameDirectionFlowAsync(
168174
string targetBranch,
169175
string headBranch,
170176
bool headBranchExisted,
177+
bool forceUpdate,
171178
CancellationToken cancellationToken)
172179
{
173180
var lastFlownSha = lastFlows.LastFlow.VmrSha;
@@ -247,6 +254,7 @@ await RecreatePreviousFlowsAndApplyChanges(
247254
headBranch,
248255
targetBranch,
249256
excludedAssets,
257+
forceUpdate,
250258
reapplyChanges: async () =>
251259
{
252260
foreach (VmrIngestionPatch patch in patches)

‎src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCodeflower.cs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ Task<bool> FlowCodeAsync(
3232
string targetBranch,
3333
string headBranch,
3434
bool headBranchExisted,
35+
bool forceUpdate,
3536
CancellationToken cancellationToken = default);
3637
}
3738

@@ -48,8 +49,21 @@ public abstract class VmrCodeFlower : IVmrCodeFlower
4849
private readonly ILocalGitRepoFactory _localGitRepoFactory;
4950
private readonly IVersionDetailsParser _versionDetailsParser;
5051
private readonly IFileSystem _fileSystem;
52+
private readonly ICommentCollector _commentCollector;
5153
private readonly ILogger<VmrCodeFlower> _logger;
5254

55+
private static readonly string CannotFlowAdditionalFlowsInPrMsg =
56+
"""
57+
The source repository has received code changes from an opposite flow. Any additional codeflows into this PR may potentially result in lost changes.
58+
59+
Please continue with one of the following options:
60+
1. Close or merge this PR and let the codeflow continue normally
61+
2. Close or merge this PR and receive the new codeflow immediately by triggering the subscription:
62+
`darc trigger-subscription --id <subscriptionId>`
63+
3. Force-flow new changes into this PR at your own risk (some PR commits might be reverted):
64+
`darc trigger-subscription --force --id <subscriptionId>`
65+
""";
66+
5367
protected VmrCodeFlower(
5468
IVmrInfo vmrInfo,
5569
ISourceManifest sourceManifest,
@@ -58,6 +72,7 @@ protected VmrCodeFlower(
5872
ILocalGitRepoFactory localGitRepoFactory,
5973
IVersionDetailsParser versionDetailsParser,
6074
IFileSystem fileSystem,
75+
ICommentCollector commentCollector,
6176
ILogger<VmrCodeFlower> logger)
6277
{
6378
_vmrInfo = vmrInfo;
@@ -67,6 +82,7 @@ protected VmrCodeFlower(
6782
_localGitRepoFactory = localGitRepoFactory;
6883
_versionDetailsParser = versionDetailsParser;
6984
_fileSystem = fileSystem;
85+
_commentCollector = commentCollector;
7086
_logger = logger;
7187
}
7288

@@ -86,6 +102,7 @@ public async Task<bool> FlowCodeAsync(
86102
string targetBranch,
87103
string headBranch,
88104
bool headBranchExisted,
105+
bool forceUpdate,
89106
CancellationToken cancellationToken = default)
90107
{
91108
var lastFlow = lastFlows.LastFlow;
@@ -95,6 +112,12 @@ public async Task<bool> FlowCodeAsync(
95112
return false;
96113
}
97114

115+
if (lastFlow.Name == currentFlow.Name && headBranchExisted && !forceUpdate)
116+
{
117+
_commentCollector.AddComment(CannotFlowAdditionalFlowsInPrMsg, CommentType.Warning);
118+
throw new BlockingCodeflowException("Cannot apply codeflow on PR head branch because an opposite direction flow has been merged.");
119+
}
120+
98121
await EnsureCodeflowLinearityAsync(repo, currentFlow, lastFlows);
99122

100123
_logger.LogInformation("Last flow was {type} flow: {sourceSha} -> {targetSha}",
@@ -116,6 +139,7 @@ public async Task<bool> FlowCodeAsync(
116139
targetBranch,
117140
headBranch,
118141
headBranchExisted,
142+
forceUpdate,
119143
cancellationToken);
120144
}
121145
else
@@ -165,6 +189,7 @@ protected abstract Task<bool> SameDirectionFlowAsync(
165189
string targetBranch,
166190
string headBranch,
167191
bool headBranchExisted,
192+
bool forceUpdate,
168193
CancellationToken cancellationToken);
169194

170195
/// <summary>
@@ -343,6 +368,7 @@ protected async Task RecreatePreviousFlowsAndApplyChanges(
343368
string headBranch,
344369
string targetBranch,
345370
IReadOnlyCollection<string>? excludedAssets,
371+
bool forceUpdate,
346372
Func<Task> reapplyChanges,
347373
CancellationToken cancellationToken)
348374
{
@@ -392,6 +418,7 @@ await FlowCodeAsync(
392418
targetBranch,
393419
headBranch,
394420
headBranchExisted: true, // Head branch was created when we rewound to the previous flow
421+
forceUpdate,
395422
cancellationToken);
396423

397424
var changedFilesAfterRecreation = await GetChangesInHeadBranch(

‎src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrForwardFlower.cs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public interface IVmrForwardFlower : IVmrCodeFlower
3232
/// <param name="targetBranch">Target branch to create the PR against. If target branch does not exist, it is created off of this branch</param>
3333
/// <param name="headBranch">New/existing branch to make the changes on</param>
3434
/// <param name="targetVmrUri">URI of the VMR to update</param>
35-
/// <param name="skipMeaninglessUpdates">Skip creating PR if only insignificant changes are present</param>
35+
/// <param name="forceUpdate">Force the update to be performed</param>
3636
/// <returns>CodeFlowResult containing information about the codeflow calculation</returns>
3737
Task<CodeFlowResult> FlowForwardAsync(
3838
string mapping,
@@ -42,7 +42,7 @@ Task<CodeFlowResult> FlowForwardAsync(
4242
string targetBranch,
4343
string headBranch,
4444
string targetVmrUri,
45-
bool skipMeaninglessUpdates = false,
45+
bool forceUpdate,
4646
CancellationToken cancellationToken = default);
4747
}
4848

@@ -76,8 +76,9 @@ public VmrForwardFlower(
7676
IProcessManager processManager,
7777
IBasicBarClient barClient,
7878
IFileSystem fileSystem,
79+
ICommentCollector commentCollector,
7980
ILogger<VmrCodeFlower> logger)
80-
: base(vmrInfo, sourceManifest, dependencyTracker, localGitClient, localGitRepoFactory, versionDetailsParser, fileSystem, logger)
81+
: base(vmrInfo, sourceManifest, dependencyTracker, localGitClient, localGitRepoFactory, versionDetailsParser, fileSystem, commentCollector, logger)
8182
{
8283
_vmrInfo = vmrInfo;
8384
_sourceManifest = sourceManifest;
@@ -101,7 +102,7 @@ public async Task<CodeFlowResult> FlowForwardAsync(
101102
string targetBranch,
102103
string headBranch,
103104
string targetVmrUri,
104-
bool skipMeaninglessUpdates = false,
105+
bool forceUpdate,
105106
CancellationToken cancellationToken = default)
106107
{
107108
ILocalGitRepo sourceRepo = _localGitRepoFactory.Create(repoPath);
@@ -124,6 +125,7 @@ public async Task<CodeFlowResult> FlowForwardAsync(
124125
targetBranch,
125126
headBranch,
126127
headBranchExisted,
128+
forceUpdate,
127129
cancellationToken);
128130

129131
IReadOnlyCollection<UnixPath>? conflictedFiles = null;
@@ -143,8 +145,8 @@ public async Task<CodeFlowResult> FlowForwardAsync(
143145
cancellationToken);
144146
}
145147

146-
// We try to detect if the changes were meaningful and it's worth creating a new PR
147-
if (conflictedFiles != null && skipMeaninglessUpdates && hasChanges && !headBranchExisted)
148+
// If we don't force the update, we'll set hasChanges to false when the updates are not meaningful
149+
if (conflictedFiles != null && !forceUpdate && hasChanges && !headBranchExisted)
148150
{
149151
hasChanges &= await _codeflowChangeAnalyzer.ForwardFlowHasMeaningfulChangesAsync(mapping.Name, headBranch, targetBranch);
150152
}
@@ -223,6 +225,7 @@ protected override async Task<bool> SameDirectionFlowAsync(
223225
string targetBranch,
224226
string headBranch,
225227
bool headBranchExisted,
228+
bool forceUpdate,
226229
CancellationToken cancellationToken)
227230
{
228231
try
@@ -256,6 +259,7 @@ await RecreatePreviousFlowsAndApplyChanges(
256259
headBranch,
257260
targetBranch,
258261
excludedAssets,
262+
forceUpdate,
259263
reapplyChanges: async () =>
260264
{
261265
hadChanges = await _vmrUpdater.UpdateRepository(

‎src/ProductConstructionService/ProductConstructionService.DependencyFlow/InProgressPullRequest.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ public class InProgressPullRequest : DependencyFlowWorkItem
5555
public Dictionary<Guid, int> NextBuildsToProcess { get; set; } = [];
5656

5757
public CodeFlowDirection CodeFlowDirection { get; set; }
58+
59+
public bool BlockedFromFutureUpdates { get; set; } = false;
5860
}
5961

6062
public enum InProgressPullRequestState

‎src/ProductConstructionService/ProductConstructionService.DependencyFlow/PcsVmrBackFlower.cs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ Task<CodeFlowResult> FlowBackAsync(
3333
Subscription subscription,
3434
Build build,
3535
string targetBranch,
36+
bool forceApply,
3637
CancellationToken cancellationToken = default);
3738
}
3839

@@ -58,8 +59,9 @@ public PcsVmrBackFlower(
5859
IBackflowConflictResolver versionFileConflictResolver,
5960
IFileSystem fileSystem,
6061
IBasicBarClient barClient,
62+
ICommentCollector commentCollector,
6163
ILogger<VmrCodeFlower> logger)
62-
: base(vmrInfo, sourceManifest, dependencyTracker, vmrCloneManager, repositoryCloneManager, localGitClient, localGitRepoFactory, versionDetailsParser, vmrPatchHandler, workBranchFactory, versionFileConflictResolver, fileSystem, barClient, logger)
64+
: base(vmrInfo, sourceManifest, dependencyTracker, vmrCloneManager, repositoryCloneManager, localGitClient, localGitRepoFactory, versionDetailsParser, vmrPatchHandler, workBranchFactory, versionFileConflictResolver, fileSystem, barClient, commentCollector, logger)
6365
{
6466
_sourceManifest = sourceManifest;
6567
_dependencyTracker = dependencyTracker;
@@ -72,6 +74,7 @@ public async Task<CodeFlowResult> FlowBackAsync(
7274
Subscription subscription,
7375
Build build,
7476
string targetBranch,
77+
bool forceUpdate,
7578
CancellationToken cancellationToken = default)
7679
{
7780
(var headBranchExisted, SourceMapping mapping, ILocalGitRepo targetRepo) = await PrepareVmrAndRepo(
@@ -91,6 +94,7 @@ public async Task<CodeFlowResult> FlowBackAsync(
9194
subscription.TargetBranch,
9295
targetBranch,
9396
headBranchExisted,
97+
forceUpdate,
9498
cancellationToken);
9599

96100
return result with

‎src/ProductConstructionService/ProductConstructionService.DependencyFlow/PcsVmrForwardFlower.cs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,12 @@ internal interface IPcsVmrForwardFlower
2222
/// <param name="subscription">Subscription to flow</param>
2323
/// <param name="build">Build to flow</param>
2424
/// <param name="headBranch">Branch to flow to (or to create)</param>
25-
/// <param name="skipMeaninglessUpdates">Skip creating PR if only insignificant changes are present</param>
25+
/// <param name="forceUpdate">Force the update to be performed</param>
2626
Task<CodeFlowResult> FlowForwardAsync(
2727
Subscription subscription,
2828
Build build,
2929
string headBranch,
30-
bool skipMeaninglessUpdates,
30+
bool forceUpdate,
3131
CancellationToken cancellationToken = default);
3232
}
3333

@@ -51,8 +51,9 @@ public PcsVmrForwardFlower(
5151
IProcessManager processManager,
5252
IBasicBarClient barClient,
5353
IFileSystem fileSystem,
54+
ICommentCollector commentCollector,
5455
ILogger<VmrCodeFlower> logger)
55-
: base(vmrInfo, sourceManifest, vmrUpdater, dependencyTracker, vmrCloneManager, localGitClient, localGitRepoFactory, versionDetailsParser, codeflowChangeAnalyzer, conflictResolver, workBranchFactory, processManager, barClient, fileSystem, logger)
56+
: base(vmrInfo, sourceManifest, vmrUpdater, dependencyTracker, vmrCloneManager, localGitClient, localGitRepoFactory, versionDetailsParser, codeflowChangeAnalyzer, conflictResolver, workBranchFactory, processManager, barClient, fileSystem, commentCollector, logger)
5657
{
5758
_repositoryCloneManager = repositoryCloneManager;
5859
}
@@ -61,7 +62,7 @@ public async Task<CodeFlowResult> FlowForwardAsync(
6162
Subscription subscription,
6263
Build build,
6364
string headBranch,
64-
bool skipMeaninglessUpdates,
65+
bool forceUpdate,
6566
CancellationToken cancellationToken = default)
6667
{
6768
ILocalGitRepo sourceRepo = await _repositoryCloneManager.PrepareCloneAsync(
@@ -78,7 +79,7 @@ public async Task<CodeFlowResult> FlowForwardAsync(
7879
subscription.TargetBranch,
7980
headBranch,
8081
subscription.TargetRepository,
81-
skipMeaninglessUpdates,
82+
forceUpdate,
8283
cancellationToken);
8384
}
8485

‎src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestCommenter.cs‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ internal interface IPullRequestCommenter
1919
/// </summary>
2020
/// <param name="prUrl">The URL of the pull request to comment on</param>
2121
/// <param name="targetRepository">The target repository URL</param>
22-
Task PostCollectedCommentsAsync(string prUrl, string targetRepository);
22+
Task PostCollectedCommentsAsync(
23+
string prUrl,
24+
string targetRepository,
25+
IEnumerable<(string oldValue, string newValue)> replacements);
2326
}
2427

2528
internal class PullRequestCommenter : IPullRequestCommenter
@@ -40,7 +43,10 @@ public PullRequestCommenter(
4043
_logger = logger;
4144
}
4245

43-
public async Task PostCollectedCommentsAsync(string prUrl, string targetRepository)
46+
public async Task PostCollectedCommentsAsync(
47+
string prUrl,
48+
string targetRepository,
49+
IEnumerable<(string oldValue, string newValue)> replacements)
4450
{
4551
var comments = _commentService.GetComments();
4652
if (comments.Count == 0)
@@ -59,9 +65,17 @@ public async Task PostCollectedCommentsAsync(string prUrl, string targetReposito
5965
CommentType.Information => "> [!NOTE]",
6066
_ => throw new ArgumentOutOfRangeException($"Comment type {comment.commentType} is not supported")
6167
};
68+
69+
var commentText = comment.Text;
70+
71+
foreach (var (oldValue, newValue) in replacements)
72+
{
73+
commentText = commentText.Replace(oldValue, newValue);
74+
}
75+
6276
StringBuilder sb = new();
6377
sb.AppendLine(header);
64-
foreach (var textLine in comment.Text.Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries))
78+
foreach (var textLine in commentText.Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries))
6579
{
6680
sb.AppendLine($"> {textLine}");
6781
}

0 commit comments

Comments
 (0)