Skip to content

Unify back and forward flow merge policies #4691

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Unify back and forward flow merge policies #4691

wants to merge 9 commits into from

Conversation

dkurepa
Copy link
Member

@dkurepa dkurepa commented Apr 16, 2025

Previously, we had two separate classes for ForwardFlowMergePolicy and BackwardFlowMergePolicy, and they both had the same MergePolicyName: Codeflow consistency check.
Because of this, we had exceptions when we we're registering both here:

arcade-services/src/ProductConstructionService/ProductConstructionService.DependencyFlow/MergePolicyEvaluator.cs

Line 28 in fdb14b5

MergePolicyBuilders = mergePolicies.ToImmutableDictionary(p => p.Name);

I personally think that it's a good idea to have these have the same name. That way we can be sure that a subscription will only have one codeflow merge policy, and it will be the correct one, there will be no need for validation

#4612

test: maestro-auth-test/maestro-test-vmr#1353

premun
premun approved these changes Apr 16, 2025
View reviewed changes
Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change as it simplifies quite a few things around configuring/running the check

premun
premun requested changes Apr 16, 2025
View reviewed changes
Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not merge this as we first need to merge the revert from production to main

@adamzip
Copy link
Contributor

adamzip commented Apr 17, 2025

FYI, I am working on additional changes to the PR checks , so we are gonna have a bunch of conflicts

dkurepa and others added 2 commits April 17, 2025 17:34
@dkurepa @premun
Improve darc update-subscription by allowing to specify policy chec…
20ff167 
…ks (dotnet#4680)

<!-- 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#4612
dotnet#2599

- Allows subscription merge policy updates from the `update-subscription
command
 - Allows adding explicitly adding coherency check policies
 - Allows specifying `ignore-checks` for the standard automerge policy

---------

Co-authored-by: Přemek Vysoký <premek.vysoky@microsoft.com>
@premun
Move Forward and Backward MergePolicy into one class
e0e6e55
@premun premun force-pushed the dkurepa/MergePolicyEvaluatorFix branch from 45dbfbb to e0e6e55 Compare April 17, 2025 15:35
@premun
Copy link
Member

premun commented Apr 17, 2025

@dkurepa I pushed here the revert of the revert and then the original content of this PR

@adamzip
Copy link
Contributor

adamzip commented Apr 17, 2025

What if you just move the MergePolicyBuilder's from BackFlowMergePolicy and ForwardFlowMergePolicy to their shared base class CodeFlowMergePolicy, without introducing all the new classes and the CodeFlowMergePolicyInterpreterResult?
That should still resolve the issue with MergePolicyBuilders = mergePolicies.ToImmutableDictionary(p => p.Name); , wouldn't it?

@adamzip adamzip self-requested a review April 17, 2025 15:40
@premun
Copy link
Member

premun commented Apr 17, 2025

@adamzip I kind of like that there is just one codeflow policy for both back/forward flow as it's much easier to use when defining subscriptions.

@premun premun changed the title Move ForwardFlowMergePolicy and BackwardFlowPolicy into one class Unify back and forward flow merge policies Apr 17, 2025
@premun
Copy link
Member

premun commented Apr 17, 2025

I found some missing places where the codeflow policy still wasn't supported, I guess this PR will need a bit more work

premun
premun reviewed Apr 17, 2025
View reviewed changes
src/Microsoft.DotNet.Darc/Darc/Operations/SetRepositoryMergePoliciesOperation.cs Outdated
@@ -95,6 +95,16 @@ public override async Task<int> ExecuteAsync()
});
}

if (_options.) // TODO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to handle default branch policies

@adamzip
Copy link
Contributor

adamzip commented Apr 17, 2025

@adamzip I kind of like that there is just one codeflow policy for both back/forward flow as it's much easier to use when defining subscriptions.

Isn't it just a difference in naming? You could keep the current class structure as is, and have CodeFlowMergePolicy, which is the "one codeflow policy" as you say, and maybe rename BackFlowMergePolicy to BackFlowMergePolicyImplementation or something of the like.
You could even keep the change Djuradj added to StandardMergePolicy so that CodeFlowMergePolicy's EvaluateAsync is called and then it calls specific implementations defined in the FF / BF implementations.
Basically everything stays the same except you don't need to create CodeFlowMergePolicyInterpreter, and the FF / BF classes can inherit the strings and the Fail/Succeed methods as usual

@premun
Copy link
Member

premun commented Apr 17, 2025

Yeah sure, that also works

@dkurepa
Copy link
Member Author

dkurepa commented Apr 29, 2025

@adamzip I kind of like that there is just one codeflow policy for both back/forward flow as it's much easier to use when defining subscriptions.

Isn't it just a difference in naming? You could keep the current class structure as is, and have CodeFlowMergePolicy, which is the "one codeflow policy" as you say, and maybe rename BackFlowMergePolicy to BackFlowMergePolicyImplementation or something of the like. You could even keep the change Djuradj added to StandardMergePolicy so that CodeFlowMergePolicy's EvaluateAsync is called and then it calls specific implementations defined in the FF / BF implementations. Basically everything stays the same except you don't need to create CodeFlowMergePolicyInterpreter, and the FF / BF classes can inherit the strings and the Fail/Succeed methods as usual

Yeah fair point, those MergePolicyImplementations we're overkill I think. Will run a test now to quickly test everything

@dkurepa dkurepa requested a review from premun April 29, 2025 13:43
@dkurepa
Copy link
Member Author

dkurepa commented Apr 29, 2025

test: https://github.com/maestro-auth-test/maestro-test-vmr/pull/1542/checks

adamzip
adamzip reviewed Apr 29, 2025
View reviewed changes
src/Maestro/Maestro.MergePolicies/CodeFlowMergePolicy.cs Outdated
{
public override string DisplayName => "Code flow verification";

protected static readonly string configurationErrorsHeader = """
protected static readonly string _configurationErrorsHeader = """
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had looked up the casing convention for protected static fields and the consensus I found was that both underscore and camelcase are acceptable, but it's common to have no underscore.
Not asking this to be changed, just want share the info

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally thought protected static fields are PascalCase

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like that is the case, will fix

premun
premun reviewed Apr 29, 2025
View reviewed changes
Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkurepa before merging this, please try to use darc locally to add this policy onto a subscription to see how it all works together.

@dkurepa
Copy link
Member Author

dkurepa commented Apr 29, 2025

@dkurepa before merging this, please try to use darc locally to add this policy onto a subscription to see how it all works together.

I did run a modified scenario test with a breakpoint where I "corrupted" the source-manifest.json, and then let the service run the code policy check, linked above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamzip adamzip adamzip left review comments

@premun premun premun requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dkurepa @adamzip @premun