Opposite flow merged during open pr by adamzip · Pull Request #5230 · dotnet/arcade-services

adamzip · 2025-09-01T13:40:48Z

#5148
Note: still WIP. The refactoring commit actually includes some functional changes - I should clean it up

Co-authored-by: Přemek Vysoký <[email protected]>

Copilot

Pull Request Overview

This PR implements functionality to handle "opposite flow merged during open PR" scenarios by blocking pull requests from receiving future codeflow updates when conflicting flows have been merged.

Key changes include:

Added detection logic for when opposite direction flows have been merged while a PR is open
Implemented PR blocking mechanism with new BlockingCodeflowException and BlockedFromFutureUpdates property
Enhanced comment system to support template replacements for better user messaging

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
PullRequestUpdater.cs	Refactored code flow execution into separate method and added blocking logic for PRs
PullRequestCommenter.cs	Added template replacement functionality for dynamic comment content
InProgressPullRequest.cs	Added `BlockedFromFutureUpdates` property to track blocked PRs
VmrCodeflower.cs	Added detection logic for opposite flow scenarios and blocking exception
Exceptions.cs	Added new `BlockingCodeflowException` for codeflow blocking scenarios
PcsVmrForwardFlower.cs	Updated constructor to include comment collector dependency
PcsVmrBackFlower.cs	Updated constructor to include comment collector dependency
VmrForwardFlower.cs	Updated constructor to include comment collector dependency
VmrBackflower.cs	Updated constructor to include comment collector dependency

…ndencyFlow/PullRequestUpdater.cs Co-authored-by: Copilot <[email protected]>

dkurepa · 2025-09-02T15:10:10Z

    private readonly ILogger<VmrCodeFlower> _logger;

+    private static readonly string CannotFlowAdditionalFlowsInPrMsg = string.Concat(
+        "The source repository has received code changes from outside of this PR branch. This PR will stop ",


isn't the problem that an opposite direction codeflow was merged?
For example, if we're printing this for a backflow, it means that a forward flow for that repo was merged, causing the next backflow to be considered "opposite direction"

Yes, that's true

dkurepa · 2025-09-02T15:11:23Z

+        "PR must be merged or closed. Once this is done, code flows into the repository will resume normally ",
+        "on the next subscription trigger. In order to force the subscription trigger, merge or close this PR " +
+        "and run the following command using the subscription-id found in this PR. \n",
+        "`darc trigger-subscription --id <subscriptionId>`.");


think this has to be updated for the force parameter?

That message is about closing or merging the PR and triggering the subscription normally (no force needed)

but that is another option, no? if a person wants to trigger another subscription update in the same PR, they can use force

I think this part of the sentence is problematic:

In order to force the subscription trigger

and then it proceeds to give instructions for re-opening while we have 2 options - force-trigger or re-open

premun · 2025-09-03T11:00:11Z

+    private static readonly string CannotFlowAdditionalFlowsInPrMsg = string.Concat(
+        "The source repository has received code changes from an opposite flow. Any additional codeflows into ",
+        "this PR may potentially result in lost changes. Please continue with one of the following options: \n",
+        "1. Close or merge this PR and let the codeflow continue normally. \n",
+        "2. Close or merge this PR and receive the new codeflow immediately by running ",
+        "`darc trigger-subscription --id <subscriptionId>`. \n",
+        "3. Keep flowing changes into this PR anyway by force-triggering the subscription: ",
+        "`darc trigger-subscription --force --id <subscriptionId>`.");


I think the new lines are missing in some of these place. Would it be better to just use a multi-line string?

Suggested change

private static readonly string CannotFlowAdditionalFlowsInPrMsg = string.Concat(

"The source repository has received code changes from an opposite flow. Any additional codeflows into ",

"this PR may potentially result in lost changes. Please continue with one of the following options: \n",

"1. Close or merge this PR and let the codeflow continue normally. \n",

"2. Close or merge this PR and receive the new codeflow immediately by running ",

"`darc trigger-subscription --id <subscriptionId>`. \n",

"3. Keep flowing changes into this PR anyway by force-triggering the subscription: ",

"`darc trigger-subscription --force --id <subscriptionId>`.");

private static readonly string CannotFlowAdditionalFlowsInPrMsg =

"""

The source repository has received code changes from an opposite flow. Any additional codeflows into

this PR may potentially result in lost changes.

Please continue with one of the following options:

1. Close or merge this PR and let the codeflow continue normally

2. Close or merge this PR and receive the new codeflow immediately by running

`darc trigger-subscription --id <subscriptionId>`

3. Force-flow changes into this PR but at your own risk (some PR commits might be reverted):

`darc trigger-subscription --force --id <subscriptionId>`

""";

I think the newlines are in the right place. I avoided multiline string because I didn't want extremely long lines in the editor. Here's what the comment would currently look like on GH:

Important
The source repository has received code changes from an opposite flow. Any additional codeflows into this PR may potentially result in lost changes. Please continue with one of the following options:

Close or merge this PR and let the codeflow continue normally.

Close or merge this PR and receive the new codeflow immediately by running darc trigger-subscription --id <subscriptionId>.

Keep flowing changes into this PR anyway by force-triggering the subscription: darc trigger-subscription --id <subscriptionId>.

In case of unclarities, consult the FAQ or tag \@dotnet/product-construction for assistance.

Ah, that looks good.
Mine will look like this:

Important
The source repository has received code changes from an opposite flow. Any additional codeflows into
this PR may potentially result in lost changes.

Please continue with one of the following options:

Close or merge this PR and let the codeflow continue normally

Close or merge this PR and receive the new codeflow immediately by running
darc trigger-subscription --id <subscriptionId>

Force-flow changes into this PR but at your own risk (some PR commits might be reverted):
darc trigger-subscription --force --id <subscriptionId>

Maybe it's more readable without all those \n and ",?

@adamzip my suggestion also had this part at your own risk (some PR commits might be reverted). Maybe we should keep that?

I also had the commands on their own line so they are easier to copy (3 mouse clicks)

adamzip force-pushed the opposite-flow-merged-during-open-pr branch 2 times, most recently from aa2e9a9 to 838c278 Compare September 1, 2025 14:29

premun reviewed Sep 1, 2025

View reviewed changes

adamzip added 3 commits September 1, 2025 17:58

[REFACTORING] Extract codeflow + exceptions

b4ba5f7

[REFACTORING] Remove unused code

237e401

Block future flows if opposite is merged

3ea0c5a

adamzip force-pushed the opposite-flow-merged-during-open-pr branch from 838c278 to 3ea0c5a Compare September 1, 2025 16:00

adamzip and others added 4 commits September 1, 2025 18:06

Apply suggestion from @premun

714dabd

Co-authored-by: Přemek Vysoký <[email protected]>

Apply suggestion from @premun

4437078

Co-authored-by: Přemek Vysoký <[email protected]>

Apply suggestion from @premun

0d78ff6

Co-authored-by: Přemek Vysoký <[email protected]>

Add real subscription id to PR comment

e8bb21e

adamzip marked this pull request as ready for review September 1, 2025 16:36

Copilot AI review requested due to automatic review settings September 1, 2025 16:36

Merge branch 'main' into opposite-flow-merged-during-open-pr

e726734

Copilot AI reviewed Sep 1, 2025

View reviewed changes

Comment thread src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/VmrCodeflower.cs Outdated

Comment thread src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdater.cs Outdated

adamzip and others added 3 commits September 1, 2025 18:45

Update src/ProductConstructionService/ProductConstructionService.Depe…

0f754c3

…ndencyFlow/PullRequestUpdater.cs Co-authored-by: Copilot <[email protected]>

fix comment spaces

d721e1a

Dont throw blocking ex if forceUpdate=true

65f9208

dkurepa reviewed Sep 2, 2025

View reviewed changes

Add force update option to warning msg

7e0b870

premun reviewed Sep 3, 2025

View reviewed changes

Use multiline string and add extra new line

6ca624d

premun previously approved these changes Sep 3, 2025

View reviewed changes

finalize warning msg

ca99150

adamzip dismissed premun’s stale review via ca99150 September 3, 2025 11:35

premun approved these changes Sep 3, 2025

View reviewed changes

adamzip merged commit ee1c036 into dotnet:main Sep 3, 2025
9 checks passed

Conversation

adamzip commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamzip Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

premun Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adamzip commented Sep 1, 2025 •

edited

Loading

adamzip Sep 3, 2025 •

edited

Loading

premun Sep 3, 2025 •

edited

Loading